[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2021-02-13 Thread Tyker via Phabricator via cfe-commits 
Tyker added a comment.

I added the changes to the langref in https://reviews.llvm.org/D96646


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2021-02-09 Thread Johannes Doerfert via Phabricator via cfe-commits 
jdoerfert requested changes to this revision.
jdoerfert added a comment.
This revision now requires changes to proceed.

status change, see last message


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2021-02-09 Thread Johannes Doerfert via Phabricator via cfe-commits 
jdoerfert reopened this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

The lang ref needs to reflect the new type 
https://llvm.org/docs/LangRef.html#llvm-var-annotation-intrinsic
Please also review the auto-upgrade patch for this: 
https://reviews.llvm.org/D95993


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-11-06 Thread Victor Campos via Phabricator via cfe-commits 
vhscampos added a comment.

@Tyker This is causing another build failure in another example:

  [2858/4034] Building CXX object 
tools/clang/examples/Attribute/CMakeFiles/Attribute.dir/Attribute.cpp.o
  FAILED: 
tools/clang/examples/Attribute/CMakeFiles/Attribute.dir/Attribute.cpp.o 
  CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /usr/lib/ccache/clang++ 
-DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS 
-D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/clang/examples/Attribute 
-I/home/vicspe01/workspace/upstream/llvm-project/clang/examples/Attribute 
-I/home/vicspe01/workspace/upstream/llvm-project/clang/include 
-Itools/clang/include -Iinclude 
-I/home/vicspe01/workspace/upstream/llvm-project/llvm/include -fPIC 
-fvisibility-inlines-hidden -Werror=date-time 
-Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter 
-Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic 
-Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default 
-Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor 
-Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections 
-fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3  -fPIC  
-fno-exceptions -fno-rtti -UNDEBUG -std=c++14 -MD -MT 
tools/clang/examples/Attribute/CMakeFiles/Attribute.dir/Attribute.cpp.o -MF 
tools/clang/examples/Attribute/CMakeFiles/Attribute.dir/Attribute.cpp.o.d -o 
tools/clang/examples/Attribute/CMakeFiles/Attribute.dir/Attribute.cpp.o -c 
/home/vicspe01/workspace/upstream/llvm-project/clang/examples/Attribute/Attribute.cpp
  
/home/vicspe01/workspace/upstream/llvm-project/clang/examples/Attribute/Attribute.cpp:73:16:
 error: no matching function for call to 'Create'
  D->addAttr(AnnotateAttr::Create(S.Context, "example(" + Str.str() + ")",
 ^~~~
  tools/clang/include/clang/AST/Attrs.inc:885:24: note: candidate function not 
viable: requires at least 4 arguments, but 3 were provided
static AnnotateAttr *Create(ASTContext , llvm::StringRef Annotation, 
Expr * *Args, unsigned ArgsSize, const AttributeCommonInfo  = 
{SourceRange{}});
 ^
  tools/clang/include/clang/AST/Attrs.inc:887:24: note: candidate function not 
viable: requires 6 arguments, but 3 were provided
static AnnotateAttr *Create(ASTContext , llvm::StringRef Annotation, 
Expr * *Args, unsigned ArgsSize, SourceRange Range, AttributeCommonInfo::Syntax 
Syntax);
 ^
  1 error generated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-27 Thread Tyker via Phabricator via cfe-commits 
Tyker added a comment.

In D88645#2353152 , @thakis wrote:

> Looks like this broke tests: http://45.33.8.238/linux/31159/step_12.txt
>
> Please take a look, and revert for now if it takes a while to fix.

this is fixed by 4afa077899b 


In D88645#2354196 , @john.brawn wrote:

> This also causes the AnnotateFunctions example to no longer build:
>
>   
> /home/jb/work/llvm-project/clang/examples/AnnotateFunctions/AnnotateFunctions.cpp:
>  In member function ‘virtual bool 
> {anonymous}::AnnotateFunctionsConsumer::HandleTopLevelDecl(clang::DeclGroupRef)’:
>   
> /home/jb/work/llvm-project/clang/examples/AnnotateFunctions/AnnotateFunctions.cpp:36:70:
>  error: no matching function for call to 
> ‘clang::AnnotateAttr::CreateImplicit(clang::ASTContext&, const char [19])’
> "example_annotation"));
> ^
>
> it looks like it needs to be adjusted for the extra variadic arg that the 
> attribute now has.

this is fixed by 2618247c61c 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-26 Thread John Brawn via Phabricator via cfe-commits 
john.brawn added a comment.

This also causes the AnnotateFunctions example to no longer build:

  
/home/jb/work/llvm-project/clang/examples/AnnotateFunctions/AnnotateFunctions.cpp:
 In member function ‘virtual bool 
{anonymous}::AnnotateFunctionsConsumer::HandleTopLevelDecl(clang::DeclGroupRef)’:
  
/home/jb/work/llvm-project/clang/examples/AnnotateFunctions/AnnotateFunctions.cpp:36:70:
 error: no matching function for call to 
‘clang::AnnotateAttr::CreateImplicit(clang::ASTContext&, const char [19])’
"example_annotation"));
^

it looks like it needs to be adjusted for the extra variadic arg that the 
attribute now has.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-26 Thread Nico Weber via Phabricator via cfe-commits 
thakis added a comment.

Looks like this broke tests: http://45.33.8.238/linux/31159/step_12.txt

Please take a look, and revert for now if it takes a while to fix.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-26 Thread Tyker via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd3205bbca3e0: [Annotation] Allows annotation to carry some 
additional constant arguments. (authored by Tyker).

Changed prior to commit:
  https://reviews.llvm.org/D88645?vs=300213=300608#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/CodeGen/annotations-field.c
  clang/test/CodeGen/annotations-global.c
  clang/test/CodeGen/annotations-loc.c
  clang/test/CodeGen/annotations-var.c
  clang/test/CodeGenCXX/attr-annotate.cpp
  clang/test/CodeGenCXX/attr-annotate2.cpp
  clang/test/Misc/pragma-attribute-cxx.cpp
  clang/test/Misc/pragma-attribute-objc.m
  clang/test/Parser/access-spec-attrs.cpp
  clang/test/Parser/objc-implementation-attrs.m
  clang/test/Sema/annotate.c
  clang/test/Sema/pragma-attribute.c
  clang/test/SemaCXX/attr-annotate.cpp
  llvm/include/llvm/IR/Intrinsics.td
  llvm/test/Analysis/CostModel/X86/free-intrinsics.ll
  llvm/test/Analysis/CostModel/free-intrinsics-datalayout.ll
  llvm/test/Analysis/CostModel/free-intrinsics-no_info.ll
  llvm/test/CodeGen/Generic/ptr-annotate.ll
  llvm/test/Transforms/InstCombine/assume_inevitable.ll

Index: llvm/test/Transforms/InstCombine/assume_inevitable.ll
===
--- llvm/test/Transforms/InstCombine/assume_inevitable.ll
+++ llvm/test/Transforms/InstCombine/assume_inevitable.ll
@@ -15,7 +15,7 @@
 ; CHECK-NEXT:[[DUMMY_EQ:%.*]] = icmp ugt i32 [[LOADRES]], 42
 ; CHECK-NEXT:tail call void @llvm.assume(i1 [[DUMMY_EQ]])
 ; CHECK-NEXT:[[M_I8:%.*]] = bitcast i64* [[M]] to i8*
