[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked an inline comment as done.
yaxunl added a comment.

In D70172#3077696 , @hokein wrote:

> This patch seems to cause a new crash, details are at 
> https://bugs.llvm.org/show_bug.cgi?id=52250.

I will take a look. Thanks.


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

https://reviews.llvm.org/D70172

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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

This patch seems to case a new crash, details are at 
https://bugs.llvm.org/show_bug.cgi?id=52250.


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

https://reviews.llvm.org/D70172

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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Wei Wang via Phabricator via cfe-commits]

weiwang added a comment.

In D70172#2964118 , @sugak wrote:

> Hi @yaxunl! I'm working on upgrading a large codebase from LLVM-9 to LLVM-12. 
> I noticed on average 10% compilation speed regression that seems to be caused 
> this change. We use Clang modules and historically provide `-fopenmp` 
> compiler flag by default. The problem seems to be that compiling and 
> importing modules is now slower, with the generated modules size increased by 
> 2X. llvm-bcanalyzer tool shows that it's dominated by 
> `DECLS_TO_CHECK_FOR_DEFERRED_DIAGS`.  If I understand it right, your change 
> is only relevant when target offloading is used. I inspected all of `#pragma 
> omp` directives and can confirm that we don't use it.
>
> I see that most of this code is gated by OpenMP flag. I wonder if there is a 
> finer grain way to enable openmp parallel code generation without target 
> offloading? Would it make sense to extend this code to check if 
> `-fopenom-targets` is set before recording 
> `DECLS_TO_CHECK_FOR_DEFERRED_DIAGS`?
>
> Note, this was measured with @weiwang's https://reviews.llvm.org/D101793.

We did an internal measurement by not adding decls into deferred diags, and 
that resolves the build regression. Wonder if we can have a special case for 
emitting diag as they are encountered when everything is on host side.


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

https://reviews.llvm.org/D70172

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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Igor Sugak via Phabricator via cfe-commits]

sugak added subscribers: weiwang, sugak.
sugak added a comment.
Herald added a subscriber: sstefan1.

Hi @yaxunl! I'm working on upgrading a large codebase from LLVM-9 to LLVM-12. I 
noticed on average 10% compilation speed regression that seems to be caused 
this change. We use Clang modules and historically provide `-fopenmp` compiler 
flag by default. The problem seem to be that compiling and importing modules is 
now slower, with the generated modules size increased by 2X. llvm-bcanalyzer 
tool shows that it's dominated by `DECLS_TO_CHECK_FOR_DEFERRED_DIAGS`.  If I 
understand it right, your change is only relevant when target offloading is 
used. I inspected all of `#pragma omp` directives and can confirm that we don't 
use it.

I see that most of this code is gated by OpenMP flag. I wonder if there is a 
finer grain way to enable openmp parallel code generation without target 
offloading? Would it make sense to extend this code to check if 
`-fopenom-targets` is set before recording `DECLS_TO_CHECK_FOR_DEFERRED_DIAGS`?

Note, this was measured @weiwang's https://reviews.llvm.org/D101793.


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

https://reviews.llvm.org/D70172

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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 2 inline comments as done.
yaxunl added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1540
+} else if (auto *VD = dyn_cast(D)) {
+  if (auto *Init = VD->getInit()) {
+auto DevTy = OMPDeclareTargetDeclAttr::getDeviceType(VD);

rjmccall wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > yaxunl wrote:
> > > > rjmccall wrote:
> > > > > Can there also be deferred diagnostics associated with this 
> > > > > initializer?
> > > > Yes. A global variable may be marked by omp declare target directive to 
> > > > be emitted on device. If the global var is initialized with the address 
> > > > of a function, the function will be emitted on device. If the device 
> > > > function calls a host device function which contains a deferred diag, 
> > > > that diag will be emitted. This can only be known after everything is 
> > > > parsed.
> > > I meant directly with the initializer.  Is there a way today to defer a 
> > > diagnostic that you would emit while processing an initializer 
> > > expression?  If so, this needs to trigger that.
> > I don't think the initializer itself (without a target declare directive) 
> > will cause a deferred diagnostic since it does not cause change of emission 
> > states of functions. 
> Okay, so if I'm getting this right: only functions are emitted lazily, and 
> variables have to be marked specially in order to get emitted on the device, 
> so there's no need to defer diagnostics within variable initializations 
> because we always know at the time of processing the variable where it will 
> be emitted?
right.


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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

This looks good, assuming there's either no issue with the lazy emission of 
variables or that you just intend to tackle that later.




Comment at: clang/lib/Sema/Sema.cpp:1540
+} else if (auto *VD = dyn_cast(D)) {
+  if (auto *Init = VD->getInit()) {
+auto DevTy = OMPDeclareTargetDeclAttr::getDeviceType(VD);

yaxunl wrote:
> rjmccall wrote:
> > yaxunl wrote:
> > > rjmccall wrote:
> > > > Can there also be deferred diagnostics associated with this initializer?
> > > Yes. A global variable may be marked by omp declare target directive to 
> > > be emitted on device. If the global var is initialized with the address 
> > > of a function, the function will be emitted on device. If the device 
> > > function calls a host device function which contains a deferred diag, 
> > > that diag will be emitted. This can only be known after everything is 
> > > parsed.
> > I meant directly with the initializer.  Is there a way today to defer a 
> > diagnostic that you would emit while processing an initializer expression?  
> > If so, this needs to trigger that.
> I don't think the initializer itself (without a target declare directive) 
> will cause a deferred diagnostic since it does not cause change of emission 
> states of functions. 
Okay, so if I'm getting this right: only functions are emitted lazily, and 
variables have to be marked specially in order to get emitted on the device, so 
there's no need to defer diagnostics within variable initializations because we 
always know at the time of processing the variable where it will be emitted?


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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 4 inline comments as done.
yaxunl added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1514
+  void visitUsedDecl(SourceLocation Loc, Decl *D) {
+if (auto *TD = dyn_cast(D)) {
+  for (auto *DD : TD->decls()) {

rjmccall wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > bader wrote:
> > > > yaxunl wrote:
> > > > > rjmccall wrote:
> > > > > > yaxunl wrote:
> > > > > > > rjmccall wrote:
> > > > > > > > erichkeane wrote:
> > > > > > > > > rjmccall wrote:
> > > > > > > > > > erichkeane wrote:
> > > > > > > > > > > Note that when recommitting this (if you choose to), this 
> > > > > > > > > > > needs to also handle NamespaceDecl.  We're a downstream 
> > > > > > > > > > > and discovered that this doesn't properly handle 
> > > > > > > > > > > functions or records handled in a namespace.
> > > > > > > > > > > 
> > > > > > > > > > > It can be implemented identically to TranslationUnitDecl.
> > > > > > > > > > Wait, what?  We shouldn't be doing this for 
> > > > > > > > > > TranslationUnitDecl either.   I don't even know how we're 
> > > > > > > > > > "using" a TranslationUnitDecl, but neither this case not 
> > > > > > > > > > the case for `NamespaceDecl` should be recursively using 
> > > > > > > > > > every declaration declared inside it.  If there's a 
> > > > > > > > > > declaration in a namespace that's being used, it should be 
> > > > > > > > > > getting visited as part of the actual use of it.
> > > > > > > > > > 
> > > > > > > > > > The logic for `RecordDecl` has the same problem.  
> > > > > > > > > Despite the name, this seems to be more of a home-written ast 
> > > > > > > > > walking class.  The entry point is the 'translation unit' 
> > > > > > > > > which seems to walk through everything in an attempt to find 
> > > > > > > > > all the functions (including those that are 'marked' as used 
> > > > > > > > > by an attribute).
> > > > > > > > > 
> > > > > > > > > You'll see the FunctionDecl section makes this assumption as 
> > > > > > > > > well (not necessarily that we got to a function via a call). 
> > > > > > > > > IMO, this approach is strange, and we should register entry 
> > > > > > > > > points in some manner (functions marked as emitted to the 
> > > > > > > > > device in some fashion), then just follow its call-graph (via 
> > > > > > > > > the clang::CallGraph?) to emit all of these functions.
> > > > > > > > > 
> > > > > > > > > It seemed really odd to see this approach here, but it seemed 
> > > > > > > > > well reviewed by the time I noticed it (via a downstream bug) 
> > > > > > > > > so I figured I'd lost my chance to disagree with the approach.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > Sure, but `visitUsedDecl` isn't the right place to be entering 
> > > > > > > > the walk.  `visitUsedDecl` is supposed to be the *callback* 
> > > > > > > > from the walk.  If they need to walk all the global 
> > > > > > > > declarations to find kernels instead of tracking the kernels as 
> > > > > > > > they're encountered (which would be a *much* better approach), 
> > > > > > > > it should be done as a separate function.
> > > > > > > > 
> > > > > > > > I just missed this in the review.
> > > > > > > The deferred diagnostics could be initiated by non-kernel 
> > > > > > > functions or even host functions.
> > > > > > > 
> > > > > > > Let's consider a device code library where no kernels are 
> > > > > > > defined. A device function is emitted, which calls a host device 
> > > > > > > function which has a deferred diagnostic. All device functions 
> > > > > > > that are emitted need to be checked.
> > > > > > > 
> > > > > > > Same with host functions that are emitted, which may call a host 
> > > > > > > device function which has deferred diagnostic.
> > > > > > > 
> > > > > > > Also not just function calls need to be checked. A function 
> > > > > > > address may be taken then called through function pointer. 
> > > > > > > Therefore any reference to a function needs to be followed.
> > > > > > > 
> > > > > > > In the case of OpenMP, the initialization of a global function 
> > > > > > > pointer which refers a function may trigger a deferred 
> > > > > > > diangostic. There are tests for that.
> > > > > > Right, I get that emitting deferred diagnostics for a declaration D 
> > > > > > needs to trigger any deferred diagnostics in declarations used by 
> > > > > > D, recursively.  You essentially have a graph of lazily-emitted 
> > > > > > declarations (which may or may not have deferred diagnostics) and a 
> > > > > > number of eagerly-emitted "root" declarations with use-edges 
> > > > > > leading into that graph.  Any declaration that's reachable from a 
> > > > > > root will need to be emitted and so needs to have any deferred 
> > > > > > diagnostics emitted as well.  My question is why you're finding 
> > > > > > these roots with a retroactive walk of the entire translation unit 
> > > > > > instead of either building a 

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 251167.
yaxunl marked 2 inline comments as done.
yaxunl added a comment.

revised by John's comments


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

https://reviews.llvm.org/D70172

Files:
  clang/include/clang/Sema/ExternalSemaSource.h
  clang/include/clang/Sema/MultiplexExternalSemaSource.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Sema/MultiplexExternalSemaSource.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCUDA.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/UsedDeclVisitor.h
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/OpenMP/declare_target_messages.cpp
  clang/test/OpenMP/nvptx_target_exceptions_messages.cpp
  clang/test/SemaCUDA/bad-calls-on-same-line.cu
  clang/test/SemaCUDA/call-device-fn-from-host.cu
  clang/test/SemaCUDA/call-host-fn-from-device.cu
  clang/test/SemaCUDA/openmp-target.cu
  clang/test/SemaCUDA/trace-through-global.cu

Index: clang/test/SemaCUDA/trace-through-global.cu
===
--- clang/test/SemaCUDA/trace-through-global.cu
+++ clang/test/SemaCUDA/trace-through-global.cu
@@ -38,7 +38,7 @@
   // Notice that these two diagnostics are different: Because the call to hd1
   // is not dependent on T, the call to hd1 comes from 'launch_kernel', while
   // the call to hd3, being dependent, comes from 'launch_kernel'.
-  hd1(); // expected-note {{called by 'launch_kernel'}}
+  hd1(); // expected-note {{called by 'launch_kernel'}}
   hd3(T()); // expected-note {{called by 'launch_kernel'}}
 }
 
Index: clang/test/SemaCUDA/openmp-target.cu
===
--- clang/test/SemaCUDA/openmp-target.cu
+++ clang/test/SemaCUDA/openmp-target.cu
@@ -16,9 +16,9 @@
 void bazzz() {bazz();}
 #pragma omp declare target to(bazzz) device_type(nohost)
 void any() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
-void host1() {bazz();}
+void host1() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
 #pragma omp declare target to(host1) device_type(host)
-void host2() {bazz();}
+void host2() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
 #pragma omp declare target to(host2)
 void device() {host1();}
 #pragma omp declare target to(device) device_type(nohost)
Index: clang/test/SemaCUDA/call-host-fn-from-device.cu
===
--- clang/test/SemaCUDA/call-host-fn-from-device.cu
+++ clang/test/SemaCUDA/call-host-fn-from-device.cu
@@ -56,14 +56,14 @@
 }
 
 template  __host__ __device__ void hd2() { host_fn(); }
-// expected-error@-1 2 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 __global__ void kernel() { hd2(); }
 
 __host__ __device__ void hd() { host_fn(); }
 // expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 
 template  __host__ __device__ void hd3() { host_fn(); }
-// expected-error@-1 2 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 __device__ void device_fn() { hd3(); }
 
 // No error because this is never instantiated.
Index: clang/test/SemaCUDA/call-device-fn-from-host.cu
===
--- clang/test/SemaCUDA/call-device-fn-from-host.cu
+++ clang/test/SemaCUDA/call-device-fn-from-host.cu
@@ -1,7 +1,7 @@
 // RUN: %clang_cc1 %s --std=c++11 -triple x86_64-unknown-linux -emit-llvm -o - \
 // RUN:   -verify -verify-ignore-unexpected=note
 // RUN: %clang_cc1 %s --std=c++11 -triple x86_64-unknown-linux -emit-llvm -o - \
-// RUN:   -verify -verify-ignore-unexpected=note -fopenmp
+// RUN:   -verify=expected,omp -verify-ignore-unexpected=note -fopenmp
 
 // Note: This test won't work with -fsyntax-only, because some of these errors
 // are emitted during codegen.
@@ -39,7 +39,7 @@
 }
 
 template  __host__ __device__ void hd2() { device_fn(); }
-// expected-error@-1 2 {{reference to __device__ function 'device_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __device__ function 'device_fn' in __host__ __device__ function}}
 void host_fn() { hd2(); }
 
 __host__ __device__ void hd() { device_fn(); }
Index: clang/test/SemaCUDA/bad-calls-on-same-line.cu
===
--- clang/test/SemaCUDA/bad-calls-on-same-line.cu
+++ clang/test/SemaCUDA/bad-calls-on-same-line.cu
@@ -33,8 +33,8 @@
 
 void host_fn() {
   hd();
-  

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1514
+  void visitUsedDecl(SourceLocation Loc, Decl *D) {
+if (auto *TD = dyn_cast(D)) {
+  for (auto *DD : TD->decls()) {

yaxunl wrote:
> rjmccall wrote:
> > bader wrote:
> > > yaxunl wrote:
> > > > rjmccall wrote:
> > > > > yaxunl wrote:
> > > > > > rjmccall wrote:
> > > > > > > erichkeane wrote:
> > > > > > > > rjmccall wrote:
> > > > > > > > > erichkeane wrote:
> > > > > > > > > > Note that when recommitting this (if you choose to), this 
> > > > > > > > > > needs to also handle NamespaceDecl.  We're a downstream and 
> > > > > > > > > > discovered that this doesn't properly handle functions or 
> > > > > > > > > > records handled in a namespace.
> > > > > > > > > > 
> > > > > > > > > > It can be implemented identically to TranslationUnitDecl.
> > > > > > > > > Wait, what?  We shouldn't be doing this for 
> > > > > > > > > TranslationUnitDecl either.   I don't even know how we're 
> > > > > > > > > "using" a TranslationUnitDecl, but neither this case not the 
> > > > > > > > > case for `NamespaceDecl` should be recursively using every 
> > > > > > > > > declaration declared inside it.  If there's a declaration in 
> > > > > > > > > a namespace that's being used, it should be getting visited 
> > > > > > > > > as part of the actual use of it.
> > > > > > > > > 
> > > > > > > > > The logic for `RecordDecl` has the same problem.  
> > > > > > > > Despite the name, this seems to be more of a home-written ast 
> > > > > > > > walking class.  The entry point is the 'translation unit' which 
> > > > > > > > seems to walk through everything in an attempt to find all the 
> > > > > > > > functions (including those that are 'marked' as used by an 
> > > > > > > > attribute).
> > > > > > > > 
> > > > > > > > You'll see the FunctionDecl section makes this assumption as 
> > > > > > > > well (not necessarily that we got to a function via a call). 
> > > > > > > > IMO, this approach is strange, and we should register entry 
> > > > > > > > points in some manner (functions marked as emitted to the 
> > > > > > > > device in some fashion), then just follow its call-graph (via 
> > > > > > > > the clang::CallGraph?) to emit all of these functions.
> > > > > > > > 
> > > > > > > > It seemed really odd to see this approach here, but it seemed 
> > > > > > > > well reviewed by the time I noticed it (via a downstream bug) 
> > > > > > > > so I figured I'd lost my chance to disagree with the approach.
> > > > > > > > 
> > > > > > > > 
> > > > > > > Sure, but `visitUsedDecl` isn't the right place to be entering 
> > > > > > > the walk.  `visitUsedDecl` is supposed to be the *callback* from 
> > > > > > > the walk.  If they need to walk all the global declarations to 
> > > > > > > find kernels instead of tracking the kernels as they're 
> > > > > > > encountered (which would be a *much* better approach), it should 
> > > > > > > be done as a separate function.
> > > > > > > 
> > > > > > > I just missed this in the review.
> > > > > > The deferred diagnostics could be initiated by non-kernel functions 
> > > > > > or even host functions.
> > > > > > 
> > > > > > Let's consider a device code library where no kernels are defined. 
> > > > > > A device function is emitted, which calls a host device function 
> > > > > > which has a deferred diagnostic. All device functions that are 
> > > > > > emitted need to be checked.
> > > > > > 
> > > > > > Same with host functions that are emitted, which may call a host 
> > > > > > device function which has deferred diagnostic.
> > > > > > 
> > > > > > Also not just function calls need to be checked. A function address 
> > > > > > may be taken then called through function pointer. Therefore any 
> > > > > > reference to a function needs to be followed.
> > > > > > 
> > > > > > In the case of OpenMP, the initialization of a global function 
> > > > > > pointer which refers a function may trigger a deferred diangostic. 
> > > > > > There are tests for that.
> > > > > Right, I get that emitting deferred diagnostics for a declaration D 
> > > > > needs to trigger any deferred diagnostics in declarations used by D, 
> > > > > recursively.  You essentially have a graph of lazily-emitted 
> > > > > declarations (which may or may not have deferred diagnostics) and a 
> > > > > number of eagerly-emitted "root" declarations with use-edges leading 
> > > > > into that graph.  Any declaration that's reachable from a root will 
> > > > > need to be emitted and so needs to have any deferred diagnostics 
> > > > > emitted as well.  My question is why you're finding these roots with 
> > > > > a retroactive walk of the entire translation unit instead of either 
> > > > > building a list of roots as you go or (better yet) building a list of 
> > > > > lazily-emitted declarations that are used by those roots.  You can 
> > > > > unambiguously identify at the point of declaration whether an 

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 250960.
yaxunl marked 6 inline comments as done.
yaxunl added a comment.

revised by John's comments.


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

https://reviews.llvm.org/D70172

Files:
  clang/include/clang/Sema/ExternalSemaSource.h
  clang/include/clang/Sema/MultiplexExternalSemaSource.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Sema/MultiplexExternalSemaSource.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCUDA.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/OpenMP/declare_target_messages.cpp
  clang/test/OpenMP/nvptx_target_exceptions_messages.cpp
  clang/test/SemaCUDA/bad-calls-on-same-line.cu
  clang/test/SemaCUDA/call-device-fn-from-host.cu
  clang/test/SemaCUDA/call-host-fn-from-device.cu
  clang/test/SemaCUDA/openmp-target.cu
  clang/test/SemaCUDA/trace-through-global.cu

Index: clang/test/SemaCUDA/trace-through-global.cu
===
--- clang/test/SemaCUDA/trace-through-global.cu
+++ clang/test/SemaCUDA/trace-through-global.cu
@@ -38,7 +38,7 @@
   // Notice that these two diagnostics are different: Because the call to hd1
   // is not dependent on T, the call to hd1 comes from 'launch_kernel', while
   // the call to hd3, being dependent, comes from 'launch_kernel'.
-  hd1(); // expected-note {{called by 'launch_kernel'}}
+  hd1(); // expected-note {{called by 'launch_kernel'}}
   hd3(T()); // expected-note {{called by 'launch_kernel'}}
 }
 
Index: clang/test/SemaCUDA/openmp-target.cu
===
--- clang/test/SemaCUDA/openmp-target.cu
+++ clang/test/SemaCUDA/openmp-target.cu
@@ -16,9 +16,9 @@
 void bazzz() {bazz();}
 #pragma omp declare target to(bazzz) device_type(nohost)
 void any() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
-void host1() {bazz();}
+void host1() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
 #pragma omp declare target to(host1) device_type(host)
-void host2() {bazz();}
+void host2() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
 #pragma omp declare target to(host2)
 void device() {host1();}
 #pragma omp declare target to(device) device_type(nohost)
Index: clang/test/SemaCUDA/call-host-fn-from-device.cu
===
--- clang/test/SemaCUDA/call-host-fn-from-device.cu
+++ clang/test/SemaCUDA/call-host-fn-from-device.cu
@@ -56,14 +56,14 @@
 }
 
 template  __host__ __device__ void hd2() { host_fn(); }
-// expected-error@-1 2 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 __global__ void kernel() { hd2(); }
 
 __host__ __device__ void hd() { host_fn(); }
 // expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 
 template  __host__ __device__ void hd3() { host_fn(); }
-// expected-error@-1 2 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 __device__ void device_fn() { hd3(); }
 
 // No error because this is never instantiated.
Index: clang/test/SemaCUDA/call-device-fn-from-host.cu
===
--- clang/test/SemaCUDA/call-device-fn-from-host.cu
+++ clang/test/SemaCUDA/call-device-fn-from-host.cu
@@ -1,7 +1,7 @@
 // RUN: %clang_cc1 %s --std=c++11 -triple x86_64-unknown-linux -emit-llvm -o - \
 // RUN:   -verify -verify-ignore-unexpected=note
 // RUN: %clang_cc1 %s --std=c++11 -triple x86_64-unknown-linux -emit-llvm -o - \
-// RUN:   -verify -verify-ignore-unexpected=note -fopenmp
+// RUN:   -verify=expected,omp -verify-ignore-unexpected=note -fopenmp
 
 // Note: This test won't work with -fsyntax-only, because some of these errors
 // are emitted during codegen.
@@ -39,7 +39,7 @@
 }
 
 template  __host__ __device__ void hd2() { device_fn(); }
-// expected-error@-1 2 {{reference to __device__ function 'device_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __device__ function 'device_fn' in __host__ __device__ function}}
 void host_fn() { hd2(); }
 
 __host__ __device__ void hd() { device_fn(); }
Index: clang/test/SemaCUDA/bad-calls-on-same-line.cu
===
--- clang/test/SemaCUDA/bad-calls-on-same-line.cu
+++ clang/test/SemaCUDA/bad-calls-on-same-line.cu
@@ -33,8 +33,8 @@
 
 void host_fn() {
   hd();
-  hd();  // expected-note {{function 

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1514
+  void visitUsedDecl(SourceLocation Loc, Decl *D) {
+if (auto *TD = dyn_cast(D)) {
+  for (auto *DD : TD->decls()) {

rjmccall wrote:
> bader wrote:
> > yaxunl wrote:
> > > rjmccall wrote:
> > > > yaxunl wrote:
> > > > > rjmccall wrote:
> > > > > > erichkeane wrote:
> > > > > > > rjmccall wrote:
> > > > > > > > erichkeane wrote:
> > > > > > > > > Note that when recommitting this (if you choose to), this 
> > > > > > > > > needs to also handle NamespaceDecl.  We're a downstream and 
> > > > > > > > > discovered that this doesn't properly handle functions or 
> > > > > > > > > records handled in a namespace.
> > > > > > > > > 
> > > > > > > > > It can be implemented identically to TranslationUnitDecl.
> > > > > > > > Wait, what?  We shouldn't be doing this for TranslationUnitDecl 
> > > > > > > > either.   I don't even know how we're "using" a 
> > > > > > > > TranslationUnitDecl, but neither this case not the case for 
> > > > > > > > `NamespaceDecl` should be recursively using every declaration 
> > > > > > > > declared inside it.  If there's a declaration in a namespace 
> > > > > > > > that's being used, it should be getting visited as part of the 
> > > > > > > > actual use of it.
> > > > > > > > 
> > > > > > > > The logic for `RecordDecl` has the same problem.  
> > > > > > > Despite the name, this seems to be more of a home-written ast 
> > > > > > > walking class.  The entry point is the 'translation unit' which 
> > > > > > > seems to walk through everything in an attempt to find all the 
> > > > > > > functions (including those that are 'marked' as used by an 
> > > > > > > attribute).
> > > > > > > 
> > > > > > > You'll see the FunctionDecl section makes this assumption as well 
> > > > > > > (not necessarily that we got to a function via a call). IMO, this 
> > > > > > > approach is strange, and we should register entry points in some 
> > > > > > > manner (functions marked as emitted to the device in some 
> > > > > > > fashion), then just follow its call-graph (via the 
> > > > > > > clang::CallGraph?) to emit all of these functions.
> > > > > > > 
> > > > > > > It seemed really odd to see this approach here, but it seemed 
> > > > > > > well reviewed by the time I noticed it (via a downstream bug) so 
> > > > > > > I figured I'd lost my chance to disagree with the approach.
> > > > > > > 
> > > > > > > 
> > > > > > Sure, but `visitUsedDecl` isn't the right place to be entering the 
> > > > > > walk.  `visitUsedDecl` is supposed to be the *callback* from the 
> > > > > > walk.  If they need to walk all the global declarations to find 
> > > > > > kernels instead of tracking the kernels as they're encountered 
> > > > > > (which would be a *much* better approach), it should be done as a 
> > > > > > separate function.
> > > > > > 
> > > > > > I just missed this in the review.
> > > > > The deferred diagnostics could be initiated by non-kernel functions 
> > > > > or even host functions.
> > > > > 
> > > > > Let's consider a device code library where no kernels are defined. A 
> > > > > device function is emitted, which calls a host device function which 
> > > > > has a deferred diagnostic. All device functions that are emitted need 
> > > > > to be checked.
> > > > > 
> > > > > Same with host functions that are emitted, which may call a host 
> > > > > device function which has deferred diagnostic.
> > > > > 
> > > > > Also not just function calls need to be checked. A function address 
> > > > > may be taken then called through function pointer. Therefore any 
> > > > > reference to a function needs to be followed.
> > > > > 
> > > > > In the case of OpenMP, the initialization of a global function 
> > > > > pointer which refers a function may trigger a deferred diangostic. 
> > > > > There are tests for that.
> > > > Right, I get that emitting deferred diagnostics for a declaration D 
> > > > needs to trigger any deferred diagnostics in declarations used by D, 
> > > > recursively.  You essentially have a graph of lazily-emitted 
> > > > declarations (which may or may not have deferred diagnostics) and a 
> > > > number of eagerly-emitted "root" declarations with use-edges leading 
> > > > into that graph.  Any declaration that's reachable from a root will 
> > > > need to be emitted and so needs to have any deferred diagnostics 
> > > > emitted as well.  My question is why you're finding these roots with a 
> > > > retroactive walk of the entire translation unit instead of either 
> > > > building a list of roots as you go or (better yet) building a list of 
> > > > lazily-emitted declarations that are used by those roots.  You can 
> > > > unambiguously identify at the point of declaration whether an entity 
> > > > will be eagerly or lazily emitted, right?  If you just store those 
> > > > initial edges into the lazily-emitted declarations graph and then 
> > > > initiate the 

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1514
+  void visitUsedDecl(SourceLocation Loc, Decl *D) {
+if (auto *TD = dyn_cast(D)) {
+  for (auto *DD : TD->decls()) {

bader wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > yaxunl wrote:
> > > > rjmccall wrote:
> > > > > erichkeane wrote:
> > > > > > rjmccall wrote:
> > > > > > > erichkeane wrote:
> > > > > > > > Note that when recommitting this (if you choose to), this needs 
> > > > > > > > to also handle NamespaceDecl.  We're a downstream and 
> > > > > > > > discovered that this doesn't properly handle functions or 
> > > > > > > > records handled in a namespace.
> > > > > > > > 
> > > > > > > > It can be implemented identically to TranslationUnitDecl.
> > > > > > > Wait, what?  We shouldn't be doing this for TranslationUnitDecl 
> > > > > > > either.   I don't even know how we're "using" a 
> > > > > > > TranslationUnitDecl, but neither this case not the case for 
> > > > > > > `NamespaceDecl` should be recursively using every declaration 
> > > > > > > declared inside it.  If there's a declaration in a namespace 
> > > > > > > that's being used, it should be getting visited as part of the 
> > > > > > > actual use of it.
> > > > > > > 
> > > > > > > The logic for `RecordDecl` has the same problem.  
> > > > > > Despite the name, this seems to be more of a home-written ast 
> > > > > > walking class.  The entry point is the 'translation unit' which 
> > > > > > seems to walk through everything in an attempt to find all the 
> > > > > > functions (including those that are 'marked' as used by an 
> > > > > > attribute).
> > > > > > 
> > > > > > You'll see the FunctionDecl section makes this assumption as well 
> > > > > > (not necessarily that we got to a function via a call). IMO, this 
> > > > > > approach is strange, and we should register entry points in some 
> > > > > > manner (functions marked as emitted to the device in some fashion), 
> > > > > > then just follow its call-graph (via the clang::CallGraph?) to emit 
> > > > > > all of these functions.
> > > > > > 
> > > > > > It seemed really odd to see this approach here, but it seemed well 
> > > > > > reviewed by the time I noticed it (via a downstream bug) so I 
> > > > > > figured I'd lost my chance to disagree with the approach.
> > > > > > 
> > > > > > 
> > > > > Sure, but `visitUsedDecl` isn't the right place to be entering the 
> > > > > walk.  `visitUsedDecl` is supposed to be the *callback* from the 
> > > > > walk.  If they need to walk all the global declarations to find 
> > > > > kernels instead of tracking the kernels as they're encountered (which 
> > > > > would be a *much* better approach), it should be done as a separate 
> > > > > function.
> > > > > 
> > > > > I just missed this in the review.
> > > > The deferred diagnostics could be initiated by non-kernel functions or 
> > > > even host functions.
> > > > 
> > > > Let's consider a device code library where no kernels are defined. A 
> > > > device function is emitted, which calls a host device function which 
> > > > has a deferred diagnostic. All device functions that are emitted need 
> > > > to be checked.
> > > > 
> > > > Same with host functions that are emitted, which may call a host device 
> > > > function which has deferred diagnostic.
> > > > 
> > > > Also not just function calls need to be checked. A function address may 
> > > > be taken then called through function pointer. Therefore any reference 
> > > > to a function needs to be followed.
> > > > 
> > > > In the case of OpenMP, the initialization of a global function pointer 
> > > > which refers a function may trigger a deferred diangostic. There are 
> > > > tests for that.
> > > Right, I get that emitting deferred diagnostics for a declaration D needs 
> > > to trigger any deferred diagnostics in declarations used by D, 
> > > recursively.  You essentially have a graph of lazily-emitted declarations 
> > > (which may or may not have deferred diagnostics) and a number of 
> > > eagerly-emitted "root" declarations with use-edges leading into that 
> > > graph.  Any declaration that's reachable from a root will need to be 
> > > emitted and so needs to have any deferred diagnostics emitted as well.  
> > > My question is why you're finding these roots with a retroactive walk of 
> > > the entire translation unit instead of either building a list of roots as 
> > > you go or (better yet) building a list of lazily-emitted declarations 
> > > that are used by those roots.  You can unambiguously identify at the 
> > > point of declaration whether an entity will be eagerly or lazily emitted, 
> > > right?  If you just store those initial edges into the lazily-emitted 
> > > declarations graph and then initiate the recursive walk from them at the 
> > > end of the translation unit, you'll only end up walking declarations that 
> > > are actually relevant to your compilation, so you'll have much 

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 250666.
yaxunl marked 6 inline comments as done.
yaxunl added a comment.

revised by John's comments.


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

https://reviews.llvm.org/D70172

Files:
  clang/include/clang/Sema/ExternalSemaSource.h
  clang/include/clang/Sema/MultiplexExternalSemaSource.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Sema/MultiplexExternalSemaSource.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCUDA.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/OpenMP/declare_target_messages.cpp
  clang/test/OpenMP/nvptx_target_exceptions_messages.cpp
  clang/test/SemaCUDA/bad-calls-on-same-line.cu
  clang/test/SemaCUDA/call-device-fn-from-host.cu
  clang/test/SemaCUDA/call-host-fn-from-device.cu
  clang/test/SemaCUDA/openmp-target.cu
  clang/test/SemaCUDA/trace-through-global.cu

Index: clang/test/SemaCUDA/trace-through-global.cu
===
--- clang/test/SemaCUDA/trace-through-global.cu
+++ clang/test/SemaCUDA/trace-through-global.cu
@@ -38,7 +38,7 @@
   // Notice that these two diagnostics are different: Because the call to hd1
   // is not dependent on T, the call to hd1 comes from 'launch_kernel', while
   // the call to hd3, being dependent, comes from 'launch_kernel'.
-  hd1(); // expected-note {{called by 'launch_kernel'}}
+  hd1(); // expected-note {{called by 'launch_kernel'}}
   hd3(T()); // expected-note {{called by 'launch_kernel'}}
 }
 
Index: clang/test/SemaCUDA/openmp-target.cu
===
--- clang/test/SemaCUDA/openmp-target.cu
+++ clang/test/SemaCUDA/openmp-target.cu
@@ -16,9 +16,9 @@
 void bazzz() {bazz();}
 #pragma omp declare target to(bazzz) device_type(nohost)
 void any() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
-void host1() {bazz();}
+void host1() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
 #pragma omp declare target to(host1) device_type(host)
-void host2() {bazz();}
+void host2() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
 #pragma omp declare target to(host2)
 void device() {host1();}
 #pragma omp declare target to(device) device_type(nohost)
Index: clang/test/SemaCUDA/call-host-fn-from-device.cu
===
--- clang/test/SemaCUDA/call-host-fn-from-device.cu
+++ clang/test/SemaCUDA/call-host-fn-from-device.cu
@@ -56,14 +56,14 @@
 }
 
 template  __host__ __device__ void hd2() { host_fn(); }
-// expected-error@-1 2 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 __global__ void kernel() { hd2(); }
 
 __host__ __device__ void hd() { host_fn(); }
 // expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 
 template  __host__ __device__ void hd3() { host_fn(); }
-// expected-error@-1 2 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 __device__ void device_fn() { hd3(); }
 
 // No error because this is never instantiated.
Index: clang/test/SemaCUDA/call-device-fn-from-host.cu
===
--- clang/test/SemaCUDA/call-device-fn-from-host.cu
+++ clang/test/SemaCUDA/call-device-fn-from-host.cu
@@ -1,7 +1,7 @@
 // RUN: %clang_cc1 %s --std=c++11 -triple x86_64-unknown-linux -emit-llvm -o - \
 // RUN:   -verify -verify-ignore-unexpected=note
 // RUN: %clang_cc1 %s --std=c++11 -triple x86_64-unknown-linux -emit-llvm -o - \
-// RUN:   -verify -verify-ignore-unexpected=note -fopenmp
+// RUN:   -verify=expected,omp -verify-ignore-unexpected=note -fopenmp
 
 // Note: This test won't work with -fsyntax-only, because some of these errors
 // are emitted during codegen.
@@ -39,7 +39,7 @@
 }
 
 template  __host__ __device__ void hd2() { device_fn(); }
-// expected-error@-1 2 {{reference to __device__ function 'device_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __device__ function 'device_fn' in __host__ __device__ function}}
 void host_fn() { hd2(); }
 
 __host__ __device__ void hd() { device_fn(); }
Index: clang/test/SemaCUDA/bad-calls-on-same-line.cu
===
--- clang/test/SemaCUDA/bad-calls-on-same-line.cu
+++ clang/test/SemaCUDA/bad-calls-on-same-line.cu
@@ -33,8 +33,8 @@
 
 void host_fn() {
   hd();
-  hd();  // expected-note {{function 

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 23 inline comments as done.
yaxunl added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:1470
+/// diagnostics should be emitted.
+SmallVector DeclsToCheckForDeferredDiags;
+

rjmccall wrote:
> This needs to be saved and restored in modules / PCH.
done



Comment at: clang/lib/Sema/Sema.cpp:1468
   }
-  S.DeviceDeferredDiags.erase(It);
 

yaxunl wrote:
> Fznamznon wrote:
> > This particular change causes duplication of deferred diagnostics.
> > Consider the following example (please correct me if I'm doing something 
> > wrong, I'm not an expert in OpenMP):
> > 
> > ```
> > int foobar1() { throw 1; } // error is expected here
> > 
> > // let's try to use foobar1 in the code where exceptions aren't allowed
> > #pragma omp declare target
> > int (*B)() = 
> > #pragma omp end declare target
> > 
> > // and in some other place let's use foobar1 in device code again
> > #pragma omp declare target
> > int a = foobar1();
> > #pragma omp end declare target
> > 
> > ```
> > Then diagnostic for `foobar1`  will be duplicated for each use of `foobar1` 
> > under `target` directive.
> > I first experienced this behavior not with OpenMP, so I suppose reproducer 
> > can be done for each programming model which uses deferred diagnostics.
> The change is intentional so that each call chain causing the diagnostic can 
> be identified. The drawback is that it is more verbose.
> 
> I can change this behavior so that the diagnostic will be emitted only for 
> the first call chain that causes the diagnostic, if less verbose diagnostics 
> is preferred.
the change is intentional to report all use chains which result in deferred 
diagnostics, otherwise user may fix one issue then see another issue, instead 
of see all of the issues in one compilation.



Comment at: clang/lib/Sema/Sema.cpp:1472
   if (HasWarningOrError && ShowCallStack)
-emitCallStackNotes(S, FD);
+emitCallStackNotes(*this, FD);
+}

rjmccall wrote:
> Hmm.  I know this is existing code, but I just realized something.  I think 
> it's okay to not emit the notes on every diagnostic, but you might want to 
> emit them on the first diagnostic from a function instead of after the last.  
> If the real bug is that the program is using something it's not supposed to 
> use, and there are enough errors in that function to reach the error limit, 
> then the diagnostics emitter will associate these notes with a diagnostic 
> it's suppressing and so presumably suppress them as well, leaving the user 
> with no way to find this information.
done



Comment at: clang/lib/Sema/Sema.cpp:1494
+  visitUsedDecl(E->getLocation(), FD);
+}
+  }

rjmccall wrote:
> This needs to trigger if you use a variable with delayed diagnostics, too, 
> right?
> 
> When you add these methods to `UsedDeclVisitor`, you'll be able to remove 
> them here.
fixed



Comment at: clang/lib/Sema/Sema.cpp:1512
+Inherited::VisitCapturedStmt(Node);
+  }
+

rjmccall wrote:
> Should this also go in the base `UsedDeclVisitor`?  I'm less sure about that 
> because the captured statement is really always a part of the enclosing 
> function, right?  Should the delay mechanism just be looking through local 
> sub-contexts instead?
yes this one should also go to UsedDeclVisitor since this statement causes a 
RecordDecl generated which includes a FunctionDecl for a kernel, therefore this 
RecordDecl needs to be visited as used decl. I am not sure if other sub-context 
have the same effect. If so, I think they need to be handled case by case.



Comment at: clang/lib/Sema/UsedDeclVisitor.h:21
+template 
+class UsedDeclVisitor : public EvaluatedExprVisitor {
+protected:

rjmccall wrote:
> Could you add this in a separate patch?
extracted to https://reviews.llvm.org/D76262



Comment at: clang/lib/Sema/UsedDeclVisitor.h:30
+
+  Derived () { return *static_cast(this); }
+

rjmccall wrote:
> There should definitely be cases in here for every expression that uses a 
> declaration, including both `DeclRefExpr` and `MemberExpr`.  Those might be 
> overridden in subclasses, but that's their business; the default behavior 
> should be to visit every used decl.
done


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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:1470
+/// diagnostics should be emitted.
+SmallVector DeclsToCheckForDeferredDiags;
+

This needs to be saved and restored in modules / PCH.



Comment at: clang/lib/Sema/Sema.cpp:1472
   if (HasWarningOrError && ShowCallStack)
-emitCallStackNotes(S, FD);
+emitCallStackNotes(*this, FD);
+}

Hmm.  I know this is existing code, but I just realized something.  I think 
it's okay to not emit the notes on every diagnostic, but you might want to emit 
them on the first diagnostic from a function instead of after the last.  If the 
real bug is that the program is using something it's not supposed to use, and 
there are enough errors in that function to reach the error limit, then the 
diagnostics emitter will associate these notes with a diagnostic it's 
suppressing and so presumably suppress them as well, leaving the user with no 
way to find this information.



Comment at: clang/lib/Sema/Sema.cpp:1494
+  visitUsedDecl(E->getLocation(), FD);
+}
+  }

This needs to trigger if you use a variable with delayed diagnostics, too, 
right?

When you add these methods to `UsedDeclVisitor`, you'll be able to remove them 
here.



Comment at: clang/lib/Sema/Sema.cpp:1512
+Inherited::VisitCapturedStmt(Node);
+  }
+

Should this also go in the base `UsedDeclVisitor`?  I'm less sure about that 
because the captured statement is really always a part of the enclosing 
function, right?  Should the delay mechanism just be looking through local 
sub-contexts instead?



Comment at: clang/lib/Sema/UsedDeclVisitor.h:21
+template 
+class UsedDeclVisitor : public EvaluatedExprVisitor {
+protected:

Could you add this in a separate patch?



Comment at: clang/lib/Sema/UsedDeclVisitor.h:30
+
+  Derived () { return *static_cast(this); }
+

There should definitely be cases in here for every expression that uses a 
declaration, including both `DeclRefExpr` and `MemberExpr`.  Those might be 
overridden in subclasses, but that's their business; the default behavior 
should be to visit every used decl.


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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

Ping


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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 247275.
yaxunl added a comment.

Do not traverse the whole CU. Record potentially emitted functions and 
variables in the normal parsing and traverse them instead.

Also fixed bug 44948 and regression in check-mlir.


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

https://reviews.llvm.org/D70172

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCUDA.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/UsedDeclVisitor.h
  clang/test/OpenMP/bug44948.cpp
  clang/test/OpenMP/declare_target_messages.cpp
  clang/test/OpenMP/nvptx_target_exceptions_messages.cpp
  clang/test/SemaCUDA/bad-calls-on-same-line.cu
  clang/test/SemaCUDA/call-device-fn-from-host.cu
  clang/test/SemaCUDA/call-host-fn-from-device.cu
  clang/test/SemaCUDA/openmp-target.cu
  clang/test/SemaCUDA/trace-through-global.cu
  clang/test/SemaCXX/evaluated-expr-marker.cpp

Index: clang/test/SemaCXX/evaluated-expr-marker.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/evaluated-expr-marker.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -verify %s
+// expected-no-diagnostics
+
+// Make sure no assertion due to variable a not being marked as referenced.
+class A {
+public:
+  int foo();
+};
+
+static A a;
+
+struct B {
+  B(int x = a.foo());
+};
+
+void test() {
+  B x;
+}
Index: clang/test/SemaCUDA/trace-through-global.cu
===
--- clang/test/SemaCUDA/trace-through-global.cu
+++ clang/test/SemaCUDA/trace-through-global.cu
@@ -38,7 +38,7 @@
   // Notice that these two diagnostics are different: Because the call to hd1
   // is not dependent on T, the call to hd1 comes from 'launch_kernel', while
   // the call to hd3, being dependent, comes from 'launch_kernel'.
-  hd1(); // expected-note {{called by 'launch_kernel'}}
+  hd1(); // expected-note {{called by 'launch_kernel'}}
   hd3(T()); // expected-note {{called by 'launch_kernel'}}
 }
 
Index: clang/test/SemaCUDA/openmp-target.cu
===
--- clang/test/SemaCUDA/openmp-target.cu
+++ clang/test/SemaCUDA/openmp-target.cu
@@ -16,9 +16,9 @@
 void bazzz() {bazz();}
 #pragma omp declare target to(bazzz) device_type(nohost)
 void any() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
-void host1() {bazz();}
+void host1() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
 #pragma omp declare target to(host1) device_type(host)
-void host2() {bazz();}
+void host2() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
 #pragma omp declare target to(host2)
 void device() {host1();}
 #pragma omp declare target to(device) device_type(nohost)
Index: clang/test/SemaCUDA/call-host-fn-from-device.cu
===
--- clang/test/SemaCUDA/call-host-fn-from-device.cu
+++ clang/test/SemaCUDA/call-host-fn-from-device.cu
@@ -56,14 +56,14 @@
 }
 
 template  __host__ __device__ void hd2() { host_fn(); }