-; CHECK-NEXT:[[M_A:%.*]] = call i8* @llvm.ptr.annotation.p0i8(i8* nonnull [[M_I8]], i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i64 0, i64 0), i32 2)
+; CHECK-NEXT:[[M_A:%.*]] = call i8* @llvm.ptr.annotation.p0i8(i8* nonnull [[M_I8]], i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i64 0, i64 0), i32 2, i8* null)
 ; CHECK-NEXT:[[M_X:%.*]] = bitcast i8* [[M_A]] to i64*
 ; CHECK-NEXT:[[OBJSZ:%.*]] = call i64 @llvm.objectsize.i64.p0i8(i8* [[C:%.*]], i1 false, i1 false, i1 false)
 ; CHECK-NEXT:store i64 [[OBJSZ]], i64* [[M_X]], align 4
@@ -44,7 +44,7 @@
   call void @llvm.lifetime.end.p0i8(i64 1, i8* %dummy)
 
   %m_i8 = bitcast i64* %m to i8*
-  %m_a = call i8* @llvm.ptr.annotation.p0i8(i8* %m_i8, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2)
+  %m_a = call i8* @llvm.ptr.annotation.p0i8(i8* %m_i8, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2, i8* null)
   %m_x = bitcast i8* %m_a to i64*
   %objsz = call i64 @llvm.objectsize.i64.p0i8(i8* %c, i1 false)
   store i64 %objsz, i64* %m_x
@@ -64,7 +64,7 @@
 
 declare i64 @llvm.objectsize.i64.p0i8(i8*, i1)
 declare i32 @llvm.annotation.i32(i32, i8*, i8*, i32)
-declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32)
+declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32, i8*)
 
 declare void @llvm.lifetime.start.p0i8(i64, i8* nocapture)
 declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture)
Index: llvm/test/CodeGen/Generic/ptr-annotate.ll
===
--- llvm/test/CodeGen/Generic/ptr-annotate.ll
+++ llvm/test/CodeGen/Generic/ptr-annotate.ll
@@ -10,9 +10,9 @@
 define void @foo() {
 entry:
   %m = alloca i8, align 4
-  %0 = call i8* @llvm.ptr.annotation.p0i8(i8* %m, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2)
+  %0 = call i8* @llvm.ptr.annotation.p0i8(i8* %m, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2, i8* null)
   store i8 1, i8* %0, align 4
   ret void
 }
 
-declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32) #1
+declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32, i8*) #1
Index: llvm/test/Analysis/CostModel/free-intrinsics-no_info.ll
===
---

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-23 Thread Tyker via Phabricator via cfe-commits 
Tyker updated this revision to Diff 300213.
Tyker added a comment.

In D88645#2347731 , @aaron.ballman 
wrote:

> In D88645#2347725 , @Tyker wrote:
>
>> In D88645#2347050 , @aaron.ballman 
>> wrote:
>>
>>> LGTM aside from a request for a comment to be added. Thank you!
>>
>> do you mean an RFC on llvm-dev/cfe-dev ?
>
> Oh gosh no! I just meant I was asking for a comment to be added in 
> SemaDeclAttr.td before you commit. Sorry for the confusion! :-)

Thank for the review.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/CodeGen/annotations-field.c
  clang/test/CodeGen/annotations-global.c
  clang/test/CodeGen/annotations-loc.c
  clang/test/CodeGen/annotations-var.c
  clang/test/CodeGenCXX/attr-annotate.cpp
  clang/test/CodeGenCXX/attr-annotate2.cpp
  clang/test/Misc/pragma-attribute-cxx.cpp
  clang/test/Misc/pragma-attribute-objc.m
  clang/test/Parser/access-spec-attrs.cpp
  clang/test/Parser/objc-implementation-attrs.m
  clang/test/Sema/annotate.c
  clang/test/Sema/pragma-attribute.c
  clang/test/SemaCXX/attr-annotate.cpp
  llvm/include/llvm/IR/Intrinsics.td
  llvm/test/Analysis/CostModel/X86/free-intrinsics.ll
  llvm/test/Analysis/CostModel/free-intrinsics-datalayout.ll
  llvm/test/Analysis/CostModel/free-intrinsics-no_info.ll
  llvm/test/CodeGen/Generic/ptr-annotate.ll
  llvm/test/Transforms/InstCombine/assume_inevitable.ll

Index: llvm/test/Transforms/InstCombine/assume_inevitable.ll
===
--- llvm/test/Transforms/InstCombine/assume_inevitable.ll
+++ llvm/test/Transforms/InstCombine/assume_inevitable.ll
@@ -15,7 +15,7 @@
 ; CHECK-NEXT:[[DUMMY_EQ:%.*]] = icmp ugt i32 [[LOADRES]], 42
 ; CHECK-NEXT:tail call void @llvm.assume(i1 [[DUMMY_EQ]])
 ; CHECK-NEXT:[[M_I8:%.*]] = bitcast i64* [[M]] to i8*
-; CHECK-NEXT:[[M_A:%.*]] = call i8* @llvm.ptr.annotation.p0i8(i8* nonnull [[M_I8]], i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i64 0, i64 0), i32 2)
+; CHECK-NEXT:[[M_A:%.*]] = call i8* @llvm.ptr.annotation.p0i8(i8* nonnull [[M_I8]], i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i64 0, i64 0), i32 2, i8* null)
 ; CHECK-NEXT:[[M_X:%.*]] = bitcast i8* [[M_A]] to i64*
 ; CHECK-NEXT:[[OBJSZ:%.*]] = call i64 @llvm.objectsize.i64.p0i8(i8* [[C:%.*]], i1 false, i1 false, i1 false)
 ; CHECK-NEXT:store i64 [[OBJSZ]], i64* [[M_X]], align 4
@@ -44,7 +44,7 @@
   call void @llvm.lifetime.end.p0i8(i64 1, i8* %dummy)
 
   %m_i8 = bitcast i64* %m to i8*
-  %m_a = call i8* @llvm.ptr.annotation.p0i8(i8* %m_i8, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2)
+  %m_a = call i8* @llvm.ptr.annotation.p0i8(i8* %m_i8, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2, i8* null)
   %m_x = bitcast i8* %m_a to i64*
   %objsz = call i64 @llvm.objectsize.i64.p0i8(i8* %c, i1 false)
   store i64 %objsz, i64* %m_x
@@ -64,7 +64,7 @@
 
 declare i64 @llvm.objectsize.i64.p0i8(i8*, i1)
 declare i32 @llvm.annotation.i32(i32, i8*, i8*, i32)
-declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32)
+declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32, i8*)
 
 declare void @llvm.lifetime.start.p0i8(i64, i8* nocapture)
 declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture)
Index: llvm/test/CodeGen/Generic/ptr-annotate.ll
===
--- llvm/test/CodeGen/Generic/ptr-annotate.ll
+++ llvm/test/CodeGen/Generic/ptr-annotate.ll
@@ -10,9 +10,9 @@
 define void @foo() {
 entry:
   %m = alloca i8, align 4
-  %0 = call i8* @llvm.ptr.annotation.p0i8(i8* %m, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2)
+  %0 = call i8* @llvm.ptr.annotation.p0i8(i8* %m, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2, i8* null)
   store i8 1, i8* %0, align 4
   ret void
 }
 
-declare i8*

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-22 Thread Erich Keane via Phabricator via cfe-commits 
erichkeane added a comment.

In D88645#2347725 , @Tyker wrote:

> In D88645#2347050 , @aaron.ballman 
> wrote:
>
>> LGTM aside from a request for a comment to be added. Thank you!
>
> do you mean an RFC on llvm-dev/cfe-dev ?

He means this review comment he made requesting a comment in the code: >Ah, 
thank you for explaining that! Can you add a comment to the code to make that 
clear for others who may run across it?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-22 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

In D88645#2347725 , @Tyker wrote:

> In D88645#2347050 , @aaron.ballman 
> wrote:
>
>> LGTM aside from a request for a comment to be added. Thank you!
>
> do you mean an RFC on llvm-dev/cfe-dev ?

Oh gosh no! I just meant I was asking for a comment to be added in 
SemaDeclAttr.td before you commit. Sorry for the confusion! :-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-22 Thread Tyker via Phabricator via cfe-commits 
Tyker added a comment.

In D88645#2347050 , @aaron.ballman 
wrote:

> LGTM aside from a request for a comment to be added. Thank you!

do you mean an RFC on llvm-dev/cfe-dev ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-22 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from a request for a comment to be added. Thank you!




Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3706
+
+if (!Result || !Notes.empty()) {
+  Diag(E->getBeginLoc(), diag::err_attribute_argument_n_type)

Tyker wrote:
> aaron.ballman wrote:
> > I'm surprised that the presence of notes alone would mean the attribute 
> > argument has an error and should be skipped. Are there circumstances under 
> > which `Result` is true but `Notes` is non-empty?
> AFAIK
> Result means the expression can be folded to a constant.
> Note.empty() means the expression is a valid constant expression in the 
> current language mode.
> 
> the difference is observable in code like:
> ```
> constexpr int foldable_but_invalid() {
>   int *A = new int(0);
>   return *A;
> }
> 
> [[clang::annotate("", foldable_but_invalid())]] void f1() {}
> ```
> foldable_but_invalid retruns a constant but any constant evaluation of this 
> function is invalid because it doesn't desallocate A.
> with !Notes.empty() this fails, without it no errors occurs.
> 
> i think it is desirable that attributes don't diverge from the language mode.
> I added a test for this.
Ah, thank you for explaining that! Can you add a comment to the code to make 
that clear for others who may run across it?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-20 Thread Tyker via Phabricator via cfe-commits 
Tyker added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3706
+
+if (!Result || !Notes.empty()) {
+  Diag(E->getBeginLoc(), diag::err_attribute_argument_n_type)

aaron.ballman wrote:
> I'm surprised that the presence of notes alone would mean the attribute 
> argument has an error and should be skipped. Are there circumstances under 
> which `Result` is true but `Notes` is non-empty?
AFAIK
Result means the expression can be folded to a constant.
Note.empty() means the expression is a valid constant expression in the current 
language mode.

the difference is observable in code like:
```
constexpr int foldable_but_invalid() {
  int *A = new int(0);
  return *A;
}

[[clang::annotate("", foldable_but_invalid())]] void f1() {}
```
foldable_but_invalid retruns a constant but any constant evaluation of this 
function is invalid because it doesn't desallocate A.
with !Notes.empty() this fails, without it no errors occurs.

i think it is desirable that attributes don't diverge from the language mode.
I added a test for this.



Comment at: clang/test/SemaCXX/attr-annotate.cpp:96
+}
+
+void test() {}

aaron.ballman wrote:
> Some more tests for various constant expression situations that may be weird:
> ```
> int n = 10;
> int vla[n];
> 
> [[clang::annotate("vlas are awful", sizeof(vla[++n]))] int i = 0; // reject, 
> the sizeof is not unevaluated
> 
> [[clang::annotate("_Generic selection expression should be fine", _Generic(n, 
> int : 0, default : 1))]] int j = 0; // second arg should resolve to 0 fine
> 
> void designator();
> [[clang::annotate("function designators?", designator)]] int k = 0; // Should 
> work?
> ```
added CK_FunctionToPointerDecay cast when needed
the rest was working as is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-20 Thread Tyker via Phabricator via cfe-commits 
Tyker updated this revision to Diff 299333.
Tyker marked 4 inline comments as done.
Tyker added a comment.
Herald added a subscriber: dexonsmith.

addressed comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/CodeGen/annotations-field.c
  clang/test/CodeGen/annotations-global.c
  clang/test/CodeGen/annotations-loc.c
  clang/test/CodeGen/annotations-var.c
  clang/test/CodeGenCXX/attr-annotate.cpp
  clang/test/CodeGenCXX/attr-annotate2.cpp
  clang/test/Misc/pragma-attribute-cxx.cpp
  clang/test/Misc/pragma-attribute-objc.m
  clang/test/Parser/access-spec-attrs.cpp
  clang/test/Parser/objc-implementation-attrs.m
  clang/test/Sema/annotate.c
  clang/test/Sema/pragma-attribute.c
  clang/test/SemaCXX/attr-annotate.cpp
  llvm/include/llvm/IR/Intrinsics.td
  llvm/test/Analysis/CostModel/X86/free-intrinsics.ll
  llvm/test/Analysis/CostModel/free-intrinsics-datalayout.ll
  llvm/test/Analysis/CostModel/free-intrinsics-no_info.ll
  llvm/test/CodeGen/Generic/ptr-annotate.ll
  llvm/test/Transforms/InstCombine/assume_inevitable.ll

Index: llvm/test/Transforms/InstCombine/assume_inevitable.ll
===
--- llvm/test/Transforms/InstCombine/assume_inevitable.ll
+++ llvm/test/Transforms/InstCombine/assume_inevitable.ll
@@ -15,7 +15,7 @@
 ; CHECK-NEXT:[[DUMMY_EQ:%.*]] = icmp ugt i32 [[LOADRES]], 42
 ; CHECK-NEXT:tail call void @llvm.assume(i1 [[DUMMY_EQ]])
 ; CHECK-NEXT:[[M_I8:%.*]] = bitcast i64* [[M]] to i8*
-; CHECK-NEXT:[[M_A:%.*]] = call i8* @llvm.ptr.annotation.p0i8(i8* nonnull [[M_I8]], i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i64 0, i64 0), i32 2)
+; CHECK-NEXT:[[M_A:%.*]] = call i8* @llvm.ptr.annotation.p0i8(i8* nonnull [[M_I8]], i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i64 0, i64 0), i32 2, i8* null)
 ; CHECK-NEXT:[[M_X:%.*]] = bitcast i8* [[M_A]] to i64*
 ; CHECK-NEXT:[[OBJSZ:%.*]] = call i64 @llvm.objectsize.i64.p0i8(i8* [[C:%.*]], i1 false, i1 false, i1 false)
 ; CHECK-NEXT:store i64 [[OBJSZ]], i64* [[M_X]], align 4
@@ -44,7 +44,7 @@
   call void @llvm.lifetime.end.p0i8(i64 1, i8* %dummy)
 
   %m_i8 = bitcast i64* %m to i8*
-  %m_a = call i8* @llvm.ptr.annotation.p0i8(i8* %m_i8, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2)
+  %m_a = call i8* @llvm.ptr.annotation.p0i8(i8* %m_i8, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2, i8* null)
   %m_x = bitcast i8* %m_a to i64*
   %objsz = call i64 @llvm.objectsize.i64.p0i8(i8* %c, i1 false)
   store i64 %objsz, i64* %m_x
@@ -64,7 +64,7 @@
 
 declare i64 @llvm.objectsize.i64.p0i8(i8*, i1)
 declare i32 @llvm.annotation.i32(i32, i8*, i8*, i32)
-declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32)
+declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32, i8*)
 
 declare void @llvm.lifetime.start.p0i8(i64, i8* nocapture)
 declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture)