-// expected-error@-1 2 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 __global__ void kernel() { hd2(); }
 
 __host__ __device__ void hd() { host_fn(); }
 // expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 
 template  __host__ __device__ void hd3() { host_fn(); }
-// expected-error@-1 2 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 __device__ void device_fn() { hd3(); }
 
 // No error because this is never instantiated.
Index: clang/test/SemaCUDA/call-device-fn-from-host.cu
===
--- clang/test/SemaCUDA/call-device-fn-from-host.cu
+++ clang/test/SemaCUDA/call-device-fn-from-host.cu
@@ -1,7 +1,7 @@
 // RUN: %clang_cc1 %s --std=c++11 -triple x86_64-unknown-linux -emit-llvm -o - \
 // RUN:   -verify -verify-ignore-unexpected=note
 // RUN: %clang_cc1 %s --std=c++11 -triple x86_64-unknown-linux -emit-llvm -o - \
-// RUN:   -verify -verify-ignore-unexpected=note -fopenmp
+// RUN:   -verify=expected,omp -verify-ignore-unexpected=note -fopenmp
 
 // Note: This test won't work with -fsyntax-only, because some of these errors
 // are emitted during codegen.
@@ -39,7 +39,7 @@
 }
 
 template  __host__ __device__ void hd2() { device_fn(); }
-// expected-error@-1 2 {{reference to __device__ function 'device_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __device__ function 'device_fn' in __host__ __device__ function}}
 void host_fn() { hd2(); }
 

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