Index: llvm/test/CodeGen/Generic/ptr-annotate.ll
===
--- llvm/test/CodeGen/Generic/ptr-annotate.ll
+++ llvm/test/CodeGen/Generic/ptr-annotate.ll
@@ -10,9 +10,9 @@
 define void @foo() {
 entry:
   %m = alloca i8, align 4
-  %0 = call i8* @llvm.ptr.annotation.p0i8(i8* %m, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2)
+  %0 = call i8* @llvm.ptr.annotation.p0i8(i8* %m, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2, i8* null)
   store i8 1, i8* %0, align 4
   ret void
 }
 
-declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32) #1
+declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32, i8*) #1
Index: llvm/test/Analysis/CostModel/free-intrinsics-no_info.ll
===
--- llvm/test/Analysis/CostModel/free-intrinsics-no_info.ll
+++ llvm/test/Analysis/CostModel/free-intrinsics-no_info.ll
@@ -15,8 +15,8 @@
 ; CHECK-SIZE-NEXT:  Cost Model: Found an

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-19 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3696-3697
+
+if (E->isValueDependent() || E->isTypeDependent())
+  continue;
+

Should this move up above the point where we're checking whether the expression 
has an array type?



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3706
+
+if (!Result || !Notes.empty()) {
+  Diag(E->getBeginLoc(), diag::err_attribute_argument_n_type)

I'm surprised that the presence of notes alone would mean the attribute 
argument has an error and should be skipped. Are there circumstances under 
which `Result` is true but `Notes` is non-empty?



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3693
+if (E->isValueDependent() || E->isTypeDependent() ||
+(CE && CE->hasAPValueResult()))
+  continue;

Tyker wrote:
> aaron.ballman wrote:
> > What is the rationale for bailing out when the constant expression has an 
> > `APValue` result?
> the intent was that during nested template instantiation arguement will be 
> evaluated as soon as they are neither value nor type dependent and the result 
> will be stored in the ConstantExpr. if an arguement has already been 
> evaluated in a previous instantiation we don't want to evaluate it again.
> but because of SubstExpr ConstantExpr are getting removed so it removed it.
Ah, thank you for the explanation and change.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:195
+  return;
+else
+  Args.push_back(Result.get());

No `else` after a `return`.



Comment at: clang/test/SemaCXX/attr-annotate.cpp:96
+}
+
+void test() {}