I still got assertion when I use the built clang with check-mlir. The reduced 
testcase is

  class A {
  public:
int foo();
  };
  
  static A a;
  
  struct B {
B(int x = a.foo());
  };
  
  void test() {
B x;
  }

The assertion I got is:

  clang: /home/yaxunl/git/llvm/llvm/tools/clang/lib/CodeGen/CGExpr.cpp:2628: 
clang::CodeGen::LValue clang::CodeGen::CodeGenFunction::EmitDeclRefLValue(const 
clang::DeclRefExpr *): Assertion `(ND->isUsed(false) || !isa(ND) || 
E->isNonOdrUse() || !E->getLocation().isValid()) && "Should not use decl 
without marking it used!"' failed.
  Stack dump:
  
  
   #0 0x0258c614 PrintStackTraceSignalHandler(void*) 
(/home/yaxunl/git/llvm/assert/bin/clang+0x258c614)
   #1 0x0258a1ae llvm::sys::RunSignalHandlers() 
(/home/yaxunl/git/llvm/assert/bin/clang+0x258a1ae)
   #2 0x0258b7a2 llvm::sys::CleanupOnSignal(unsigned long) 
(/home/yaxunl/git/llvm/assert/bin/clang+0x258b7a2)
   #3 0x0251d0c3 (anonymous 
namespace)::CrashRecoveryContextImpl::HandleCrash(int, unsigned long) 
(/home/yaxunl/git/llvm/assert/bin/clang+0x251d0c3)
   #4 0x0251d1fc CrashRecoverySignalHandler(int) 
(/home/yaxunl/git/llvm/assert/bin/clang+0x251d1fc)
   #5 0x7f0dde3bf390 __restore_rt 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x11390)
   #6 0x7f0ddcf29428 raise 
/build/glibc-LK5gWL/glibc-2.23/signal/../sysdeps/unix/sysv/linux/raise.c:54:0
   #7 0x7f0ddcf2b02a abort 
/build/glibc-LK5gWL/glibc-2.23/stdlib/abort.c:91:0
   #8 0x7f0ddcf21bd7 __assert_fail_base 
/build/glibc-LK5gWL/glibc-2.23/assert/assert.c:92:0
   #9 0x7f0ddcf21c82 (/lib/x86_64-linux-gnu/libc.so.6+0x2dc82)
  #10 0x02a1a5df 
clang::CodeGen::CodeGenFunction::EmitDeclRefLValue(clang::DeclRefExpr const*) 
(/home/yaxunl/git/llvm/assert/bin/clang+0x2a1a5df)
  #11 0x02a0dfb6 
clang::CodeGen::CodeGenFunction::EmitLValue(clang::Expr const*) 
(/home/yaxunl/git/llvm/assert/bin/clang+0x2a0dfb6)
  #12 0x02a39973 
clang::CodeGen::CodeGenFunction::EmitCXXMemberOrOperatorMemberCallExpr(clang::CallExpr
 const*, clang::CXXMethodDecl const*, clang::CodeGen::ReturnValueSlot, bool, 
clang::NestedNameSpecifier*, bool, clang::Expr const*) 
(/home/yaxunl/git/llvm/assert/bin/clang+0x2a39973)
  #13 0x02a389b9 
clang::CodeGen::CodeGenFunction::EmitCXXMemberCallExpr(clang::CXXMemberCallExpr 
const*, clang::CodeGen::ReturnValueSlot) 
(/home/yaxunl/git/llvm/assert/bin/clang+0x2a389b9)
  #14 0x02a28f95 
clang::CodeGen::CodeGenFunction::EmitCallExpr(clang::CallExpr const*, 
clang::CodeGen::ReturnValueSlot) 
(/home/yaxunl/git/llvm/assert/bin/clang+0x2a28f95)
  #15 0x02a5be29 (anonymous 
namespace)::ScalarExprEmitter::VisitCallExpr(clang::CallExpr const*) 
(/home/yaxunl/git/llvm/assert/bin/clang+0x2a5be29)
  #16 0x02a55b19 clang::StmtVisitorBase::Visit(clang::Stmt*) 
(/home/yaxunl/git/llvm/assert/bin/clang+0x2a55b19)
  #17 0x02a4b615 
clang::CodeGen::CodeGenFunction::EmitScalarExpr(clang::Expr const*, bool) 
(/home/yaxunl/git/llvm/assert/bin/clang+0x2a4b615)
  #18 0x02a0da30 
clang::CodeGen::CodeGenFunction::EmitAnyExpr(clang::Expr const*, 
clang::CodeGen::AggValueSlot, bool) 
(/home/yaxunl/git/llvm/assert/bin/clang+0x2a0da30)
  #19 0x02a0edde 
clang::CodeGen::CodeGenFunction::EmitAnyExprToTemp(clang::Expr const*) 
(/home/yaxunl/git/llvm/assert/bin/clang+0x2a0edde)
  #20 0x029cdd6b 
clang::CodeGen::CodeGenFunction::EmitCallArg(clang::CodeGen::CallArgList&, 
clang::Expr const*, clang::QualType) 
(/home/yaxunl/git/llvm/assert/bin/clang+0x29cdd6b)
  #21 0x029ccc41 
clang::CodeGen::CodeGenFunction::EmitCallArgs(clang::CodeGen::CallArgList&, 
llvm::ArrayRef, 
llvm::iterator_range >, 
clang::CodeGen::CodeGenFunction::AbstractCallee, unsigned int, 
clang::CodeGen::CodeGenFunction::EvaluationOrder) 
(/home/yaxunl/git/llvm/assert/bin/clang+0x29ccc41)
  #22 0x028d8e7b void 
clang::CodeGen::CodeGenFunction::EmitCallArgs(clang::CodeGen::CallArgList&,
 clang::FunctionProtoType const*, 
llvm::iterator_range >, 
clang::CodeGen::CodeGenFunction::AbstractCallee, unsigned int, 
clang::CodeGen::CodeGenFunction::EvaluationOrder) 
(/home/yaxunl/git/llvm/assert/bin/clang+0x28d8e7b)
  #23 0x029de431 
clang::CodeGen::CodeGenFunction::EmitCXXConstructorCall(clang::CXXConstructorDecl
 const*, clang::CXXCtorType, bool, bool, clang::CodeGen::AggValueSlot, 
clang::CXXConstructExpr const*) 
(/home/yaxunl/git/llvm/assert/bin/clang+0x29de431)
  #24 0x02a3b84e 
clang::CodeGen::CodeGenFunction::EmitCXXConstructExpr(clang::CXXConstructExpr 
const*, clang::CodeGen::AggValueSlot) 
(/home/yaxunl/git/llvm/assert/bin/clang+0x2a3b84e)
  #25 0x02a32a8b (anonymous 
namespace)::AggExprEmitter::VisitCXXConstructExpr(clang::CXXConstructExpr 
const*) (/home/yaxunl/git/llvm/assert/bin/clang+0x2a32a8b)
  #26 0x02a2d44f 
clang::CodeGen::CodeGenFunction::EmitAggExpr(clang::Expr const*, 

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

Also, we cannot remove traversing of RecordDecl and CapturedDecl encountered in 
function body since we have OpenMP test like this:

  int main() {
  #pragma omp target
{
  t1(0);
}
return 0;
  }

This results in a kernel function embedded in a captured record decl in AST. We 
have to drill into the record decl to get the kernel and the function called by 
it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D70172#1894036 , @yaxunl wrote:

> I tried recording functions to be emitted during normal parsing and using it 
> as starting point for the final traversal. It is quite promising. I only get 
> one lit test failure for OpenMP:
>
>   int foobar2();
>  
>   #pragma omp declare target
>   int (*B)() = 
>   #pragma omp end declare target
>  
>   int foobar2() { throw 1; } // expected-error {{cannot use 'throw' with 
> exceptions disabled}}
>  
>
>
> In this case, the emission state of foobar2 cannot be determined by itself. 
> It can only be determined to be emitted through variable B. Therefore, I also 
> need to record variables that are potentially emitted.


Okay.  Sounds like you have some common cause with 
https://reviews.llvm.org/D71227, then.  Pinging @hliao.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

I tried recording functions to be emitted during normal parsing and using it as 
starting point for the final traversal. It is quite promising. I only get one 
lit test failure for OpenMP:

  int foobar2();
  
  #pragma omp declare target
  int (*B)() = 
  #pragma omp end declare target
  
  int foobar2() { throw 1; } // expected-error {{cannot use 'throw' with 
exceptions disabled}}

In this case, the emission state of foobar2 cannot be determined by itself. It 
can only be determined to be emitted through variable B. Therefore, I also need 
to record variables that are potentially emitted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1514
+  void visitUsedDecl(SourceLocation Loc, Decl *D) {
+if (auto *TD = dyn_cast(D)) {
+  for (auto *DD : TD->decls()) {

yaxunl wrote:
> rjmccall wrote:
> > yaxunl wrote:
> > > rjmccall wrote:
> > > > erichkeane wrote:
> > > > > rjmccall wrote:
> > > > > > erichkeane wrote:
> > > > > > > Note that when recommitting this (if you choose to), this needs 
> > > > > > > to also handle NamespaceDecl.  We're a downstream and discovered 
> > > > > > > that this doesn't properly handle functions or records handled in 
> > > > > > > a namespace.
> > > > > > > 
> > > > > > > It can be implemented identically to TranslationUnitDecl.
> > > > > > Wait, what?  We shouldn't be doing this for TranslationUnitDecl 
> > > > > > either.   I don't even know how we're "using" a 
> > > > > > TranslationUnitDecl, but neither this case not the case for 
> > > > > > `NamespaceDecl` should be recursively using every declaration 
> > > > > > declared inside it.  If there's a declaration in a namespace that's 
> > > > > > being used, it should be getting visited as part of the actual use 
> > > > > > of it.
> > > > > > 
> > > > > > The logic for `RecordDecl` has the same problem.  
> > > > > Despite the name, this seems to be more of a home-written ast walking 
> > > > > class.  The entry point is the 'translation unit' which seems to walk 
> > > > > through everything in an attempt to find all the functions (including 
> > > > > those that are 'marked' as used by an attribute).
> > > > > 
> > > > > You'll see the FunctionDecl section makes this assumption as well 
> > > > > (not necessarily that we got to a function via a call). IMO, this 
> > > > > approach is strange, and we should register entry points in some 
> > > > > manner (functions marked as emitted to the device in some fashion), 
> > > > > then just follow its call-graph (via the clang::CallGraph?) to emit 
> > > > > all of these functions.
> > > > > 
> > > > > It seemed really odd to see this approach here, but it seemed well 
> > > > > reviewed by the time I noticed it (via a downstream bug) so I figured 
> > > > > I'd lost my chance to disagree with the approach.
> > > > > 
> > > > > 
> > > > Sure, but `visitUsedDecl` isn't the right place to be entering the 
> > > > walk.  `visitUsedDecl` is supposed to be the *callback* from the walk.  
> > > > If they need to walk all the global declarations to find kernels 
> > > > instead of tracking the kernels as they're encountered (which would be 
> > > > a *much* better approach), it should be done as a separate function.
> > > > 
> > > > I just missed this in the review.
> > > The deferred diagnostics could be initiated by non-kernel functions or 
> > > even host functions.
> > > 
> > > Let's consider a device code library where no kernels are defined. A 
> > > device function is emitted, which calls a host device function which has 
> > > a deferred diagnostic. All device functions that are emitted need to be 
> > > checked.
> > > 
> > > Same with host functions that are emitted, which may call a host device 
> > > function which has deferred diagnostic.
> > > 
> > > Also not just function calls need to be checked. A function address may 
> > > be taken then called through function pointer. Therefore any reference to 
> > > a function needs to be followed.
> > > 
> > > In the case of OpenMP, the initialization of a global function pointer 
> > > which refers a function may trigger a deferred diangostic. There are 
> > > tests for that.
> > Right, I get that emitting deferred diagnostics for a declaration D needs 
> > to trigger any deferred diagnostics in declarations used by D, recursively. 
> >  You essentially have a graph of lazily-emitted declarations (which may or 
> > may not have deferred diagnostics) and a number of eagerly-emitted "root" 
> > declarations with use-edges leading into that graph.  Any declaration 
> > that's reachable from a root will need to be emitted and so needs to have 
> > any deferred diagnostics emitted as well.  My question is why you're 
> > finding these roots with a retroactive walk of the entire translation unit 
> > instead of either building a list of roots as you go or (better yet) 
> > building a list of lazily-emitted declarations that are used by those 
> > roots.  You can unambiguously identify at the point of declaration whether 
> > an entity will be eagerly or lazily emitted, right?  If you just store 
> > those initial edges into the lazily-emitted declarations graph and then 
> > initiate the recursive walk from them at the end of the translation unit, 
> > you'll only end up walking declarations that are actually relevant to your 
> > compilation, so you'll have much better locality and (if this matters to 
> > you) you'll naturally work a lot better with PCH and modules.
> I will try the approach you suggested. Basically I will record the emitted 
> functions and 

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked an inline comment as done.
yaxunl added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1514
+  void visitUsedDecl(SourceLocation Loc, Decl *D) {
+if (auto *TD = dyn_cast(D)) {
+  for (auto *DD : TD->decls()) {

rjmccall wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > erichkeane wrote:
> > > > rjmccall wrote:
> > > > > erichkeane wrote:
> > > > > > Note that when recommitting this (if you choose to), this needs to 
> > > > > > also handle NamespaceDecl.  We're a downstream and discovered that 
> > > > > > this doesn't properly handle functions or records handled in a 
> > > > > > namespace.
> > > > > > 
> > > > > > It can be implemented identically to TranslationUnitDecl.
> > > > > Wait, what?  We shouldn't be doing this for TranslationUnitDecl 
> > > > > either.   I don't even know how we're "using" a TranslationUnitDecl, 
> > > > > but neither this case not the case for `NamespaceDecl` should be 
> > > > > recursively using every declaration declared inside it.  If there's a 
> > > > > declaration in a namespace that's being used, it should be getting 
> > > > > visited as part of the actual use of it.
> > > > > 
> > > > > The logic for `RecordDecl` has the same problem.  
> > > > Despite the name, this seems to be more of a home-written ast walking 
> > > > class.  The entry point is the 'translation unit' which seems to walk 
> > > > through everything in an attempt to find all the functions (including 
> > > > those that are 'marked' as used by an attribute).
> > > > 
> > > > You'll see the FunctionDecl section makes this assumption as well (not 
> > > > necessarily that we got to a function via a call). IMO, this approach 
> > > > is strange, and we should register entry points in some manner 
> > > > (functions marked as emitted to the device in some fashion), then just 
> > > > follow its call-graph (via the clang::CallGraph?) to emit all of these 
> > > > functions.
> > > > 
> > > > It seemed really odd to see this approach here, but it seemed well 
> > > > reviewed by the time I noticed it (via a downstream bug) so I figured 
> > > > I'd lost my chance to disagree with the approach.
> > > > 
> > > > 
> > > Sure, but `visitUsedDecl` isn't the right place to be entering the walk.  
> > > `visitUsedDecl` is supposed to be the *callback* from the walk.  If they 
> > > need to walk all the global declarations to find kernels instead of 
> > > tracking the kernels as they're encountered (which would be a *much* 
> > > better approach), it should be done as a separate function.
> > > 
> > > I just missed this in the review.
> > The deferred diagnostics could be initiated by non-kernel functions or even 
> > host functions.
> > 
> > Let's consider a device code library where no kernels are defined. A device 
> > function is emitted, which calls a host device function which has a 
> > deferred diagnostic. All device functions that are emitted need to be 
> > checked.
> > 
> > Same with host functions that are emitted, which may call a host device 
> > function which has deferred diagnostic.
> > 
> > Also not just function calls need to be checked. A function address may be 
> > taken then called through function pointer. Therefore any reference to a 
> > function needs to be followed.
> > 
> > In the case of OpenMP, the initialization of a global function pointer 
> > which refers a function may trigger a deferred diangostic. There are tests 
> > for that.
> Right, I get that emitting deferred diagnostics for a declaration D needs to 
> trigger any deferred diagnostics in declarations used by D, recursively.  You 
> essentially have a graph of lazily-emitted declarations (which may or may not 
> have deferred diagnostics) and a number of eagerly-emitted "root" 
> declarations with use-edges leading into that graph.  Any declaration that's 
> reachable from a root will need to be emitted and so needs to have any 
> deferred diagnostics emitted as well.  My question is why you're finding 
> these roots with a retroactive walk of the entire translation unit instead of 
> either building a list of roots as you go or (better yet) building a list of 
> lazily-emitted declarations that are used by those roots.  You can 
> unambiguously identify at the point of declaration whether an entity will be 
> eagerly or lazily emitted, right?  If you just store those initial edges into 
> the lazily-emitted declarations graph and then initiate the recursive walk 
> from them at the end of the translation unit, you'll only end up walking 
> declarations that are actually relevant to your compilation, so you'll have 
> much better locality and (if this matters to you) you'll naturally work a lot 
> better with PCH and modules.
I will try the approach you suggested. Basically I will record the emitted 
functions and variables during parsing and use them as starting point for the 
final traversal.

This should work for CUDA/HIP. However it may be tricky for 

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1514
+  void visitUsedDecl(SourceLocation Loc, Decl *D) {
+if (auto *TD = dyn_cast(D)) {
+  for (auto *DD : TD->decls()) {

yaxunl wrote:
> rjmccall wrote:
> > erichkeane wrote:
> > > rjmccall wrote:
> > > > erichkeane wrote:
> > > > > Note that when recommitting this (if you choose to), this needs to 
> > > > > also handle NamespaceDecl.  We're a downstream and discovered that 
> > > > > this doesn't properly handle functions or records handled in a 
> > > > > namespace.
> > > > > 
> > > > > It can be implemented identically to TranslationUnitDecl.
> > > > Wait, what?  We shouldn't be doing this for TranslationUnitDecl either. 
> > > >   I don't even know how we're "using" a TranslationUnitDecl, but 
> > > > neither this case not the case for `NamespaceDecl` should be 
> > > > recursively using every declaration declared inside it.  If there's a 
> > > > declaration in a namespace that's being used, it should be getting 
> > > > visited as part of the actual use of it.
> > > > 
> > > > The logic for `RecordDecl` has the same problem.  
> > > Despite the name, this seems to be more of a home-written ast walking 
> > > class.  The entry point is the 'translation unit' which seems to walk 
> > > through everything in an attempt to find all the functions (including 
> > > those that are 'marked' as used by an attribute).
> > > 
> > > You'll see the FunctionDecl section makes this assumption as well (not 
> > > necessarily that we got to a function via a call). IMO, this approach is 
> > > strange, and we should register entry points in some manner (functions 
> > > marked as emitted to the device in some fashion), then just follow its 
> > > call-graph (via the clang::CallGraph?) to emit all of these functions.
> > > 
> > > It seemed really odd to see this approach here, but it seemed well 
> > > reviewed by the time I noticed it (via a downstream bug) so I figured I'd 
> > > lost my chance to disagree with the approach.
> > > 
> > > 
> > Sure, but `visitUsedDecl` isn't the right place to be entering the walk.  
> > `visitUsedDecl` is supposed to be the *callback* from the walk.  If they 
> > need to walk all the global declarations to find kernels instead of 
> > tracking the kernels as they're encountered (which would be a *much* better 
> > approach), it should be done as a separate function.
> > 
> > I just missed this in the review.
> The deferred diagnostics could be initiated by non-kernel functions or even 
> host functions.
> 
> Let's consider a device code library where no kernels are defined. A device 
> function is emitted, which calls a host device function which has a deferred 
> diagnostic. All device functions that are emitted need to be checked.
> 
> Same with host functions that are emitted, which may call a host device 
> function which has deferred diagnostic.
> 
> Also not just function calls need to be checked. A function address may be 
> taken then called through function pointer. Therefore any reference to a 
> function needs to be followed.
> 
> In the case of OpenMP, the initialization of a global function pointer which 
> refers a function may trigger a deferred diangostic. There are tests for that.
Right, I get that emitting deferred diagnostics for a declaration D needs to 
trigger any deferred diagnostics in declarations used by D, recursively.  You 
essentially have a graph of lazily-emitted declarations (which may or may not 
have deferred diagnostics) and a number of eagerly-emitted "root" declarations 
with use-edges leading into that graph.  Any declaration that's reachable from 
a root will need to be emitted and so needs to have any deferred diagnostics 
emitted as well.  My question is why you're finding these roots with a 
retroactive walk of the entire translation unit instead of either building a 
list of roots as you go or (better yet) building a list of lazily-emitted 
declarations that are used by those roots.  You can unambiguously identify at 
the point of declaration whether an entity will be eagerly or lazily emitted, 
right?  If you just store those initial edges into the lazily-emitted 
declarations graph and then initiate the recursive walk from them at the end of 
the translation unit, you'll only end up walking declarations that are actually 
relevant to your compilation, so you'll have much better locality and (if this 
matters to you) you'll naturally work a lot better with PCH and modules.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked an inline comment as done.
yaxunl added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1514
+  void visitUsedDecl(SourceLocation Loc, Decl *D) {
+if (auto *TD = dyn_cast(D)) {
+  for (auto *DD : TD->decls()) {

rjmccall wrote:
> erichkeane wrote:
> > rjmccall wrote:
> > > erichkeane wrote:
> > > > Note that when recommitting this (if you choose to), this needs to also 
> > > > handle NamespaceDecl.  We're a downstream and discovered that this 
> > > > doesn't properly handle functions or records handled in a namespace.
> > > > 
> > > > It can be implemented identically to TranslationUnitDecl.
> > > Wait, what?  We shouldn't be doing this for TranslationUnitDecl either.   
> > > I don't even know how we're "using" a TranslationUnitDecl, but neither 
> > > this case not the case for `NamespaceDecl` should be recursively using 
> > > every declaration declared inside it.  If there's a declaration in a 
> > > namespace that's being used, it should be getting visited as part of the 
> > > actual use of it.
> > > 
> > > The logic for `RecordDecl` has the same problem.  
> > Despite the name, this seems to be more of a home-written ast walking 
> > class.  The entry point is the 'translation unit' which seems to walk 
> > through everything in an attempt to find all the functions (including those 
> > that are 'marked' as used by an attribute).
> > 
> > You'll see the FunctionDecl section makes this assumption as well (not 
> > necessarily that we got to a function via a call). IMO, this approach is 
> > strange, and we should register entry points in some manner (functions 
> > marked as emitted to the device in some fashion), then just follow its 
> > call-graph (via the clang::CallGraph?) to emit all of these functions.
> > 
> > It seemed really odd to see this approach here, but it seemed well reviewed 
> > by the time I noticed it (via a downstream bug) so I figured I'd lost my 
> > chance to disagree with the approach.
> > 
> > 
> Sure, but `visitUsedDecl` isn't the right place to be entering the walk.  
> `visitUsedDecl` is supposed to be the *callback* from the walk.  If they need 
> to walk all the global declarations to find kernels instead of tracking the 
> kernels as they're encountered (which would be a *much* better approach), it 
> should be done as a separate function.
> 
> I just missed this in the review.
The deferred diagnostics could be initiated by non-kernel functions or even 
host functions.

Let's consider a device code library where no kernels are defined. A device 
function is emitted, which calls a host device function which has a deferred 
diagnostic. All device functions that are emitted need to be checked.

Same with host functions that are emitted, which may call a host device 
function which has deferred diagnostic.

Also not just function calls need to be checked. A function address may be 
taken then called through function pointer. Therefore any reference to a 
function needs to be followed.

In the case of OpenMP, the initialization of a global function pointer which 
refers a function may trigger a deferred diangostic. There are tests for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D70172#1883567 , @ABataev wrote:

> Seems to me, it causes some other issues. See 
> https://bugs.llvm.org/show_bug.cgi?id=44948 for example


I will fix that bug.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Seems to me, it causes some other issues. See 
https://bugs.llvm.org/show_bug.cgi?id=44948 for example


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1514
+  void visitUsedDecl(SourceLocation Loc, Decl *D) {
+if (auto *TD = dyn_cast(D)) {
+  for (auto *DD : TD->decls()) {

erichkeane wrote:
> rjmccall wrote:
> > erichkeane wrote:
> > > Note that when recommitting this (if you choose to), this needs to also 
> > > handle NamespaceDecl.  We're a downstream and discovered that this 
> > > doesn't properly handle functions or records handled in a namespace.
> > > 
> > > It can be implemented identically to TranslationUnitDecl.
> > Wait, what?  We shouldn't be doing this for TranslationUnitDecl either.   I 
> > don't even know how we're "using" a TranslationUnitDecl, but neither this 
> > case not the case for `NamespaceDecl` should be recursively using every 
> > declaration declared inside it.  If there's a declaration in a namespace 
> > that's being used, it should be getting visited as part of the actual use 
> > of it.
> > 
> > The logic for `RecordDecl` has the same problem.  
> Despite the name, this seems to be more of a home-written ast walking class.  
> The entry point is the 'translation unit' which seems to walk through 
> everything in an attempt to find all the functions (including those that are 
> 'marked' as used by an attribute).
> 
> You'll see the FunctionDecl section makes this assumption as well (not 
> necessarily that we got to a function via a call). IMO, this approach is 
> strange, and we should register entry points in some manner (functions marked 
> as emitted to the device in some fashion), then just follow its call-graph 
> (via the clang::CallGraph?) to emit all of these functions.
> 
> It seemed really odd to see this approach here, but it seemed well reviewed 
> by the time I noticed it (via a downstream bug) so I figured I'd lost my 
> chance to disagree with the approach.
> 
> 
Sure, but `visitUsedDecl` isn't the right place to be entering the walk.  
`visitUsedDecl` is supposed to be the *callback* from the walk.  If they need 
to walk all the global declarations to find kernels instead of tracking the 
kernels as they're encountered (which would be a *much* better approach), it 
should be done as a separate function.

I just missed this in the review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1514
+  void visitUsedDecl(SourceLocation Loc, Decl *D) {
+if (auto *TD = dyn_cast(D)) {
+  for (auto *DD : TD->decls()) {

rjmccall wrote:
> erichkeane wrote:
> > Note that when recommitting this (if you choose to), this needs to also 
> > handle NamespaceDecl.  We're a downstream and discovered that this doesn't 
> > properly handle functions or records handled in a namespace.
> > 
> > It can be implemented identically to TranslationUnitDecl.
> Wait, what?  We shouldn't be doing this for TranslationUnitDecl either.   I 
> don't even know how we're "using" a TranslationUnitDecl, but neither this 
> case not the case for `NamespaceDecl` should be recursively using every 
> declaration declared inside it.  If there's a declaration in a namespace 
> that's being used, it should be getting visited as part of the actual use of 
> it.
> 
> The logic for `RecordDecl` has the same problem.  
Despite the name, this seems to be more of a home-written ast walking class.  
The entry point is the 'translation unit' which seems to walk through 
everything in an attempt to find all the functions (including those that are 
'marked' as used by an attribute).

You'll see the FunctionDecl section makes this assumption as well (not 
necessarily that we got to a function via a call). IMO, this approach is 
strange, and we should register entry points in some manner (functions marked 
as emitted to the device in some fashion), then just follow its call-graph (via 
the clang::CallGraph?) to emit all of these functions.

It seemed really odd to see this approach here, but it seemed well reviewed by 
the time I noticed it (via a downstream bug) so I figured I'd lost my chance to 
disagree with the approach.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1514
+  void visitUsedDecl(SourceLocation Loc, Decl *D) {
+if (auto *TD = dyn_cast(D)) {
+  for (auto *DD : TD->decls()) {

erichkeane wrote:
> Note that when recommitting this (if you choose to), this needs to also 
> handle NamespaceDecl.  We're a downstream and discovered that this doesn't 
> properly handle functions or records handled in a namespace.
> 
> It can be implemented identically to TranslationUnitDecl.
Wait, what?  We shouldn't be doing this for TranslationUnitDecl either.   I 
don't even know how we're "using" a TranslationUnitDecl, but neither this case 
not the case for `NamespaceDecl` should be recursively using every declaration 
declared inside it.  If there's a declaration in a namespace that's being used, 
it should be getting visited as part of the actual use of it.

The logic for `RecordDecl` has the same problem.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70172



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

RE: [PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Liu, Yaxun (Sam) via cfe-commits]

[AMD Official Use Only - Internal Distribution Only]

Will do. Thanks.

Sam

-Original Message-
From: Erich Keane via Phabricator  
Sent: Wednesday, February 19, 2020 8:52 AM
To: Liu, Yaxun (Sam) ; t...@google.com; rjmcc...@gmail.com; 
rich...@metafoo.co.uk; jdoerf...@anl.gov; a.bat...@hotmail.com
Cc: erich.ke...@intel.com; echri...@gmail.com; jpien...@google.com; 
mask...@google.com; michael.hl...@gmail.com; mariya.podchishcha...@intel.com; 
alexey.ba...@intel.com; zhang.guans...@gmail.com; her...@google.com; 
r...@google.com; cfe-commits@lists.llvm.org; mlek...@skidmore.edu; 
blitzrak...@gmail.com; shen...@google.com
Subject: [PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a 
post-parsing AST travese

[CAUTION: External Email]

erichkeane added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1514
+  void visitUsedDecl(SourceLocation Loc, Decl *D) {
+if (auto *TD = dyn_cast(D)) {
+  for (auto *DD : TD->decls()) {

Note that when recommitting this (if you choose to), this needs to also handle 
NamespaceDecl.  We're a downstream and discovered that this doesn't properly 
handle functions or records handled in a namespace.

It can be implemented identically to TranslationUnitDecl.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD70172%2Fnew%2Fdata=02%7C01%7Cyaxun.liu%40amd.com%7C9e1ec947ff5e4e65a52408d7b55bfd7b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637177279020210466sdata=KdCHlCMkdeIhQ8%2B11lXTxz8srsvmYKtk2UiIR5aYheo%3Dreserved=0

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD70172data=02%7C01%7Cyaxun.liu%40amd.com%7C9e1ec947ff5e4e65a52408d7b55bfd7b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637177279020210466sdata=iZhUpTdtwu8kR%2FIRvhMVwMbig28F960X9LWzPTGO9WI%3Dreserved=0


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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1514
+  void visitUsedDecl(SourceLocation Loc, Decl *D) {
+if (auto *TD = dyn_cast(D)) {
+  for (auto *DD : TD->decls()) {

Note that when recommitting this (if you choose to), this needs to also handle 
NamespaceDecl.  We're a downstream and discovered that this doesn't properly 
handle functions or records handled in a namespace.

It can be implemented identically to TranslationUnitDecl.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70172



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

RE: [PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Liu, Yaxun (Sam) via cfe-commits]

[AMD Official Use Only - Internal Distribution Only]

Reverted.

I will make sure it does not regress mlir before commit again.

Thanks.

Sam

-Original Message-
From: Eric Christopher via Phabricator  
Sent: Tuesday, February 18, 2020 11:21 AM
To: Liu, Yaxun (Sam) ; t...@google.com; rjmcc...@gmail.com; 
rich...@metafoo.co.uk; jdoerf...@anl.gov; a.bat...@hotmail.com
Cc: jpien...@google.com; mask...@google.com; michael.hl...@gmail.com; 
mariya.podchishcha...@intel.com; alexey.ba...@intel.com; 
zhang.guans...@gmail.com; her...@google.com; r...@google.com; 
cfe-commits@lists.llvm.org; mlek...@skidmore.edu; blitzrak...@gmail.com; 
shen...@google.com
Subject: [PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a 
post-parsing AST travese

[CAUTION: External Email]

echristo added a comment.

In D70172#1879481 
<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD70172%231879481data=02%7C01%7Cyaxun.liu%40amd.com%7Ce901b932c7c140a8a22c08d7b4a7b390%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637176504711389865sdata=8ifPEoeddzVyx3dpbTAvPeGKoa3oaTb6n18K6S5cVtY%3Dreserved=0>,
 @jpienaar wrote:

> This seems to result in triggering clang/lib/CodeGen/CGExpr.cpp:2626 when 
> compiling mlir/lib/Transforms/AffineDataCopyGeneration.cpp with clang build 
> with assertions on (clean build at e8e078c 
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FrGe8e078c8bf7987b95298062a644bac9eed26f988data=02%7C01%7Cyaxun.liu%40amd.com%7Ce901b932c7c140a8a22c08d7b4a7b390%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637176504711389865sdata=sM7yVx1tX%2BfMXRAvKKNMfXefVOQrg9LPxOXx1ByKAZs%3Dreserved=0>
>  just before this change, broken at this, assert triggering at build fix 
> commit).
>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbuildkite.com%2Fmlir%2Fmlir-core%2Fbuilds%2F2792%23a54fb239-718b-4f0b-a309-f83e46ceb252data=02%7C01%7Cyaxun.liu%40amd.com%7Ce901b932c7c140a8a22c08d7b4a7b390%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637176504711389865sdata=8OqEGO2vIUaVIEsaRfhzmTgJRGnMFR2ZpXH2LSkr9Ds%3Dreserved=0


Seems reasonable to revert if there's a testcase that they can get from 
rebuilding llvm with mlir enabled.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD70172%2Fnew%2Fdata=02%7C01%7Cyaxun.liu%40amd.com%7Ce901b932c7c140a8a22c08d7b4a7b390%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637176504711389865sdata=64lMij5bibrJnOLdYufLxO5u3GNpJe%2BzoSOdYef7zyw%3Dreserved=0

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD70172data=02%7C01%7Cyaxun.liu%40amd.com%7Ce901b932c7c140a8a22c08d7b4a7b390%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637176504711389865sdata=hfZYdMzByxpdoxtqLEP%2FAHWNKBHWkcXRbqXtrSqCfE4%3Dreserved=0


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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Eric Christopher via Phabricator via cfe-commits]

echristo added a comment.

In D70172#1879481 , @jpienaar wrote:

> This seems to result in triggering clang/lib/CodeGen/CGExpr.cpp:2626 when 
> compiling mlir/lib/Transforms/AffineDataCopyGeneration.cpp with clang build 
> with assertions on (clean build at e8e078c 
>  just 
> before this change, broken at this, assert triggering at build fix commit).
>
> https://buildkite.com/mlir/mlir-core/builds/2792#a54fb239-718b-4f0b-a309-f83e46ceb252


Seems reasonable to revert if there's a testcase that they can get from 
rebuilding llvm with mlir enabled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70172



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

RE: [PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Liu, Yaxun (Sam) via cfe-commits]

[AMD Official Use Only - Internal Distribution Only]

Probably I missed something. I will look again.

Thanks.

Sam

-Original Message-
From: Jacques Pienaar  
Sent: Tuesday, February 18, 2020 8:56 AM
To: Liu, Yaxun (Sam) 
Cc: reviews+d70172+public+e13d5528b180f...@reviews.llvm.org; t...@google.com; 
rjmcc...@gmail.com; rich...@metafoo.co.uk; jdoerf...@anl.gov; 
a.bat...@hotmail.com; mask...@google.com; michael.hl...@gmail.com; 
mariya.podchishcha...@intel.com; alexey.ba...@intel.com; 
zhang.guans...@gmail.com; her...@google.com; r...@google.com; 
cfe-commits@lists.llvm.org; mlek...@skidmore.edu; blitzrak...@gmail.com; 
shen...@google.com
Subject: Re: [PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a 
post-parsing AST travese

[CAUTION: External Email]
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Jacques Pienaar via cfe-commits]

How did you come to this conclusion?
https://buildkite.com/mlir/mlir-core/builds/2790#9ecce42d-5a20-4e52-8621-c7c0dcee7aa7
is
a clean build at e8e078c8bf7987b95298062a644bac9eed26f988 that is just
before 1b978ddba05cb15e22b4e75adb5e7362ad861987 (if I look at commits on
github) and at fb44b9db95a333efdfa9a33ddc1778f97428f5f5 that is just after
20c5968e0953d978be4d9d1062801e8758c393b5 it is also green. And I have
hourly builds that are green for days before.

-- Jacques

On Mon, Feb 17, 2020 at 4:51 PM Liu, Yaxun (Sam)  wrote:

> [AMD Official Use Only - Internal Distribution Only]
>
> It seems to be caused by some commit hash before
> 20c5968e0953d978be4d9d1062801e8758c393b5.
>
> Sam
>
> -Original Message-
> From: Jacques Pienaar via Phabricator 
> Sent: Monday, February 17, 2020 10:58 AM
> To: Liu, Yaxun (Sam) ; t...@google.com;
> rjmcc...@gmail.com; rich...@metafoo.co.uk; jdoerf...@anl.gov;
> a.bat...@hotmail.com
> Cc: jpien...@google.com; mask...@google.com; michael.hl...@gmail.com;
> mariya.podchishcha...@intel.com; alexey.ba...@intel.com;
> zhang.guans...@gmail.com; her...@google.com; r...@google.com;
> cfe-commits@lists.llvm.org; mlek...@skidmore.edu; blitzrak...@gmail.com;
> shen...@google.com
> Subject: [PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by
> a post-parsing AST travese
>
> [CAUTION: External Email]
>
> jpienaar added a comment.
>
> This seems to result in triggering clang/lib/CodeGen/CGExpr.cpp:2626 when
> compiling mlir/lib/Transforms/AffineDataCopyGeneration.cpp with clang build
> with assertions on (clean build at e8e078c <
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FrGe8e078c8bf7987b95298062a644bac9eed26f988data=02%7C01%7Cyaxun.liu%40amd.com%7C929d8d11429a4c4072e108d7b3db6042%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175627129064872sdata=J%2FZKM2oBbn9XTXKRLgonFUWYktyIjlKQF5LVmwmvnkQ%3Dreserved=0>
> just before this change, broken at this, assert triggering at build fix
> commit).
>
>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbuildkite.com%2Fmlir%2Fmlir-core%2Fbuilds%2F2792%23a54fb239-718b-4f0b-a309-f83e46ceb252data=02%7C01%7Cyaxun.liu%40amd.com%7C929d8d11429a4c4072e108d7b3db6042%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175627129064872sdata=nma5u9l5h6tkzQEX0hVfYe6VJQBXubt%2FCOF%2BWyWSujM%3Dreserved=0
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD70172%2Fnew%2Fdata=02%7C01%7Cyaxun.liu%40amd.com%7C929d8d11429a4c4072e108d7b3db6042%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175627129064872sdata=yGGqSBdf6zVYW7IDFW57UEagQAyvaOfn2a8xu%2BaIr9o%3Dreserved=0
>
>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD70172data=02%7C01%7Cyaxun.liu%40amd.com%7C929d8d11429a4c4072e108d7b3db6042%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175627129064872sdata=sbHpgiR9tKyv0%2F4ZnuhUX%2BFaIHdtwoHTyOJxX1%2F46mg%3Dreserved=0
>
>
>


smime.p7s
Description: S/MIME Cryptographic Signature
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

RE: [PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Liu, Yaxun (Sam) via cfe-commits]

[AMD Official Use Only - Internal Distribution Only]

It seems to be caused by some commit hash before 
20c5968e0953d978be4d9d1062801e8758c393b5.

Sam

-Original Message-
From: Jacques Pienaar via Phabricator  
Sent: Monday, February 17, 2020 10:58 AM
To: Liu, Yaxun (Sam) ; t...@google.com; rjmcc...@gmail.com; 
rich...@metafoo.co.uk; jdoerf...@anl.gov; a.bat...@hotmail.com
Cc: jpien...@google.com; mask...@google.com; michael.hl...@gmail.com; 
mariya.podchishcha...@intel.com; alexey.ba...@intel.com; 
zhang.guans...@gmail.com; her...@google.com; r...@google.com; 
cfe-commits@lists.llvm.org; mlek...@skidmore.edu; blitzrak...@gmail.com; 
shen...@google.com
Subject: [PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a 
post-parsing AST travese

[CAUTION: External Email]

jpienaar added a comment.

This seems to result in triggering clang/lib/CodeGen/CGExpr.cpp:2626 when 
compiling mlir/lib/Transforms/AffineDataCopyGeneration.cpp with clang build 
with assertions on (clean build at e8e078c 
<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FrGe8e078c8bf7987b95298062a644bac9eed26f988data=02%7C01%7Cyaxun.liu%40amd.com%7C929d8d11429a4c4072e108d7b3db6042%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175627129064872sdata=J%2FZKM2oBbn9XTXKRLgonFUWYktyIjlKQF5LVmwmvnkQ%3Dreserved=0>
 just before this change, broken at this, assert triggering at build fix 
commit).

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbuildkite.com%2Fmlir%2Fmlir-core%2Fbuilds%2F2792%23a54fb239-718b-4f0b-a309-f83e46ceb252data=02%7C01%7Cyaxun.liu%40amd.com%7C929d8d11429a4c4072e108d7b3db6042%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175627129064872sdata=nma5u9l5h6tkzQEX0hVfYe6VJQBXubt%2FCOF%2BWyWSujM%3Dreserved=0


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD70172%2Fnew%2Fdata=02%7C01%7Cyaxun.liu%40amd.com%7C929d8d11429a4c4072e108d7b3db6042%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175627129064872sdata=yGGqSBdf6zVYW7IDFW57UEagQAyvaOfn2a8xu%2BaIr9o%3Dreserved=0

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD70172data=02%7C01%7Cyaxun.liu%40amd.com%7C929d8d11429a4c4072e108d7b3db6042%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175627129064872sdata=sbHpgiR9tKyv0%2F4ZnuhUX%2BFaIHdtwoHTyOJxX1%2F46mg%3Dreserved=0


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

RE: [PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Liu, Yaxun (Sam) via cfe-commits]

[AMD Official Use Only - Internal Distribution Only]

I am taking a look. Thanks.

Sam

-Original Message-
From: Jacques Pienaar via Phabricator  
Sent: Monday, February 17, 2020 10:58 AM
To: Liu, Yaxun (Sam) ; t...@google.com; rjmcc...@gmail.com; 
rich...@metafoo.co.uk; jdoerf...@anl.gov; a.bat...@hotmail.com
Cc: jpien...@google.com; mask...@google.com; michael.hl...@gmail.com; 
mariya.podchishcha...@intel.com; alexey.ba...@intel.com; 
zhang.guans...@gmail.com; her...@google.com; r...@google.com; 
cfe-commits@lists.llvm.org; mlek...@skidmore.edu; blitzrak...@gmail.com; 
shen...@google.com
Subject: [PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a 
post-parsing AST travese

[CAUTION: External Email]

jpienaar added a comment.

This seems to result in triggering clang/lib/CodeGen/CGExpr.cpp:2626 when 
compiling mlir/lib/Transforms/AffineDataCopyGeneration.cpp with clang build 
with assertions on (clean build at e8e078c 
<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FrGe8e078c8bf7987b95298062a644bac9eed26f988data=02%7C01%7Cyaxun.liu%40amd.com%7C929d8d11429a4c4072e108d7b3db6042%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175627129064872sdata=J%2FZKM2oBbn9XTXKRLgonFUWYktyIjlKQF5LVmwmvnkQ%3Dreserved=0>
 just before this change, broken at this, assert triggering at build fix 
commit).

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbuildkite.com%2Fmlir%2Fmlir-core%2Fbuilds%2F2792%23a54fb239-718b-4f0b-a309-f83e46ceb252data=02%7C01%7Cyaxun.liu%40amd.com%7C929d8d11429a4c4072e108d7b3db6042%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175627129064872sdata=nma5u9l5h6tkzQEX0hVfYe6VJQBXubt%2FCOF%2BWyWSujM%3Dreserved=0


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD70172%2Fnew%2Fdata=02%7C01%7Cyaxun.liu%40amd.com%7C929d8d11429a4c4072e108d7b3db6042%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175627129064872sdata=yGGqSBdf6zVYW7IDFW57UEagQAyvaOfn2a8xu%2BaIr9o%3Dreserved=0

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD70172data=02%7C01%7Cyaxun.liu%40amd.com%7C929d8d11429a4c4072e108d7b3db6042%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175627129064872sdata=sbHpgiR9tKyv0%2F4ZnuhUX%2BFaIHdtwoHTyOJxX1%2F46mg%3Dreserved=0


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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Jacques Pienaar via Phabricator via cfe-commits]

jpienaar added a comment.

This seems to result in triggering clang/lib/CodeGen/CGExpr.cpp:2626 when 
compiling mlir/lib/Transforms/AffineDataCopyGeneration.cpp with clang build 
with assertions on (clean build at e8e078c 
 just 
before this change, broken at this, assert triggering at build fix commit).

https://buildkite.com/mlir/mlir-core/builds/2792#a54fb239-718b-4f0b-a309-f83e46ceb252


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked an inline comment as done.
yaxunl added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1468
   }
-  S.DeviceDeferredDiags.erase(It);
 

Fznamznon wrote:
> This particular change causes duplication of deferred diagnostics.
> Consider the following example (please correct me if I'm doing something 
> wrong, I'm not an expert in OpenMP):
> 
> ```
> int foobar1() { throw 1; } // error is expected here
> 
> // let's try to use foobar1 in the code where exceptions aren't allowed
> #pragma omp declare target
> int (*B)() = 
> #pragma omp end declare target
> 
> // and in some other place let's use foobar1 in device code again
> #pragma omp declare target
> int a = foobar1();
> #pragma omp end declare target
> 
> ```
> Then diagnostic for `foobar1`  will be duplicated for each use of `foobar1` 
> under `target` directive.
> I first experienced this behavior not with OpenMP, so I suppose reproducer 
> can be done for each programming model which uses deferred diagnostics.
The change is intentional so that each call chain causing the diagnostic can be 
identified. The drawback is that it is more verbose.

I can change this behavior so that the diagnostic will be emitted only for the 
first call chain that causes the diagnostic, if less verbose diagnostics is 
preferred.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1468
   }
-  S.DeviceDeferredDiags.erase(It);
 

This particular change causes duplication of deferred diagnostics.
Consider the following example (please correct me if I'm doing something wrong, 
I'm not an expert in OpenMP):

```
int foobar1() { throw 1; } // error is expected here