Some more tests for various constant expression situations that may be weird:
```
int n = 10;
int vla[n];

[[clang::annotate("vlas are awful", sizeof(vla[++n]))] int i = 0; // reject, 
the sizeof is not unevaluated

[[clang::annotate("_Generic selection expression should be fine", _Generic(n, 
int : 0, default : 1))]] int j = 0; // second arg should resolve to 0 fine

void designator();
[[clang::annotate("function designators?", designator)]] int k = 0; // Should 
work?
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-12 Thread Ronan Keryell via Phabricator via cfe-commits 
keryell added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3704-3705
+  Result = E->EvaluateAsRValue(Eval, Context, true);
+else
+  Result = E->EvaluateAsLValue(Eval, Context, true);
+

Tyker wrote:
> keryell wrote:
> > aaron.ballman wrote:
> > > Tyker wrote:
> > > > aaron.ballman wrote:
> > > > > Under what circumstances would we want the constant expressions to be 
> > > > > lvalues? I would have guessed you would want to call 
> > > > > `Expr::EvaluateAsConstantExpr()` instead of either of these.
> > > > Expr::EvaluateAsConstantExpr will evaluate expression in there value 
> > > > category.
> > > > this can be quite surprising in some situations like:
> > > > ```
> > > > const int g_i = 0;
> > > > [[clang::annotate("test", g_i)]] void t() {} // annotation carries a 
> > > > pointer/reference on g_i
> > > > [[clang::annotate("test", (int)g_i)]] void t1() {} // annotation 
> > > > carries the value 0
> > > > [[clang::annotate("test", g_i + 0)]] void t1() {} // annotation carries 
> > > > the value 0
> > > > 
> > > > ```
> > > > with EvaluateAsRValue in all of the cases above the annotation will 
> > > > carry the value 0.
> > > > 
> > > > optionally we could wrap expression with an LValue to RValue cast and 
> > > > call EvaluateAsConstantExpr this should also work.
> > > Thank you for the explanation. I think wrapping with an lvalue to rvalue 
> > > conversion may make more sense -- `EvaluateAsRValue()` tries really hard 
> > > to get an rvalue out of something even if the standard says that's not 
> > > okay. It'll automatically apply the lvalue to rvalue conversion if 
> > > appropriate, but it'll also do more than that (such as evaluating side 
> > > effects).
> > This needs some motivating comments and explanations in the code.
> > Also the variations on `g_i` annotation above are quite interesting and 
> > should appear in the unit tests. I am unsure they are now.
> > Perhaps more interesting with other values than `0` in the example. :-)
> > I guess we can still use `[[clang::annotate("test", _i)]] void t() {}` to 
> > have a pointer on `g_i` and `(int&)g_i` for reference? To add to the unit 
> > tests if not already checked.
> > 
> i changed to use EvaluateAsConstantExpr + LValueToRvalue cast.
> 
> > This needs some motivating comments and explanations in the code.
> > Also the variations on `g_i` annotation above are quite interesting and 
> > should appear in the unit tests. I am unsure they are now.
> they are tested not as expressively as above but they are tested.
> 
> > Perhaps more interesting with other values than `0` in the example. :-)
> > I guess we can still use `[[clang::annotate("test", _i)]] void t() {}` to 
> > have a pointer on `g_i`
> yes this is how it is working.
> > `(int&)g_i` for reference? To add to the unit tests if not already checked.
> (int&)g_i has the salve value category and same type as g_i it just has a 
> noop cast around it.
> pointer an reference are indistinguishable in an APValue the only difference 
> is the c++ type of what they represent.
> and they are of course lowered to the same thing. so we only need the pointer 
> case.
> 
I see. Thanks for the clarification.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-10 Thread Tyker via Phabricator via cfe-commits 
Tyker added inline comments.



Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2229-2233
+  SmallVector Args = {
+  AnnotatedVal,
+  Builder.CreateBitCast(CGM.EmitAnnotationString(AnnotationStr), 
Int8PtrTy),
+  Builder.CreateBitCast(CGM.EmitAnnotationUnit(Location), Int8PtrTy),
+  CGM.EmitAnnotationLineNo(Location),

keryell wrote:
> Curious reindenting with mix-and-match of spaces & tabulations?
these are not tabs i believe this is just how phabricator shows indentation 
changes.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3693
+if (E->isValueDependent() || E->isTypeDependent() ||
+(CE && CE->hasAPValueResult()))
+  continue;

aaron.ballman wrote:
> What is the rationale for bailing out when the constant expression has an 
> `APValue` result?
the intent was that during nested template instantiation arguement will be 
evaluated as soon as they are neither value nor type dependent and the result 
will be stored in the ConstantExpr. if an arguement has already been evaluated 
in a previous instantiation we don't want to evaluate it again.
but because of SubstExpr ConstantExpr are getting removed so it removed it.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3704-3705
+  Result = E->EvaluateAsRValue(Eval, Context, true);
+else
+  Result = E->EvaluateAsLValue(Eval, Context, true);
+

keryell wrote:
> aaron.ballman wrote:
> > Tyker wrote:
> > > aaron.ballman wrote:
> > > > Under what circumstances would we want the constant expressions to be 
> > > > lvalues? I would have guessed you would want to call 
> > > > `Expr::EvaluateAsConstantExpr()` instead of either of these.
> > > Expr::EvaluateAsConstantExpr will evaluate expression in there value 
> > > category.
> > > this can be quite surprising in some situations like:
> > > ```
> > > const int g_i = 0;
> > > [[clang::annotate("test", g_i)]] void t() {} // annotation carries a 
> > > pointer/reference on g_i
> > > [[clang::annotate("test", (int)g_i)]] void t1() {} // annotation carries 
> > > the value 0
> > > [[clang::annotate("test", g_i + 0)]] void t1() {} // annotation carries 
> > > the value 0
> > > 
> > > ```
> > > with EvaluateAsRValue in all of the cases above the annotation will carry 
> > > the value 0.
> > > 
> > > optionally we could wrap expression with an LValue to RValue cast and 
> > > call EvaluateAsConstantExpr this should also work.
> > Thank you for the explanation. I think wrapping with an lvalue to rvalue 
> > conversion may make more sense -- `EvaluateAsRValue()` tries really hard to 
> > get an rvalue out of something even if the standard says that's not okay. 
> > It'll automatically apply the lvalue to rvalue conversion if appropriate, 
> > but it'll also do more than that (such as evaluating side effects).
> This needs some motivating comments and explanations in the code.
> Also the variations on `g_i` annotation above are quite interesting and 
> should appear in the unit tests. I am unsure they are now.
> Perhaps more interesting with other values than `0` in the example. :-)
> I guess we can still use `[[clang::annotate("test", _i)]] void t() {}` to 
> have a pointer on `g_i` and `(int&)g_i` for reference? To add to the unit 
> tests if not already checked.
> 
i changed to use EvaluateAsConstantExpr + LValueToRvalue cast.

> This needs some motivating comments and explanations in the code.
> Also the variations on `g_i` annotation above are quite interesting and 
> should appear in the unit tests. I am unsure they are now.
they are tested not as expressively as above but they are tested.

> Perhaps more interesting with other values than `0` in the example. :-)
> I guess we can still use `[[clang::annotate("test", _i)]] void t() {}` to 
> have a pointer on `g_i`
yes this is how it is working.
> `(int&)g_i` for reference? To add to the unit tests if not already checked.
(int&)g_i has the salve value category and same type as g_i it just has a noop 
cast around it.
pointer an reference are indistinguishable in an APValue the only difference is 
the c++ type of what they represent.
and they are of course lowered to the same thing. so we only need the pointer 
case.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-10 Thread Ronan Keryell via Phabricator via cfe-commits 
keryell added a comment.

@erichkeane @alexandre.isoard @Naghasan: any feedback in the context of SYCL & 
HLS C++ about this feature extension?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-10 Thread Tyker via Phabricator via cfe-commits 
Tyker updated this revision to Diff 297409.
Tyker marked 11 inline comments as done.
Tyker added a comment.

addressed comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/CodeGen/annotations-field.c
  clang/test/CodeGen/annotations-global.c
  clang/test/CodeGen/annotations-loc.c
  clang/test/CodeGen/annotations-var.c
  clang/test/CodeGenCXX/attr-annotate.cpp
  clang/test/Misc/pragma-attribute-cxx.cpp
  clang/test/Misc/pragma-attribute-objc.m
  clang/test/Parser/access-spec-attrs.cpp
  clang/test/Parser/objc-implementation-attrs.m
  clang/test/Sema/annotate.c
  clang/test/Sema/pragma-attribute.c
  clang/test/SemaCXX/attr-annotate.cpp
  llvm/include/llvm/IR/Intrinsics.td
  llvm/test/Analysis/CostModel/X86/free-intrinsics.ll
  llvm/test/Analysis/CostModel/free-intrinsics-datalayout.ll
  llvm/test/Analysis/CostModel/free-intrinsics-no_info.ll
  llvm/test/CodeGen/Generic/ptr-annotate.ll
  llvm/test/Transforms/InstCombine/assume_inevitable.ll

Index: llvm/test/Transforms/InstCombine/assume_inevitable.ll
===
--- llvm/test/Transforms/InstCombine/assume_inevitable.ll
+++ llvm/test/Transforms/InstCombine/assume_inevitable.ll
@@ -15,7 +15,7 @@
 ; CHECK-NEXT:[[DUMMY_EQ:%.*]] = icmp ugt i32 [[LOADRES]], 42
 ; CHECK-NEXT:tail call void @llvm.assume(i1 [[DUMMY_EQ]])
 ; CHECK-NEXT:[[M_I8:%.*]] = bitcast i64* [[M]] to i8*
-; CHECK-NEXT:[[M_A:%.*]] = call i8* @llvm.ptr.annotation.p0i8(i8* nonnull [[M_I8]], i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i64 0, i64 0), i32 2)
+; CHECK-NEXT:[[M_A:%.*]] = call i8* @llvm.ptr.annotation.p0i8(i8* nonnull [[M_I8]], i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i64 0, i64 0), i32 2, i8* null)
 ; CHECK-NEXT:[[M_X:%.*]] = bitcast i8* [[M_A]] to i64*
 ; CHECK-NEXT:[[OBJSZ:%.*]] = call i64 @llvm.objectsize.i64.p0i8(i8* [[C:%.*]], i1 false, i1 false, i1 false)
 ; CHECK-NEXT:store i64 [[OBJSZ]], i64* [[M_X]], align 4
@@ -44,7 +44,7 @@
   call void @llvm.lifetime.end.p0i8(i64 1, i8* %dummy)
 
   %m_i8 = bitcast i64* %m to i8*
-  %m_a = call i8* @llvm.ptr.annotation.p0i8(i8* %m_i8, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2)
+  %m_a = call i8* @llvm.ptr.annotation.p0i8(i8* %m_i8, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2, i8* null)
   %m_x = bitcast i8* %m_a to i64*
   %objsz = call i64 @llvm.objectsize.i64.p0i8(i8* %c, i1 false)
   store i64 %objsz, i64* %m_x
@@ -64,7 +64,7 @@
 
 declare i64 @llvm.objectsize.i64.p0i8(i8*, i1)
 declare i32 @llvm.annotation.i32(i32, i8*, i8*, i32)
-declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32)
+declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32, i8*)
 
 declare void @llvm.lifetime.start.p0i8(i64, i8* nocapture)
 declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture)
Index: llvm/test/CodeGen/Generic/ptr-annotate.ll
===
--- llvm/test/CodeGen/Generic/ptr-annotate.ll
+++ llvm/test/CodeGen/Generic/ptr-annotate.ll
@@ -10,9 +10,9 @@
 define void @foo() {
 entry:
   %m = alloca i8, align 4
-  %0 = call i8* @llvm.ptr.annotation.p0i8(i8* %m, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2)
+  %0 = call i8* @llvm.ptr.annotation.p0i8(i8* %m, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2, i8* null)
   store i8 1, i8* %0, align 4
   ret void
 }
 
-declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32) #1
+declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32, i8*) #1
Index: llvm/test/Analysis/CostModel/free-intrinsics-no_info.ll
===
--- llvm/test/Analysis/CostModel/free-intrinsics-no_info.ll
+++ llvm/test/Analysis/CostModel/free-intrinsics-no_info.ll
@@ -15,8 +15,8 @@
 ; CHECK-SIZE-NEXT:  Cost Model: Found an estimated cost of 0 for instruction: call void @llvm.lifetime.start.p0i8(i64 1, i8*

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-09 Thread Ronan Keryell via Phabricator via cfe-commits 
keryell added a comment.

Great feature for all the weird pieces of hardware all around the world! :-)




Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2229-2233
+  SmallVector Args = {
+  AnnotatedVal,
+  Builder.CreateBitCast(CGM.EmitAnnotationString(AnnotationStr), 
Int8PtrTy),
+  Builder.CreateBitCast(CGM.EmitAnnotationUnit(Location), Int8PtrTy),
+  CGM.EmitAnnotationLineNo(Location),

Curious reindenting with mix-and-match of spaces & tabulations?



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2386-2390
+  llvm::ConstantExpr::getBitCast(ASZeroGV, Int8PtrTy),
+  llvm::ConstantExpr::getBitCast(AnnoGV, Int8PtrTy),
+  llvm::ConstantExpr::getBitCast(UnitGV, Int8PtrTy),
+  LineNoCst,
+  Args,

Same curious reindenting.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3688
+Expr * = Attr->args_begin()[Idx];
+ConstantExpr *CE = dyn_cast(E);
+if (!E) {

aaron.ballman wrote:
> Tyker wrote:
> > aaron.ballman wrote:
> > > `auto *` since the type is spelled out in the initialization.
> > > 
> > > Also, I think this is unsafe -- it looks like during template 
> > > instantiation, any arguments that have a substitution failure will come 
> > > through as `nullptr`, and this will crash.
> > > 
> > > What should happen on template instantiation failure for these arguments? 
> > > I think the whole attribute should probably be dropped with appropriate 
> > > diagnostics rather than emitting a partial attribute, but perhaps you 
> > > have different ideas.
> > When template instantation fails nullptr is put in the Expr arguments and  
> > AddAnnotationAttr will emit a diagnostic and not associate the attribut to 
> > the declaration.
> Great, thank you for the fix!
Perhaps a few comments on this clarification would be useful in the code for 
the plain mortals...



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3704-3705
+  Result = E->EvaluateAsRValue(Eval, Context, true);
+else
+  Result = E->EvaluateAsLValue(Eval, Context, true);
+

aaron.ballman wrote:
> Tyker wrote:
> > aaron.ballman wrote:
> > > Under what circumstances would we want the constant expressions to be 
> > > lvalues? I would have guessed you would want to call 
> > > `Expr::EvaluateAsConstantExpr()` instead of either of these.
> > Expr::EvaluateAsConstantExpr will evaluate expression in there value 
> > category.
> > this can be quite surprising in some situations like:
> > ```
> > const int g_i = 0;
> > [[clang::annotate("test", g_i)]] void t() {} // annotation carries a 
> > pointer/reference on g_i
> > [[clang::annotate("test", (int)g_i)]] void t1() {} // annotation carries 
> > the value 0
> > [[clang::annotate("test", g_i + 0)]] void t1() {} // annotation carries the 
> > value 0
> > 
> > ```
> > with EvaluateAsRValue in all of the cases above the annotation will carry 
> > the value 0.
> > 
> > optionally we could wrap expression with an LValue to RValue cast and call 
> > EvaluateAsConstantExpr this should also work.
> Thank you for the explanation. I think wrapping with an lvalue to rvalue 
> conversion may make more sense -- `EvaluateAsRValue()` tries really hard to 
> get an rvalue out of something even if the standard says that's not okay. 
> It'll automatically apply the lvalue to rvalue conversion if appropriate, but 
> it'll also do more than that (such as evaluating side effects).
This needs some motivating comments and explanations in the code.
Also the variations on `g_i` annotation above are quite interesting and should 
appear in the unit tests. I am unsure they are now.
Perhaps more interesting with other values than `0` in the example. :-)
I guess we can still use `[[clang::annotate("test", _i)]] void t() {}` to 
have a pointer on `g_i` and `(int&)g_i` for reference? To add to the unit tests 
if not already checked.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-09 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

In D88645#2321980 , @jdoerfert wrote:

> (Drive By: This is cool)

I didn't say this before, but yeah, I agree -- this is really quite neat, thank 
you for working on it!




Comment at: clang/include/clang/Sema/ParsedAttr.h:1077
+/// AttributeCommonInfo has a non-explicit constructor which takes an
+/// SourceRange as only argument, this constructor has many uses so making it
+/// explicit is hard. This constructor causes ambiguity with

as only argument -> as its only argument



Comment at: clang/include/clang/Sema/ParsedAttr.h:1081
+/// So we use SFINAE to disable any conversion and remove any
+/// ambiguity.
+template isValueDependent() || E->isTypeDependent() ||
+(CE && CE->hasAPValueResult()))
+  continue;

What is the rationale for bailing out when the constant expression has an 
`APValue` result?



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3688
+Expr * = Attr->args_begin()[Idx];
+ConstantExpr *CE = dyn_cast(E);
+if (!E) {

Tyker wrote:
> aaron.ballman wrote:
> > `auto *` since the type is spelled out in the initialization.
> > 
> > Also, I think this is unsafe -- it looks like during template 
> > instantiation, any arguments that have a substitution failure will come 
> > through as `nullptr`, and this will crash.
> > 
> > What should happen on template instantiation failure for these arguments? I 
> > think the whole attribute should probably be dropped with appropriate 
> > diagnostics rather than emitting a partial attribute, but perhaps you have 
> > different ideas.
> When template instantation fails nullptr is put in the Expr arguments and  
> AddAnnotationAttr will emit a diagnostic and not associate the attribut to 
> the declaration.
Great, thank you for the fix!



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3704-3705
+  Result = E->EvaluateAsRValue(Eval, Context, true);
+else
+  Result = E->EvaluateAsLValue(Eval, Context, true);
+

Tyker wrote:
> aaron.ballman wrote:
> > Under what circumstances would we want the constant expressions to be 
> > lvalues? I would have guessed you would want to call 
> > `Expr::EvaluateAsConstantExpr()` instead of either of these.
> Expr::EvaluateAsConstantExpr will evaluate expression in there value category.
> this can be quite surprising in some situations like:
> ```
> const int g_i = 0;
> [[clang::annotate("test", g_i)]] void t() {} // annotation carries a 
> pointer/reference on g_i
> [[clang::annotate("test", (int)g_i)]] void t1() {} // annotation carries the 
> value 0
> [[clang::annotate("test", g_i + 0)]] void t1() {} // annotation carries the 
> value 0
> 
> ```
> with EvaluateAsRValue in all of the cases above the annotation will carry the 
> value 0.
> 
> optionally we could wrap expression with an LValue to RValue cast and call 
> EvaluateAsConstantExpr this should also work.
Thank you for the explanation. I think wrapping with an lvalue to rvalue 
conversion may make more sense -- `EvaluateAsRValue()` tries really hard to get 
an rvalue out of something even if the standard says that's not okay. It'll 
automatically apply the lvalue to rvalue conversion if appropriate, but it'll 
also do more than that (such as evaluating side effects).



Comment at: clang/test/SemaCXX/attr-annotate.cpp:1
+// RUN: %clang_cc1 -std=c++11 -emit-llvm -verify %s
+

Tyker wrote:
> aaron.ballman wrote:
> > Do you mean to emit llvm here? I think that should probably be 
> > `-fsyntax-only`
> The reason i put an -emit-llvm is to also test the asserts in IRgen on more 
> complex code.
Tests in sema shouldn't typically run codegen, so I'd rather this was 
`-fsyntax-only` and test the assertions more explicitly in a codegen-specific 
test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-09 Thread Johannes Doerfert via Phabricator via cfe-commits 
jdoerfert added a comment.

(Drive By: This is cool)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-09 Thread Tyker via Phabricator via cfe-commits 
Tyker added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3683
+  auto *Attr =
+  AnnotateAttr::Create(Context, Str, Args.empty() ? nullptr : Args.data(),
+   Args.size(), CI.getRange(), CI.getSyntax());

aaron.ballman wrote:
> Just curious, but is the `?:` actually necessary? I would have assumed that 
> an empty `ArrayRef` would return `nullptr` from `data()`.
maybe it was(in a previous version of the code) but not anymore since there is 
always at least one argument.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3688
+Expr * = Attr->args_begin()[Idx];
+ConstantExpr *CE = dyn_cast(E);
+if (!E) {

aaron.ballman wrote:
> `auto *` since the type is spelled out in the initialization.
> 
> Also, I think this is unsafe -- it looks like during template instantiation, 
> any arguments that have a substitution failure will come through as 
> `nullptr`, and this will crash.
> 
> What should happen on template instantiation failure for these arguments? I 
> think the whole attribute should probably be dropped with appropriate 
> diagnostics rather than emitting a partial attribute, but perhaps you have 
> different ideas.
When template instantation fails nullptr is put in the Expr arguments and  
AddAnnotationAttr will emit a diagnostic and not associate the attribut to the 
declaration.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3704-3705
+  Result = E->EvaluateAsRValue(Eval, Context, true);
+else
+  Result = E->EvaluateAsLValue(Eval, Context, true);
+

aaron.ballman wrote:
> Under what circumstances would we want the constant expressions to be 
> lvalues? I would have guessed you would want to call 
> `Expr::EvaluateAsConstantExpr()` instead of either of these.
Expr::EvaluateAsConstantExpr will evaluate expression in there value category.
this can be quite surprising in some situations like:
```
const int g_i = 0;
[[clang::annotate("test", g_i)]] void t() {} // annotation carries a 
pointer/reference on g_i
[[clang::annotate("test", (int)g_i)]] void t1() {} // annotation carries the 
value 0
[[clang::annotate("test", g_i + 0)]] void t1() {} // annotation carries the 
value 0

```
with EvaluateAsRValue in all of the cases above the annotation will carry the 
value 0.

optionally we could wrap expression with an LValue to RValue cast and call 
EvaluateAsConstantExpr this should also work.



Comment at: clang/test/SemaCXX/attr-annotate.cpp:1
+// RUN: %clang_cc1 -std=c++11 -emit-llvm -verify %s
+

aaron.ballman wrote:
> Do you mean to emit llvm here? I think that should probably be `-fsyntax-only`
The reason i put an -emit-llvm is to also test the asserts in IRgen on more 
complex code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-09 Thread Tyker via Phabricator via cfe-commits 
Tyker updated this revision to Diff 297159.
Tyker marked 7 inline comments as done.
Tyker added a comment.

addressed comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/CodeGen/annotations-field.c
  clang/test/CodeGen/annotations-global.c
  clang/test/CodeGen/annotations-loc.c
  clang/test/CodeGen/annotations-var.c
  clang/test/CodeGenCXX/attr-annotate.cpp
  clang/test/Misc/pragma-attribute-cxx.cpp
  clang/test/Misc/pragma-attribute-objc.m
  clang/test/Parser/access-spec-attrs.cpp
  clang/test/Parser/objc-implementation-attrs.m
  clang/test/Sema/annotate.c
  clang/test/Sema/pragma-attribute.c
  clang/test/SemaCXX/attr-annotate.cpp
  llvm/include/llvm/IR/Intrinsics.td
  llvm/test/Analysis/CostModel/X86/free-intrinsics.ll
  llvm/test/Analysis/CostModel/free-intrinsics-datalayout.ll
  llvm/test/Analysis/CostModel/free-intrinsics-no_info.ll
  llvm/test/CodeGen/Generic/ptr-annotate.ll
  llvm/test/Transforms/InstCombine/assume_inevitable.ll

Index: llvm/test/Transforms/InstCombine/assume_inevitable.ll
===
--- llvm/test/Transforms/InstCombine/assume_inevitable.ll
+++ llvm/test/Transforms/InstCombine/assume_inevitable.ll
@@ -15,7 +15,7 @@
 ; CHECK-NEXT:[[DUMMY_EQ:%.*]] = icmp ugt i32 [[LOADRES]], 42
 ; CHECK-NEXT:tail call void @llvm.assume(i1 [[DUMMY_EQ]])
 ; CHECK-NEXT:[[M_I8:%.*]] = bitcast i64* [[M]] to i8*
-; CHECK-NEXT:[[M_A:%.*]] = call i8* @llvm.ptr.annotation.p0i8(i8* nonnull [[M_I8]], i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i64 0, i64 0), i32 2)
+; CHECK-NEXT:[[M_A:%.*]] = call i8* @llvm.ptr.annotation.p0i8(i8* nonnull [[M_I8]], i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i64 0, i64 0), i32 2, i8* null)
 ; CHECK-NEXT:[[M_X:%.*]] = bitcast i8* [[M_A]] to i64*
 ; CHECK-NEXT:[[OBJSZ:%.*]] = call i64 @llvm.objectsize.i64.p0i8(i8* [[C:%.*]], i1 false, i1 false, i1 false)
 ; CHECK-NEXT:store i64 [[OBJSZ]], i64* [[M_X]], align 4
@@ -44,7 +44,7 @@
   call void @llvm.lifetime.end.p0i8(i64 1, i8* %dummy)
 
   %m_i8 = bitcast i64* %m to i8*
-  %m_a = call i8* @llvm.ptr.annotation.p0i8(i8* %m_i8, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2)
+  %m_a = call i8* @llvm.ptr.annotation.p0i8(i8* %m_i8, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2, i8* null)
   %m_x = bitcast i8* %m_a to i64*
   %objsz = call i64 @llvm.objectsize.i64.p0i8(i8* %c, i1 false)
   store i64 %objsz, i64* %m_x
@@ -64,7 +64,7 @@
 
 declare i64 @llvm.objectsize.i64.p0i8(i8*, i1)
 declare i32 @llvm.annotation.i32(i32, i8*, i8*, i32)
-declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32)
+declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32, i8*)
 
 declare void @llvm.lifetime.start.p0i8(i64, i8* nocapture)
 declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture)
Index: llvm/test/CodeGen/Generic/ptr-annotate.ll
===
--- llvm/test/CodeGen/Generic/ptr-annotate.ll
+++ llvm/test/CodeGen/Generic/ptr-annotate.ll
@@ -10,9 +10,9 @@
 define void @foo() {
 entry:
   %m = alloca i8, align 4
-  %0 = call i8* @llvm.ptr.annotation.p0i8(i8* %m, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2)
+  %0 = call i8* @llvm.ptr.annotation.p0i8(i8* %m, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2, i8* null)
   store i8 1, i8* %0, align 4
   ret void
 }
 
-declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32) #1
+declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32, i8*) #1
Index: llvm/test/Analysis/CostModel/free-intrinsics-no_info.ll
===
--- llvm/test/Analysis/CostModel/free-intrinsics-no_info.ll
+++ llvm/test/Analysis/CostModel/free-intrinsics-no_info.ll
@@ -15,8 +15,8 @@
 ; CHECK-SIZE-NEXT:  Cost Model: Found an estimated cost of 0 for instruction: call void @llvm.lifetime.start.p0i8(i64 1, i8* undef)

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-08 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2338
+  Exprs = Exprs.drop_front();
+  if (Exprs.size() == 0)
+return llvm::ConstantPointerNull::get(Int8PtrTy);

`Exprs.empty()` would be more concise.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2354-2356
+auto *Constant = ConstEmiter.emitAbstract(
+CE->getBeginLoc(), CE->getAPValueResult(), CE->getType());
+return Constant;

You can return the expression directly, which removes a somewhat questionable 
use of `auto`.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3683
+  auto *Attr =
+  AnnotateAttr::Create(Context, Str, Args.empty() ? nullptr : Args.data(),
+   Args.size(), CI.getRange(), CI.getSyntax());

Just curious, but is the `?:` actually necessary? I would have assumed that an 
empty `ArrayRef` would return `nullptr` from `data()`.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3688
+Expr * = Attr->args_begin()[Idx];
+ConstantExpr *CE = dyn_cast(E);
+if (!E) {

`auto *` since the type is spelled out in the initialization.

Also, I think this is unsafe -- it looks like during template instantiation, 
any arguments that have a substitution failure will come through as `nullptr`, 
and this will crash.

What should happen on template instantiation failure for these arguments? I 
think the whole attribute should probably be dropped with appropriate 
diagnostics rather than emitting a partial attribute, but perhaps you have 
different ideas.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3691
+  Diag(CI.getLoc(), diag::err_attribute_argument_n_type)
+  << "'annotate'" << Idx << AANT_ArgumentConstantExpr;
+  return;

This is unfortunate -- you should be passing in the `ParsedAttr` object so that 
the diagnostic engine can take care of properly formatting the diagnostic. For 
instance, this will incorrectly name the attribute `'annotate'` instead of 
`clang::annotate'` if that's how the user spelled the attribute. I don't recall 
whether it will work on an `AttributeCommonInfo` though.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3694
+}
+if (E->getDependence() || (CE && CE->hasAPValueResult()))
+  continue;