// let's try to use foobar1 in the code where exceptions aren't allowed
#pragma omp declare target
int (*B)() = 
#pragma omp end declare target

// and in some other place let's use foobar1 in device code again
#pragma omp declare target
int a = foobar1();
#pragma omp end declare target

```
Then diagnostic for `foobar1`  will be duplicated for each use of `foobar1` 
under `target` directive.
I first experienced this behavior not with OpenMP, so I suppose reproducer can 
be done for each programming model which uses deferred diagnostics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70172



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

RE: [PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Liu, Yaxun (Sam) via cfe-commits]

[AMD Official Use Only - Internal Distribution Only]

Thanks a lot for fixing it.

Sam

-Original Message-
From: Fangrui Song via Phabricator  
Sent: Sunday, February 16, 2020 8:41 PM
To: Liu, Yaxun (Sam) ; t...@google.com; rjmcc...@gmail.com; 
rich...@metafoo.co.uk; jdoerf...@anl.gov; a.bat...@hotmail.com
Cc: mask...@google.com; michael.hl...@gmail.com; 
mariya.podchishcha...@intel.com; alexey.ba...@intel.com; 
zhang.guans...@gmail.com; her...@google.com; r...@google.com; 
cfe-commits@lists.llvm.org; mlek...@skidmore.edu; blitzrak...@gmail.com; 
shen...@google.com
Subject: [PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a 
post-parsing AST travese

[CAUTION: External Email]

MaskRay added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:14

+#include "UsedDeclVisitor.h"
 #include "clang/AST/ASTContext.h"

hliao wrote:
> this file is missing and breaks the build
Fixed by c7fa409bcadaf4ddba1862b2e52349e0ab03d1b4


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD70172%2Fnew%2Fdata=02%7C01%7Cyaxun.liu%40amd.com%7Cd887ec3ab69146d8ee4e08d7b36392d6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175112565124973sdata=16dU6RUcV1M9%2BMoNbgytch179KaG6vr9fJO%2BzeYJ4z4%3Dreserved=0

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD70172data=02%7C01%7Cyaxun.liu%40amd.com%7Cd887ec3ab69146d8ee4e08d7b36392d6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175112565124973sdata=gDh1FqkmR7VzE73YXtGyX8Z3pr8FCJwjnZH9RaVv%2FCo%3Dreserved=0


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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:14
 
+#include "UsedDeclVisitor.h"
 #include "clang/AST/ASTContext.h"

hliao wrote:
> this file is missing and breaks the build
Fixed by c7fa409bcadaf4ddba1862b2e52349e0ab03d1b4


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Michael Liao via Phabricator via cfe-commits]

hliao added a comment.

one header is missing and breaks the build




Comment at: clang/lib/Sema/Sema.cpp:14
 
+#include "UsedDeclVisitor.h"
 #include "clang/AST/ASTContext.h"

this file is missing and breaks the build


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
yaxunl marked an inline comment as done.
Closed by commit rG1b978ddba05c: [CUDA][HIP][OpenMP] Emit deferred diagnostics 
by a post-parsing AST travese (authored by yaxunl).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D70172?vs=242296=244912#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70172

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCUDA.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/declare_target_messages.cpp
  clang/test/OpenMP/nvptx_target_exceptions_messages.cpp
  clang/test/SemaCUDA/bad-calls-on-same-line.cu
  clang/test/SemaCUDA/call-device-fn-from-host.cu
  clang/test/SemaCUDA/call-host-fn-from-device.cu
  clang/test/SemaCUDA/openmp-target.cu
  clang/test/SemaCUDA/trace-through-global.cu

Index: clang/test/SemaCUDA/trace-through-global.cu
===
--- clang/test/SemaCUDA/trace-through-global.cu
+++ clang/test/SemaCUDA/trace-through-global.cu
@@ -38,7 +38,7 @@
   // Notice that these two diagnostics are different: Because the call to hd1
   // is not dependent on T, the call to hd1 comes from 'launch_kernel', while
   // the call to hd3, being dependent, comes from 'launch_kernel'.
-  hd1(); // expected-note {{called by 'launch_kernel'}}
+  hd1(); // expected-note {{called by 'launch_kernel'}}
   hd3(T()); // expected-note {{called by 'launch_kernel'}}
 }
 
Index: clang/test/SemaCUDA/openmp-target.cu
===
--- clang/test/SemaCUDA/openmp-target.cu
+++ clang/test/SemaCUDA/openmp-target.cu
@@ -16,9 +16,9 @@
 void bazzz() {bazz();}
 #pragma omp declare target to(bazzz) device_type(nohost)
 void any() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
-void host1() {bazz();}
+void host1() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
 #pragma omp declare target to(host1) device_type(host)
-void host2() {bazz();}
+void host2() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
 #pragma omp declare target to(host2)
 void device() {host1();}
 #pragma omp declare target to(device) device_type(nohost)
Index: clang/test/SemaCUDA/call-host-fn-from-device.cu
===
--- clang/test/SemaCUDA/call-host-fn-from-device.cu
+++ clang/test/SemaCUDA/call-host-fn-from-device.cu
@@ -56,14 +56,14 @@
 }
 
 template  __host__ __device__ void hd2() { host_fn(); }
-// expected-error@-1 2 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 __global__ void kernel() { hd2(); }
 
 __host__ __device__ void hd() { host_fn(); }
 // expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 
 template  __host__ __device__ void hd3() { host_fn(); }
-// expected-error@-1 2 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 __device__ void device_fn() { hd3(); }
 
 // No error because this is never instantiated.
Index: clang/test/SemaCUDA/call-device-fn-from-host.cu
===
--- clang/test/SemaCUDA/call-device-fn-from-host.cu
+++ clang/test/SemaCUDA/call-device-fn-from-host.cu
@@ -1,7 +1,7 @@
 // RUN: %clang_cc1 %s --std=c++11 -triple x86_64-unknown-linux -emit-llvm -o - \
 // RUN:   -verify -verify-ignore-unexpected=note
 // RUN: %clang_cc1 %s --std=c++11 -triple x86_64-unknown-linux -emit-llvm -o - \
-// RUN:   -verify -verify-ignore-unexpected=note -fopenmp
+// RUN:   -verify=expected,omp -verify-ignore-unexpected=note -fopenmp
 
 // Note: This test won't work with -fsyntax-only, because some of these errors
 // are emitted during codegen.
@@ -39,7 +39,7 @@
 }
 
 template  __host__ __device__ void hd2() { device_fn(); }
-// expected-error@-1 2 {{reference to __device__ function 'device_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __device__ function 'device_fn' in __host__ __device__ function}}
 void host_fn() { hd2(); }
 
 __host__ __device__ void hd() { device_fn(); }
Index: clang/test/SemaCUDA/bad-calls-on-same-line.cu
===
--- clang/test/SemaCUDA/bad-calls-on-same-line.cu
+++ clang/test/SemaCUDA/bad-calls-on-same-line.cu
@@ -33,8 +33,8 @@
 
 void host_fn() {
   hd();
-  hd();  // expected-note 

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 2 inline comments as done.
yaxunl added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:17254
+SourceLocation(), Context.getTranslationUnitDecl());
+  }
 

rjmccall wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > Thanks, this looks a lot better.
> > > 
> > > Should this be moved to SemaOpenMP.cpp (and renamed to be 
> > > OpenMP-specific), or do you think it's going to be useful in other modes?
> > It is not just for OpenMP. Deferred diagnostics are also emitted by 
> > CUDA/HIP.
> Okay.  Can it go in Sema.cpp next to the other overload of 
> `emitDeferredDiags`, then?  There isn't really much purpose to it being in 
> this file.
will do when committing. thanks.


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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

One minor request, but otherwise LGTM; feel free to commit with that change.




Comment at: clang/lib/Sema/SemaExpr.cpp:17254
+SourceLocation(), Context.getTranslationUnitDecl());
+  }
 

yaxunl wrote:
> rjmccall wrote:
> > Thanks, this looks a lot better.
> > 
> > Should this be moved to SemaOpenMP.cpp (and renamed to be OpenMP-specific), 
> > or do you think it's going to be useful in other modes?
> It is not just for OpenMP. Deferred diagnostics are also emitted by CUDA/HIP.
Okay.  Can it go in Sema.cpp next to the other overload of `emitDeferredDiags`, 
then?  There isn't really much purpose to it being in this file.


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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:17254
+SourceLocation(), Context.getTranslationUnitDecl());
+  }
 

rjmccall wrote:
> Thanks, this looks a lot better.
> 
> Should this be moved to SemaOpenMP.cpp (and renamed to be OpenMP-specific), 
> or do you think it's going to be useful in other modes?
It is not just for OpenMP. Deferred diagnostics are also emitted by CUDA/HIP.


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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 242296.
yaxunl marked 4 inline comments as done.
yaxunl added a comment.

revised by John's comments.


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

https://reviews.llvm.org/D70172

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCUDA.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/UsedDeclVisitor.h
  clang/test/OpenMP/declare_target_messages.cpp
  clang/test/OpenMP/nvptx_target_exceptions_messages.cpp
  clang/test/SemaCUDA/bad-calls-on-same-line.cu
  clang/test/SemaCUDA/call-device-fn-from-host.cu
  clang/test/SemaCUDA/call-host-fn-from-device.cu
  clang/test/SemaCUDA/openmp-target.cu
  clang/test/SemaCUDA/trace-through-global.cu

Index: clang/test/SemaCUDA/trace-through-global.cu
===
--- clang/test/SemaCUDA/trace-through-global.cu
+++ clang/test/SemaCUDA/trace-through-global.cu
@@ -38,7 +38,7 @@
   // Notice that these two diagnostics are different: Because the call to hd1
   // is not dependent on T, the call to hd1 comes from 'launch_kernel', while
   // the call to hd3, being dependent, comes from 'launch_kernel'.
-  hd1(); // expected-note {{called by 'launch_kernel'}}
+  hd1(); // expected-note {{called by 'launch_kernel'}}
   hd3(T()); // expected-note {{called by 'launch_kernel'}}
 }
 
Index: clang/test/SemaCUDA/openmp-target.cu
===
--- clang/test/SemaCUDA/openmp-target.cu
+++ clang/test/SemaCUDA/openmp-target.cu
@@ -16,9 +16,9 @@
 void bazzz() {bazz();}
 #pragma omp declare target to(bazzz) device_type(nohost)
 void any() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
-void host1() {bazz();}
+void host1() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
 #pragma omp declare target to(host1) device_type(host)
-void host2() {bazz();}
+void host2() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
 #pragma omp declare target to(host2)
 void device() {host1();}
 #pragma omp declare target to(device) device_type(nohost)
Index: clang/test/SemaCUDA/call-host-fn-from-device.cu
===
--- clang/test/SemaCUDA/call-host-fn-from-device.cu
+++ clang/test/SemaCUDA/call-host-fn-from-device.cu
@@ -56,14 +56,14 @@
 }
 
 template  __host__ __device__ void hd2() { host_fn(); }
-// expected-error@-1 2 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 __global__ void kernel() { hd2(); }
 
 __host__ __device__ void hd() { host_fn(); }
 // expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 
 template  __host__ __device__ void hd3() { host_fn(); }
-// expected-error@-1 2 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 __device__ void device_fn() { hd3(); }
 
 // No error because this is never instantiated.
Index: clang/test/SemaCUDA/call-device-fn-from-host.cu
===
--- clang/test/SemaCUDA/call-device-fn-from-host.cu
+++ clang/test/SemaCUDA/call-device-fn-from-host.cu
@@ -1,7 +1,7 @@
 // RUN: %clang_cc1 %s --std=c++11 -triple x86_64-unknown-linux -emit-llvm -o - \
 // RUN:   -verify -verify-ignore-unexpected=note
 // RUN: %clang_cc1 %s --std=c++11 -triple x86_64-unknown-linux -emit-llvm -o - \
-// RUN:   -verify -verify-ignore-unexpected=note -fopenmp
+// RUN:   -verify=expected,omp -verify-ignore-unexpected=note -fopenmp
 
 // Note: This test won't work with -fsyntax-only, because some of these errors
 // are emitted during codegen.
@@ -39,7 +39,7 @@
 }
 
 template  __host__ __device__ void hd2() { device_fn(); }
-// expected-error@-1 2 {{reference to __device__ function 'device_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __device__ function 'device_fn' in __host__ __device__ function}}
 void host_fn() { hd2(); }
 
 __host__ __device__ void hd() { device_fn(); }
Index: clang/test/SemaCUDA/bad-calls-on-same-line.cu
===
--- clang/test/SemaCUDA/bad-calls-on-same-line.cu
+++ clang/test/SemaCUDA/bad-calls-on-same-line.cu
@@ -33,8 +33,8 @@
 
 void host_fn() {
   hd();
-  hd();  // expected-note {{function template specialization 'hd'}}
+  hd();
   // expected-note@-1 {{called by 'host_fn'}}
-  hd();  // expected-note {{function template specialization 'hd'}}
+  hd();
   // expected-note@-1 {{called by 'host_fn'}}
 }
Index: clang/test/OpenMP/nvptx_target_exceptions_messages.cpp

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:17254
+SourceLocation(), Context.getTranslationUnitDecl());
+  }
 

Thanks, this looks a lot better.

Should this be moved to SemaOpenMP.cpp (and renamed to be OpenMP-specific), or 
do you think it's going to be useful in other modes?



Comment at: clang/lib/Sema/UsedDeclVisitor.h:1
+//===- CoroutineStmtBuilder.h - Implicit coroutine stmt builder -*- C++ 
-*-===//
+//

Please fix this line.



Comment at: clang/lib/Sema/UsedDeclVisitor.h:9
+//  This file defines UsedDeclVisitor, a template class for visiting used
+//  declarations.
+//

"a CRTP class which visits all the declarations that are ODR-used by an 
expression or statement."



Comment at: clang/lib/Sema/UsedDeclVisitor.h:65
+  void VisitCXXDefaultArgExpr(CXXDefaultArgExpr *E) {
+this->Visit(E->getExpr());
+  }

It's generally best to `asImpl()` when restarting on a sub-expression like 
this, just in case the derived class wants to do something there.  Same thing 
in `VisitCXXBindTemporaryExpr`.


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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 242251.
yaxunl marked 5 inline comments as done.
yaxunl added a comment.

revised by John's comments.


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

https://reviews.llvm.org/D70172

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCUDA.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/UsedDeclVisitor.h
  clang/test/OpenMP/declare_target_messages.cpp
  clang/test/OpenMP/nvptx_target_exceptions_messages.cpp
  clang/test/SemaCUDA/bad-calls-on-same-line.cu
  clang/test/SemaCUDA/call-device-fn-from-host.cu
  clang/test/SemaCUDA/call-host-fn-from-device.cu
  clang/test/SemaCUDA/openmp-target.cu
  clang/test/SemaCUDA/trace-through-global.cu

Index: clang/test/SemaCUDA/trace-through-global.cu
===
--- clang/test/SemaCUDA/trace-through-global.cu
+++ clang/test/SemaCUDA/trace-through-global.cu
@@ -38,7 +38,7 @@
   // Notice that these two diagnostics are different: Because the call to hd1
   // is not dependent on T, the call to hd1 comes from 'launch_kernel', while
   // the call to hd3, being dependent, comes from 'launch_kernel'.
-  hd1(); // expected-note {{called by 'launch_kernel'}}
+  hd1(); // expected-note {{called by 'launch_kernel'}}
   hd3(T()); // expected-note {{called by 'launch_kernel'}}
 }
 
Index: clang/test/SemaCUDA/openmp-target.cu
===
--- clang/test/SemaCUDA/openmp-target.cu
+++ clang/test/SemaCUDA/openmp-target.cu
@@ -16,9 +16,9 @@
 void bazzz() {bazz();}
 #pragma omp declare target to(bazzz) device_type(nohost)
 void any() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
-void host1() {bazz();}
+void host1() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
 #pragma omp declare target to(host1) device_type(host)
-void host2() {bazz();}
+void host2() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
 #pragma omp declare target to(host2)
 void device() {host1();}
 #pragma omp declare target to(device) device_type(nohost)
Index: clang/test/SemaCUDA/call-host-fn-from-device.cu
===
--- clang/test/SemaCUDA/call-host-fn-from-device.cu
+++ clang/test/SemaCUDA/call-host-fn-from-device.cu
@@ -56,14 +56,14 @@
 }
 
 template  __host__ __device__ void hd2() { host_fn(); }
-// expected-error@-1 2 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 __global__ void kernel() { hd2(); }
 
 __host__ __device__ void hd() { host_fn(); }
 // expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 
 template  __host__ __device__ void hd3() { host_fn(); }
-// expected-error@-1 2 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 __device__ void device_fn() { hd3(); }
 
 // No error because this is never instantiated.
Index: clang/test/SemaCUDA/call-device-fn-from-host.cu
===
--- clang/test/SemaCUDA/call-device-fn-from-host.cu
+++ clang/test/SemaCUDA/call-device-fn-from-host.cu
@@ -1,7 +1,7 @@
 // RUN: %clang_cc1 %s --std=c++11 -triple x86_64-unknown-linux -emit-llvm -o - \
 // RUN:   -verify -verify-ignore-unexpected=note
 // RUN: %clang_cc1 %s --std=c++11 -triple x86_64-unknown-linux -emit-llvm -o - \
-// RUN:   -verify -verify-ignore-unexpected=note -fopenmp
+// RUN:   -verify=expected,omp -verify-ignore-unexpected=note -fopenmp
 
 // Note: This test won't work with -fsyntax-only, because some of these errors
 // are emitted during codegen.
@@ -39,7 +39,7 @@
 }
 
 template  __host__ __device__ void hd2() { device_fn(); }
-// expected-error@-1 2 {{reference to __device__ function 'device_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __device__ function 'device_fn' in __host__ __device__ function}}
 void host_fn() { hd2(); }
 
 __host__ __device__ void hd() { device_fn(); }
Index: clang/test/SemaCUDA/bad-calls-on-same-line.cu
===
--- clang/test/SemaCUDA/bad-calls-on-same-line.cu
+++ clang/test/SemaCUDA/bad-calls-on-same-line.cu
@@ -33,8 +33,8 @@
 
 void host_fn() {
   hd();
-  hd();  // expected-note {{function template specialization 'hd'}}
+  hd();
   // expected-note@-1 {{called by 'host_fn'}}
-  hd();  // expected-note {{function template specialization 'hd'}}
+  hd();
   // expected-note@-1 {{called by 'host_fn'}}
 }
Index: clang/test/OpenMP/nvptx_target_exceptions_messages.cpp

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:17127
+  template
+  class UsedDeclVisitor : public 
EvaluatedExprVisitor>{
+  protected:

This should inherit from `EvaluatedExprVisitor`, or else calls from 
`EvaluatedExprVisitor` and above won't dispatch all the way down to the 
subclass.  This will allow subclasses to do node-specific logic, like your 
subclass's handling of `InOMPDeviceContext` or `EvaluatedExprMarker`'s need to 
do custom things with local variables, DREs, and MEs.

Please also define this in a header; it doesn't need to be file-specific.  I 
guess it needs a `Sema &` because of the call to `LookupDestructor`, so 
`lib/Sema` is probably the right place for that header.



Comment at: clang/lib/Sema/SemaExpr.cpp:17152
+  asImpl().visitDeclRefExpr(E);
 }
 

Let's not have both a `visitDeclRefExpr` and a `VisitDeclRefExpr`, 
distinguished only by capitalization.



Comment at: clang/lib/Sema/SemaExpr.cpp:17158
+  visitUsedDecl(E->getBeginLoc(), const_cast(
+  E->getTemporary()->getDestructor()));
+  this->Visit(E->getSubExpr());

Please have all these call sites call `asImpl().visitUsedDecl` directly, and 
then don't define it in this class.



Comment at: clang/lib/Sema/SemaExpr.cpp:17195
+  --InOMPDeviceContext;
+}
+

This should be in your OMP-specific subclass.


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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 242145.
yaxunl added a comment.

revised by John's comments.


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

https://reviews.llvm.org/D70172

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCUDA.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/declare_target_messages.cpp
  clang/test/OpenMP/nvptx_target_exceptions_messages.cpp
  clang/test/SemaCUDA/bad-calls-on-same-line.cu
  clang/test/SemaCUDA/call-device-fn-from-host.cu
  clang/test/SemaCUDA/call-host-fn-from-device.cu
  clang/test/SemaCUDA/openmp-target.cu
  clang/test/SemaCUDA/trace-through-global.cu

Index: clang/test/SemaCUDA/trace-through-global.cu
===
--- clang/test/SemaCUDA/trace-through-global.cu
+++ clang/test/SemaCUDA/trace-through-global.cu
@@ -38,7 +38,7 @@
   // Notice that these two diagnostics are different: Because the call to hd1
   // is not dependent on T, the call to hd1 comes from 'launch_kernel', while
   // the call to hd3, being dependent, comes from 'launch_kernel'.
-  hd1(); // expected-note {{called by 'launch_kernel'}}
+  hd1(); // expected-note {{called by 'launch_kernel'}}
   hd3(T()); // expected-note {{called by 'launch_kernel'}}
 }
 
Index: clang/test/SemaCUDA/openmp-target.cu
===
--- clang/test/SemaCUDA/openmp-target.cu
+++ clang/test/SemaCUDA/openmp-target.cu
@@ -16,9 +16,9 @@
 void bazzz() {bazz();}
 #pragma omp declare target to(bazzz) device_type(nohost)
 void any() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
-void host1() {bazz();}
+void host1() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
 #pragma omp declare target to(host1) device_type(host)
-void host2() {bazz();}
+void host2() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
 #pragma omp declare target to(host2)
 void device() {host1();}
 #pragma omp declare target to(device) device_type(nohost)
Index: clang/test/SemaCUDA/call-host-fn-from-device.cu
===
--- clang/test/SemaCUDA/call-host-fn-from-device.cu
+++ clang/test/SemaCUDA/call-host-fn-from-device.cu
@@ -56,14 +56,14 @@
 }
 
 template  __host__ __device__ void hd2() { host_fn(); }
-// expected-error@-1 2 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 __global__ void kernel() { hd2(); }
 
 __host__ __device__ void hd() { host_fn(); }
 // expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 
 template  __host__ __device__ void hd3() { host_fn(); }
-// expected-error@-1 2 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 __device__ void device_fn() { hd3(); }
 
 // No error because this is never instantiated.
Index: clang/test/SemaCUDA/call-device-fn-from-host.cu
===
--- clang/test/SemaCUDA/call-device-fn-from-host.cu
+++ clang/test/SemaCUDA/call-device-fn-from-host.cu
@@ -1,7 +1,7 @@
 // RUN: %clang_cc1 %s --std=c++11 -triple x86_64-unknown-linux -emit-llvm -o - \
 // RUN:   -verify -verify-ignore-unexpected=note
 // RUN: %clang_cc1 %s --std=c++11 -triple x86_64-unknown-linux -emit-llvm -o - \
-// RUN:   -verify -verify-ignore-unexpected=note -fopenmp
+// RUN:   -verify=expected,omp -verify-ignore-unexpected=note -fopenmp
 
 // Note: This test won't work with -fsyntax-only, because some of these errors
 // are emitted during codegen.
@@ -39,7 +39,7 @@
 }
 
 template  __host__ __device__ void hd2() { device_fn(); }
-// expected-error@-1 2 {{reference to __device__ function 'device_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __device__ function 'device_fn' in __host__ __device__ function}}
 void host_fn() { hd2(); }
 
 __host__ __device__ void hd() { device_fn(); }
Index: clang/test/SemaCUDA/bad-calls-on-same-line.cu
===
--- clang/test/SemaCUDA/bad-calls-on-same-line.cu
+++ clang/test/SemaCUDA/bad-calls-on-same-line.cu
@@ -33,8 +33,8 @@
 
 void host_fn() {
   hd();
-  hd();  // expected-note {{function template specialization 'hd'}}
+  hd();
   // expected-note@-1 {{called by 'host_fn'}}
-  hd();  // expected-note {{function template specialization 'hd'}}
+  hd();
   // expected-note@-1 {{called by 'host_fn'}}
 }
Index: clang/test/OpenMP/nvptx_target_exceptions_messages.cpp
===
--- 

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:17183
+  class DeferredDiagnosticsEmitter
+  : public EvaluatedExprVisitor {
+Sema 

Is there any way to share most of the visitation logic here with the visitor we 
use in `MarkDeclarationsUsedInExpr`?  Maybe make a `UsedDeclVisitor` CRTP class 
that calls a "asImpl().visitUsedDecl(SourceLocation Loc, Decl *D)" in the right 
places?


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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D70172#1812749 , @rjmccall wrote:

> In D70172#1812664 , @yaxunl wrote:
>
> > In D70172#1812631 , @rjmccall 
> > wrote:
> >
> > > Most uses of the destructor do not use the delete operator, though, and 
> > > therefore should not trigger the diagnostics in `f` to be emitted.  And 
> > > this really doesn't require a fully-realized use graph; you could very 
> > > easily track the current use stack when making a later pass over the 
> > > entities used.
> >
> >
> > The call graph is not for this specific situation. A call graph is needed 
> > because of the transitive nature of the deferred diagnostic message. That 
> > is, if any direct or indirect caller is emitted, the diagnostic msg needs 
> > to be emitted.
>
>
> One of the points that Richard and I have been trying to make is that this 
> really isn't specifically about *calls*, it's about *uses*.  You only want to 
> emit diagnostics associated with an entity if you actually have to emit that 
> entity, and whether you emit an entity has nothing to do with what places 
> might *call* it, but rather what places *use* it and therefore force it to be 
> emitted.  This is fortunate because call graphs are inherently imperfect 
> because of indirect calls, but use graphs are totally reliable.  It's also 
> fortunate because it means you can piggy-back on all of the existing logic 
> that Sema has for tracking ODR uses.
>
> Richard and I are also pointing out that Sema has to treat the v-table as its 
> own separate thing when tracking ODR uses, and so you need to as well.  You 
> want to emit diagnostics associated with a virtual function if you're 
> emitting code that either (1) directly uses the function (e.g. by calling 
> `x->A::foo()`) or (2) directly uses a v-table containing the function.  You 
> can't rely on Sema's normal ODR-use tracking for *either* of these, because 
> Sema might have observed a use in code that you don't actually have to emit, 
> e.g. host code if you're compiling for the device.  That is, a v-table is 
> only a "root" for virtual functions if you actually have to emit that 
> v-table, and you can't know that without tracking v-tables in your use graph.
>
> > The deferred diagnostic msg is recorded when parsing a function body. At 
> > that time we do not know which function will directly or indirectly call 
> > it. How do we keep a use stack?
>
> The "use stack" idea would apply if you switched from eagerly creating the 
> entire use graph to instead just making a late pass that walked function 
> bodies.  If you walk function bodies depth-first, starting from a true root 
> and gathering all the ODR-used entities to be  recursively walked, then you 
> can maintain a stack of what entities you're currently walking, and that 
> stack is a use-path that explains why you need to emit the current function.
>
> It should be straightforward to build a function that walks over the entities 
> used by a function body and calls a callback by just extracting it out of the 
> code in `MarkDeclarationsUsedInExpr`.


I updated the patch to remove the explicit call graph and use an AST traverse 
instead. Since this patch is big, is it OK to leave the tracking of vtable to 
some future patch? This patch is sufficient to fix the assertion seen on 
Windows. Thanks.


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

https://reviews.llvm.org/D70172



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

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 240760.
yaxunl retitled this revision from "[CUDA][HIP] Fix assertion due to dtor check 
on windows" to "[CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing 
AST travese".
yaxunl edited the summary of this revision.
yaxunl added a comment.
Herald added a subscriber: guansong.
Herald added a reviewer: jdoerfert.

Remove the call graph and do a final AST traverse by John's comments.


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

https://reviews.llvm.org/D70172

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCUDA.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/declare_target_messages.cpp
  clang/test/OpenMP/nvptx_target_exceptions_messages.cpp
  clang/test/SemaCUDA/bad-calls-on-same-line.cu
  clang/test/SemaCUDA/call-device-fn-from-host.cu
  clang/test/SemaCUDA/call-host-fn-from-device.cu
  clang/test/SemaCUDA/openmp-target.cu
  clang/test/SemaCUDA/trace-through-global.cu

Index: clang/test/SemaCUDA/trace-through-global.cu
===
--- clang/test/SemaCUDA/trace-through-global.cu
+++ clang/test/SemaCUDA/trace-through-global.cu
@@ -38,7 +38,7 @@
   // Notice that these two diagnostics are different: Because the call to hd1
   // is not dependent on T, the call to hd1 comes from 'launch_kernel', while
   // the call to hd3, being dependent, comes from 'launch_kernel'.
-  hd1(); // expected-note {{called by 'launch_kernel'}}
+  hd1(); // expected-note {{called by 'launch_kernel'}}
   hd3(T()); // expected-note {{called by 'launch_kernel'}}
 }
 
Index: clang/test/SemaCUDA/openmp-target.cu
===
--- clang/test/SemaCUDA/openmp-target.cu
+++ clang/test/SemaCUDA/openmp-target.cu
@@ -16,9 +16,9 @@
 void bazzz() {bazz();}
 #pragma omp declare target to(bazzz) device_type(nohost)
 void any() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
-void host1() {bazz();}
+void host1() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
 #pragma omp declare target to(host1) device_type(host)
-void host2() {bazz();}
+void host2() {bazz();} // expected-error {{function with 'device_type(nohost)' is not available on host}}
 #pragma omp declare target to(host2)
 void device() {host1();}
 #pragma omp declare target to(device) device_type(nohost)
Index: clang/test/SemaCUDA/call-host-fn-from-device.cu
===
--- clang/test/SemaCUDA/call-host-fn-from-device.cu
+++ clang/test/SemaCUDA/call-host-fn-from-device.cu
@@ -56,14 +56,14 @@
 }
 
 template  __host__ __device__ void hd2() { host_fn(); }
-// expected-error@-1 2 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 __global__ void kernel() { hd2(); }
 
 __host__ __device__ void hd() { host_fn(); }
 // expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 
 template  __host__ __device__ void hd3() { host_fn(); }
-// expected-error@-1 2 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __host__ function 'host_fn' in __host__ __device__ function}}
 __device__ void device_fn() { hd3(); }
 
 // No error because this is never instantiated.
Index: clang/test/SemaCUDA/call-device-fn-from-host.cu
===
--- clang/test/SemaCUDA/call-device-fn-from-host.cu
+++ clang/test/SemaCUDA/call-device-fn-from-host.cu
@@ -1,7 +1,7 @@
 // RUN: %clang_cc1 %s --std=c++11 -triple x86_64-unknown-linux -emit-llvm -o - \
 // RUN:   -verify -verify-ignore-unexpected=note
 // RUN: %clang_cc1 %s --std=c++11 -triple x86_64-unknown-linux -emit-llvm -o - \
-// RUN:   -verify -verify-ignore-unexpected=note -fopenmp
+// RUN:   -verify=expected,omp -verify-ignore-unexpected=note -fopenmp
 
 // Note: This test won't work with -fsyntax-only, because some of these errors
 // are emitted during codegen.
@@ -39,7 +39,7 @@
 }
 
 template  __host__ __device__ void hd2() { device_fn(); }
-// expected-error@-1 2 {{reference to __device__ function 'device_fn' in __host__ __device__ function}}
+// expected-error@-1 {{reference to __device__ function 'device_fn' in __host__ __device__ function}}
 void host_fn() { hd2(); }
 
 __host__ __device__ void hd() { device_fn(); }
Index: clang/test/SemaCUDA/bad-calls-on-same-line.cu
===
--- clang/test/SemaCUDA/bad-calls-on-same-line.cu
+++ clang/test/SemaCUDA/bad-calls-on-same-line.cu
@@ -33,8 +33,8 @@
 
 void host_fn() {
   hd();
-  hd();  // expected-note {{function template specialization 'hd'}}
+  hd();