`E->getDependence()` smells a bit fishy because this will early bail on any 
kind of dependency, including an error dependency. Do you mean to care only 
about type and/or value dependence here?



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3704-3705
+  Result = E->EvaluateAsRValue(Eval, Context, true);
+else
+  Result = E->EvaluateAsLValue(Eval, Context, true);
+

Under what circumstances would we want the constant expressions to be lvalues? 
I would have guessed you would want to call `Expr::EvaluateAsConstantExpr()` 
instead of either of these.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3730
+  for (unsigned Idx = 0; Idx < AL.getNumArgs(); Idx++) {
+if (AL.isArgIdent(Idx))
+  Args.push_back(nullptr);

Hmm, I think you should assert that you never get an identifier at this point 
-- the parser should be treating all of these as expressions and not simple 
identifiers.



Comment at: clang/test/SemaCXX/attr-annotate.cpp:1
+// RUN: %clang_cc1 -std=c++11 -emit-llvm -verify %s
+

Do you mean to emit llvm here? I think that should probably be `-fsyntax-only`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88645/new/

https://reviews.llvm.org/D88645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-01 Thread Tyker via Phabricator via cfe-commits 
Tyker created this revision.
Tyker added a reviewer: aaron.ballman.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.
Tyker requested review of this revision.
Herald added a subscriber: jdoerfert.

This allows using annotation in a much more contexts than it currently has.
especially when annotation with template or constexpr.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88645

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/CodeGen/annotations-field.c
  clang/test/CodeGen/annotations-global.c
  clang/test/CodeGen/annotations-loc.c
  clang/test/CodeGen/annotations-var.c
  clang/test/CodeGenCXX/attr-annotate.cpp
  clang/test/Misc/pragma-attribute-cxx.cpp
  clang/test/Misc/pragma-attribute-objc.m
  clang/test/Parser/access-spec-attrs.cpp
  clang/test/Parser/objc-implementation-attrs.m
  clang/test/Sema/annotate.c
  clang/test/Sema/pragma-attribute.c
  clang/test/SemaCXX/attr-annotate.cpp
  llvm/include/llvm/IR/Intrinsics.td
  llvm/test/Analysis/CostModel/X86/free-intrinsics.ll
  llvm/test/Analysis/CostModel/free-intrinsics-datalayout.ll
  llvm/test/Analysis/CostModel/free-intrinsics-no_info.ll
  llvm/test/CodeGen/Generic/ptr-annotate.ll
  llvm/test/Transforms/InstCombine/assume_inevitable.ll

Index: llvm/test/Transforms/InstCombine/assume_inevitable.ll
===
--- llvm/test/Transforms/InstCombine/assume_inevitable.ll
+++ llvm/test/Transforms/InstCombine/assume_inevitable.ll
@@ -15,7 +15,7 @@
 ; CHECK-NEXT:[[DUMMY_EQ:%.*]] = icmp ugt i32 [[LOADRES]], 42
 ; CHECK-NEXT:tail call void @llvm.assume(i1 [[DUMMY_EQ]])
 ; CHECK-NEXT:[[M_I8:%.*]] = bitcast i64* [[M]] to i8*
-; CHECK-NEXT:[[M_A:%.*]] = call i8* @llvm.ptr.annotation.p0i8(i8* nonnull [[M_I8]], i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i64 0, i64 0), i32 2)
+; CHECK-NEXT:[[M_A:%.*]] = call i8* @llvm.ptr.annotation.p0i8(i8* nonnull [[M_I8]], i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i64 0, i64 0), i32 2, i8* null)
 ; CHECK-NEXT:[[M_X:%.*]] = bitcast i8* [[M_A]] to i64*
 ; CHECK-NEXT:[[OBJSZ:%.*]] = call i64 @llvm.objectsize.i64.p0i8(i8* [[C:%.*]], i1 false, i1 false, i1 false)
 ; CHECK-NEXT:store i64 [[OBJSZ]], i64* [[M_X]], align 4
@@ -44,7 +44,7 @@
   call void @llvm.lifetime.end.p0i8(i64 1, i8* %dummy)
 
   %m_i8 = bitcast i64* %m to i8*
-  %m_a = call i8* @llvm.ptr.annotation.p0i8(i8* %m_i8, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2)
+  %m_a = call i8* @llvm.ptr.annotation.p0i8(i8* %m_i8, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2, i8* null)
   %m_x = bitcast i8* %m_a to i64*
   %objsz = call i64 @llvm.objectsize.i64.p0i8(i8* %c, i1 false)
   store i64 %objsz, i64* %m_x
@@ -64,7 +64,7 @@
 
 declare i64 @llvm.objectsize.i64.p0i8(i8*, i1)
 declare i32 @llvm.annotation.i32(i32, i8*, i8*, i32)
-declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32)
+declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32, i8*)
 
 declare void @llvm.lifetime.start.p0i8(i64, i8* nocapture)
 declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture)
Index: llvm/test/CodeGen/Generic/ptr-annotate.ll
===
--- llvm/test/CodeGen/Generic/ptr-annotate.ll
+++ llvm/test/CodeGen/Generic/ptr-annotate.ll
@@ -10,9 +10,9 @@
 define void @foo() {
 entry:
   %m = alloca i8, align 4
-  %0 = call i8* @llvm.ptr.annotation.p0i8(i8* %m, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2)
+  %0 = call i8* @llvm.ptr.annotation.p0i8(i8* %m, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2, i8* null)
   store i8 1, i8* %0, align 4
   ret void
 }
 
-declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32) #1
+declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32, i8*) #1
Index: llvm/test/Analysis/CostModel/free-intrinsics-no_info.ll
===
--- llvm/test/Analysis/CostModel/free-intrinsics-no_info.ll
+++