[PATCH] D77028: [NFC] Refactor DeferredDiagsEmitter and skip redundant visit

2020-04-05 Thread Yaxun Liu via Phabricator via cfe-commits
yaxunl marked 3 inline comments as done.
yaxunl added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1508
   void checkFunc(SourceLocation Loc, FunctionDecl *FD) {
+auto DiagsCountIt = DiagsCount.find(FD);
 FunctionDecl *Caller = UseStack.empty() ? nullptr : UseStack.back();

rjmccall wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > yaxunl wrote:
> > > > rjmccall wrote:
> > > > > yaxunl wrote:
> > > > > > rjmccall wrote:
> > > > > > > yaxunl wrote:
> > > > > > > > rjmccall wrote:
> > > > > > > > > It makes me a little uncomfortable to be holding an iterator 
> > > > > > > > > this long while calling a fair amount of other stuff in the 
> > > > > > > > > meantime.
> > > > > > > > > 
> > > > > > > > > Your use of DiagsCount is subtle enough that it really needs 
> > > > > > > > > to be explained in some comments.  You're doing stuff 
> > > > > > > > > conditionally based on both whether the entry exists but also 
> > > > > > > > > whether it's non-zero.
> > > > > > > > added comments
> > > > > > > Okay, thanks for that.  Two points, then.  First, it looks like 
> > > > > > > the count is really just a boolean for whether the function 
> > > > > > > recursively triggers any diagnostics.   And second, can't it just 
> > > > > > > be as simple as whether we've visited that function at all in a 
> > > > > > > context that's forcing diagnostics to be emitted?  The logic 
> > > > > > > seems to be to try to emit the diagnostics for each use-path, but 
> > > > > > > why would we want that?
> > > > > > For the second comment, we need to visit a function again for each 
> > > > > > use-path because we need to report each use-path that triggers a 
> > > > > > diagnostic, otherwise users will see a new error after they fix one 
> > > > > > error, instead of seeing all the errors at once.
> > > > > > 
> > > > > > For the first comment, I will change the count to two flags: one 
> > > > > > for the case where the function is not in device context, the other 
> > > > > > is for the case where the function is in device context. This will 
> > > > > > allow us to avoid redundant visits whether or not we are in a 
> > > > > > device context.
> > > > > > For the second comment, we need to visit a function again for each 
> > > > > > use-path because we need to report each use-path that triggers a 
> > > > > > diagnostic, otherwise users will see a new error after they fix one 
> > > > > > error, instead of seeing all the errors at once.
> > > > > 
> > > > > This is not what we do in analogous cases where errors are triggered 
> > > > > by a use, like in template instantiation.   The bug might be that the 
> > > > > device program is using a function that it shouldn't be using, or the 
> > > > > bug might be that a function that's supposed  to be usable from the 
> > > > > device is written incorrectly.  In the former case, yes, not 
> > > > > reporting the errors for each use-path may force the programmer to 
> > > > > build multiple times to find all the problematic uses.  However, in 
> > > > > the latter case you can easily end up emitting a massive number of 
> > > > > errors that completely drowns out everything else.  It's also 
> > > > > non-linear: the number of different use-paths of a particular 
> > > > > function can be combinatoric.
> > > > The deferred diagnostics fall into the first case mostly. 
> > > > 
> > > > Not all diagnostic messages happen in device host functions are 
> > > > deferred. Most of diagnostic messages are emitted immediately, 
> > > > disregarding whether the function is emitted or not. 
> > > > 
> > > > Only a few special types of diagnostic messages e.g. inline assembly 
> > > > errors, exceptions, varags are deferred. This is because clang has to 
> > > > use pragmas to make all functions device and host in some system 
> > > > headers to be able to use them in device compilation, whereas some of 
> > > > these functions will cause error only if they are emitted in device 
> > > > compilation. Since we cannot change these headers, we have to defer 
> > > > such diagnostics to the point where they are actually triggered. 
> > > > Therefore we are mostly interested in "who is calling these functions" 
> > > > instead of the diagnostics themselves.
> > > > 
> > > > For normal programs containing no diagnostics, the current approach 
> > > > should sufficiently avoid all redundant visits and all functions will 
> > > > be visited once.
> > > > 
> > > > The functions may be visited multiple times when there are deferred 
> > > > diagnostics, however this should be limited by the maximum number of 
> > > > errors.
> > > Okay.  I understand the point now about being mostly concerned about 
> > > using functions in headers.
> > > 
> > > My concern about visiting things a combinatorial number of times is less 
> > > about generating an excessive number of errors and more about taking an 
> > > excessive amount of time to finish compilation.  The 

[PATCH] D77028: [NFC] Refactor DeferredDiagsEmitter and skip redundant visit

2020-04-05 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1508
   void checkFunc(SourceLocation Loc, FunctionDecl *FD) {
+auto DiagsCountIt = DiagsCount.find(FD);
 FunctionDecl *Caller = UseStack.empty() ? nullptr : UseStack.back();

yaxunl wrote:
> rjmccall wrote:
> > yaxunl wrote:
> > > rjmccall wrote:
> > > > yaxunl wrote:
> > > > > rjmccall wrote:
> > > > > > yaxunl wrote:
> > > > > > > rjmccall wrote:
> > > > > > > > It makes me a little uncomfortable to be holding an iterator 
> > > > > > > > this long while calling a fair amount of other stuff in the 
> > > > > > > > meantime.
> > > > > > > > 
> > > > > > > > Your use of DiagsCount is subtle enough that it really needs to 
> > > > > > > > be explained in some comments.  You're doing stuff 
> > > > > > > > conditionally based on both whether the entry exists but also 
> > > > > > > > whether it's non-zero.
> > > > > > > added comments
> > > > > > Okay, thanks for that.  Two points, then.  First, it looks like the 
> > > > > > count is really just a boolean for whether the function recursively 
> > > > > > triggers any diagnostics.   And second, can't it just be as simple 
> > > > > > as whether we've visited that function at all in a context that's 
> > > > > > forcing diagnostics to be emitted?  The logic seems to be to try to 
> > > > > > emit the diagnostics for each use-path, but why would we want that?
> > > > > For the second comment, we need to visit a function again for each 
> > > > > use-path because we need to report each use-path that triggers a 
> > > > > diagnostic, otherwise users will see a new error after they fix one 
> > > > > error, instead of seeing all the errors at once.
> > > > > 
> > > > > For the first comment, I will change the count to two flags: one for 
> > > > > the case where the function is not in device context, the other is 
> > > > > for the case where the function is in device context. This will allow 
> > > > > us to avoid redundant visits whether or not we are in a device 
> > > > > context.
> > > > > For the second comment, we need to visit a function again for each 
> > > > > use-path because we need to report each use-path that triggers a 
> > > > > diagnostic, otherwise users will see a new error after they fix one 
> > > > > error, instead of seeing all the errors at once.
> > > > 
> > > > This is not what we do in analogous cases where errors are triggered by 
> > > > a use, like in template instantiation.   The bug might be that the 
> > > > device program is using a function that it shouldn't be using, or the 
> > > > bug might be that a function that's supposed  to be usable from the 
> > > > device is written incorrectly.  In the former case, yes, not reporting 
> > > > the errors for each use-path may force the programmer to build multiple 
> > > > times to find all the problematic uses.  However, in the latter case 
> > > > you can easily end up emitting a massive number of errors that 
> > > > completely drowns out everything else.  It's also non-linear: the 
> > > > number of different use-paths of a particular function can be 
> > > > combinatoric.
> > > The deferred diagnostics fall into the first case mostly. 
> > > 
> > > Not all diagnostic messages happen in device host functions are deferred. 
> > > Most of diagnostic messages are emitted immediately, disregarding whether 
> > > the function is emitted or not. 
> > > 
> > > Only a few special types of diagnostic messages e.g. inline assembly 
> > > errors, exceptions, varags are deferred. This is because clang has to use 
> > > pragmas to make all functions device and host in some system headers to 
> > > be able to use them in device compilation, whereas some of these 
> > > functions will cause error only if they are emitted in device 
> > > compilation. Since we cannot change these headers, we have to defer such 
> > > diagnostics to the point where they are actually triggered. Therefore we 
> > > are mostly interested in "who is calling these functions" instead of the 
> > > diagnostics themselves.
> > > 
> > > For normal programs containing no diagnostics, the current approach 
> > > should sufficiently avoid all redundant visits and all functions will be 
> > > visited once.
> > > 
> > > The functions may be visited multiple times when there are deferred 
> > > diagnostics, however this should be limited by the maximum number of 
> > > errors.
> > Okay.  I understand the point now about being mostly concerned about using 
> > functions in headers.
> > 
> > My concern about visiting things a combinatorial number of times is less 
> > about generating an excessive number of errors and more about taking an 
> > excessive amount of time to finish compilation.  The compiler really should 
> > not be using any exponential algorithms; a simple visited set will ensure 
> > that, but I think what you're doing does not.  Can you make a case for why 
> > that's not true?
> The current approach 

[PATCH] D77028: [NFC] Refactor DeferredDiagsEmitter and skip redundant visit

2020-04-05 Thread Yaxun Liu via Phabricator via cfe-commits
yaxunl marked 3 inline comments as done.
yaxunl added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1508
   void checkFunc(SourceLocation Loc, FunctionDecl *FD) {
+auto DiagsCountIt = DiagsCount.find(FD);
 FunctionDecl *Caller = UseStack.empty() ? nullptr : UseStack.back();

rjmccall wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > yaxunl wrote:
> > > > rjmccall wrote:
> > > > > yaxunl wrote:
> > > > > > rjmccall wrote:
> > > > > > > It makes me a little uncomfortable to be holding an iterator this 
> > > > > > > long while calling a fair amount of other stuff in the meantime.
> > > > > > > 
> > > > > > > Your use of DiagsCount is subtle enough that it really needs to 
> > > > > > > be explained in some comments.  You're doing stuff conditionally 
> > > > > > > based on both whether the entry exists but also whether it's 
> > > > > > > non-zero.
> > > > > > added comments
> > > > > Okay, thanks for that.  Two points, then.  First, it looks like the 
> > > > > count is really just a boolean for whether the function recursively 
> > > > > triggers any diagnostics.   And second, can't it just be as simple as 
> > > > > whether we've visited that function at all in a context that's 
> > > > > forcing diagnostics to be emitted?  The logic seems to be to try to 
> > > > > emit the diagnostics for each use-path, but why would we want that?
> > > > For the second comment, we need to visit a function again for each 
> > > > use-path because we need to report each use-path that triggers a 
> > > > diagnostic, otherwise users will see a new error after they fix one 
> > > > error, instead of seeing all the errors at once.
> > > > 
> > > > For the first comment, I will change the count to two flags: one for 
> > > > the case where the function is not in device context, the other is for 
> > > > the case where the function is in device context. This will allow us to 
> > > > avoid redundant visits whether or not we are in a device context.
> > > > For the second comment, we need to visit a function again for each 
> > > > use-path because we need to report each use-path that triggers a 
> > > > diagnostic, otherwise users will see a new error after they fix one 
> > > > error, instead of seeing all the errors at once.
> > > 
> > > This is not what we do in analogous cases where errors are triggered by a 
> > > use, like in template instantiation.   The bug might be that the device 
> > > program is using a function that it shouldn't be using, or the bug might 
> > > be that a function that's supposed  to be usable from the device is 
> > > written incorrectly.  In the former case, yes, not reporting the errors 
> > > for each use-path may force the programmer to build multiple times to 
> > > find all the problematic uses.  However, in the latter case you can 
> > > easily end up emitting a massive number of errors that completely drowns 
> > > out everything else.  It's also non-linear: the number of different 
> > > use-paths of a particular function can be combinatoric.
> > The deferred diagnostics fall into the first case mostly. 
> > 
> > Not all diagnostic messages happen in device host functions are deferred. 
> > Most of diagnostic messages are emitted immediately, disregarding whether 
> > the function is emitted or not. 
> > 
> > Only a few special types of diagnostic messages e.g. inline assembly 
> > errors, exceptions, varags are deferred. This is because clang has to use 
> > pragmas to make all functions device and host in some system headers to be 
> > able to use them in device compilation, whereas some of these functions 
> > will cause error only if they are emitted in device compilation. Since we 
> > cannot change these headers, we have to defer such diagnostics to the point 
> > where they are actually triggered. Therefore we are mostly interested in 
> > "who is calling these functions" instead of the diagnostics themselves.
> > 
> > For normal programs containing no diagnostics, the current approach should 
> > sufficiently avoid all redundant visits and all functions will be visited 
> > once.
> > 
> > The functions may be visited multiple times when there are deferred 
> > diagnostics, however this should be limited by the maximum number of errors.
> Okay.  I understand the point now about being mostly concerned about using 
> functions in headers.
> 
> My concern about visiting things a combinatorial number of times is less 
> about generating an excessive number of errors and more about taking an 
> excessive amount of time to finish compilation.  The compiler really should 
> not be using any exponential algorithms; a simple visited set will ensure 
> that, but I think what you're doing does not.  Can you make a case for why 
> that's not true?
The current approach is linear for the number of visits to functions.

Let's assume the number of functions is N.

The current approach already restricted revisiting a node that's already in the 

[PATCH] D77028: [NFC] Refactor DeferredDiagsEmitter and skip redundant visit

2020-04-03 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1508
   void checkFunc(SourceLocation Loc, FunctionDecl *FD) {
+auto DiagsCountIt = DiagsCount.find(FD);
 FunctionDecl *Caller = UseStack.empty() ? nullptr : UseStack.back();

yaxunl wrote:
> rjmccall wrote:
> > yaxunl wrote:
> > > rjmccall wrote:
> > > > yaxunl wrote:
> > > > > rjmccall wrote:
> > > > > > It makes me a little uncomfortable to be holding an iterator this 
> > > > > > long while calling a fair amount of other stuff in the meantime.
> > > > > > 
> > > > > > Your use of DiagsCount is subtle enough that it really needs to be 
> > > > > > explained in some comments.  You're doing stuff conditionally based 
> > > > > > on both whether the entry exists but also whether it's non-zero.
> > > > > added comments
> > > > Okay, thanks for that.  Two points, then.  First, it looks like the 
> > > > count is really just a boolean for whether the function recursively 
> > > > triggers any diagnostics.   And second, can't it just be as simple as 
> > > > whether we've visited that function at all in a context that's forcing 
> > > > diagnostics to be emitted?  The logic seems to be to try to emit the 
> > > > diagnostics for each use-path, but why would we want that?
> > > For the second comment, we need to visit a function again for each 
> > > use-path because we need to report each use-path that triggers a 
> > > diagnostic, otherwise users will see a new error after they fix one 
> > > error, instead of seeing all the errors at once.
> > > 
> > > For the first comment, I will change the count to two flags: one for the 
> > > case where the function is not in device context, the other is for the 
> > > case where the function is in device context. This will allow us to avoid 
> > > redundant visits whether or not we are in a device context.
> > > For the second comment, we need to visit a function again for each 
> > > use-path because we need to report each use-path that triggers a 
> > > diagnostic, otherwise users will see a new error after they fix one 
> > > error, instead of seeing all the errors at once.
> > 
> > This is not what we do in analogous cases where errors are triggered by a 
> > use, like in template instantiation.   The bug might be that the device 
> > program is using a function that it shouldn't be using, or the bug might be 
> > that a function that's supposed  to be usable from the device is written 
> > incorrectly.  In the former case, yes, not reporting the errors for each 
> > use-path may force the programmer to build multiple times to find all the 
> > problematic uses.  However, in the latter case you can easily end up 
> > emitting a massive number of errors that completely drowns out everything 
> > else.  It's also non-linear: the number of different use-paths of a 
> > particular function can be combinatoric.
> The deferred diagnostics fall into the first case mostly. 
> 
> Not all diagnostic messages happen in device host functions are deferred. 
> Most of diagnostic messages are emitted immediately, disregarding whether the 
> function is emitted or not. 
> 
> Only a few special types of diagnostic messages e.g. inline assembly errors, 
> exceptions, varags are deferred. This is because clang has to use pragmas to 
> make all functions device and host in some system headers to be able to use 
> them in device compilation, whereas some of these functions will cause error 
> only if they are emitted in device compilation. Since we cannot change these 
> headers, we have to defer such diagnostics to the point where they are 
> actually triggered. Therefore we are mostly interested in "who is calling 
> these functions" instead of the diagnostics themselves.
> 
> For normal programs containing no diagnostics, the current approach should 
> sufficiently avoid all redundant visits and all functions will be visited 
> once.
> 
> The functions may be visited multiple times when there are deferred 
> diagnostics, however this should be limited by the maximum number of errors.
Okay.  I understand the point now about being mostly concerned about using 
functions in headers.

My concern about visiting things a combinatorial number of times is less about 
generating an excessive number of errors and more about taking an excessive 
amount of time to finish compilation.  The compiler really should not be using 
any exponential algorithms; a simple visited set will ensure that, but I think 
what you're doing does not.  Can you make a case for why that's not true?


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

https://reviews.llvm.org/D77028



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


[PATCH] D77028: [NFC] Refactor DeferredDiagsEmitter and skip redundant visit

2020-04-03 Thread 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:1508
   void checkFunc(SourceLocation Loc, FunctionDecl *FD) {
+auto DiagsCountIt = DiagsCount.find(FD);
 FunctionDecl *Caller = UseStack.empty() ? nullptr : UseStack.back();

rjmccall wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > yaxunl wrote:
> > > > rjmccall wrote:
> > > > > It makes me a little uncomfortable to be holding an iterator this 
> > > > > long while calling a fair amount of other stuff in the meantime.
> > > > > 
> > > > > Your use of DiagsCount is subtle enough that it really needs to be 
> > > > > explained in some comments.  You're doing stuff conditionally based 
> > > > > on both whether the entry exists but also whether it's non-zero.
> > > > added comments
> > > Okay, thanks for that.  Two points, then.  First, it looks like the count 
> > > is really just a boolean for whether the function recursively triggers 
> > > any diagnostics.   And second, can't it just be as simple as whether 
> > > we've visited that function at all in a context that's forcing 
> > > diagnostics to be emitted?  The logic seems to be to try to emit the 
> > > diagnostics for each use-path, but why would we want that?
> > For the second comment, we need to visit a function again for each use-path 
> > because we need to report each use-path that triggers a diagnostic, 
> > otherwise users will see a new error after they fix one error, instead of 
> > seeing all the errors at once.
> > 
> > For the first comment, I will change the count to two flags: one for the 
> > case where the function is not in device context, the other is for the case 
> > where the function is in device context. This will allow us to avoid 
> > redundant visits whether or not we are in a device context.
> > For the second comment, we need to visit a function again for each use-path 
> > because we need to report each use-path that triggers a diagnostic, 
> > otherwise users will see a new error after they fix one error, instead of 
> > seeing all the errors at once.
> 
> This is not what we do in analogous cases where errors are triggered by a 
> use, like in template instantiation.   The bug might be that the device 
> program is using a function that it shouldn't be using, or the bug might be 
> that a function that's supposed  to be usable from the device is written 
> incorrectly.  In the former case, yes, not reporting the errors for each 
> use-path may force the programmer to build multiple times to find all the 
> problematic uses.  However, in the latter case you can easily end up emitting 
> a massive number of errors that completely drowns out everything else.  It's 
> also non-linear: the number of different use-paths of a particular function 
> can be combinatoric.
The deferred diagnostics fall into the first case mostly. 

Not all diagnostic messages happen in device host functions are deferred. Most 
of diagnostic messages are emitted immediately, disregarding whether the 
function is emitted or not. 

Only a few special types of diagnostic messages e.g. inline assembly errors, 
exceptions, varags are deferred. This is because clang has to use pragmas to 
make all functions device and host in some system headers to be able to use 
them in device compilation, whereas some of these functions will cause error 
only if they are emitted in device compilation. Since we cannot change these 
headers, we have to defer such diagnostics to the point where they are actually 
triggered. Therefore we are mostly interested in "who is calling these 
functions" instead of the diagnostics themselves.

For normal programs containing no diagnostics, the current approach should 
sufficiently avoid all redundant visits and all functions will be visited once.

The functions may be visited multiple times when there are deferred 
diagnostics, however this should be limited by the maximum number of errors.


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

https://reviews.llvm.org/D77028



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


[PATCH] D77028: [NFC] Refactor DeferredDiagsEmitter and skip redundant visit

2020-04-03 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1508
   void checkFunc(SourceLocation Loc, FunctionDecl *FD) {
+auto DiagsCountIt = DiagsCount.find(FD);
 FunctionDecl *Caller = UseStack.empty() ? nullptr : UseStack.back();

yaxunl wrote:
> rjmccall wrote:
> > yaxunl wrote:
> > > rjmccall wrote:
> > > > It makes me a little uncomfortable to be holding an iterator this long 
> > > > while calling a fair amount of other stuff in the meantime.
> > > > 
> > > > Your use of DiagsCount is subtle enough that it really needs to be 
> > > > explained in some comments.  You're doing stuff conditionally based on 
> > > > both whether the entry exists but also whether it's non-zero.
> > > added comments
> > Okay, thanks for that.  Two points, then.  First, it looks like the count 
> > is really just a boolean for whether the function recursively triggers any 
> > diagnostics.   And second, can't it just be as simple as whether we've 
> > visited that function at all in a context that's forcing diagnostics to be 
> > emitted?  The logic seems to be to try to emit the diagnostics for each 
> > use-path, but why would we want that?
> For the second comment, we need to visit a function again for each use-path 
> because we need to report each use-path that triggers a diagnostic, otherwise 
> users will see a new error after they fix one error, instead of seeing all 
> the errors at once.
> 
> For the first comment, I will change the count to two flags: one for the case 
> where the function is not in device context, the other is for the case where 
> the function is in device context. This will allow us to avoid redundant 
> visits whether or not we are in a device context.
> For the second comment, we need to visit a function again for each use-path 
> because we need to report each use-path that triggers a diagnostic, otherwise 
> users will see a new error after they fix one error, instead of seeing all 
> the errors at once.

This is not what we do in analogous cases where errors are triggered by a use, 
like in template instantiation.   The bug might be that the device program is 
using a function that it shouldn't be using, or the bug might be that a 
function that's supposed  to be usable from the device is written incorrectly.  
In the former case, yes, not reporting the errors for each use-path may force 
the programmer to build multiple times to find all the problematic uses.  
However, in the latter case you can easily end up emitting a massive number of 
errors that completely drowns out everything else.  It's also non-linear: the 
number of different use-paths of a particular function can be combinatoric.


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

https://reviews.llvm.org/D77028



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


[PATCH] D77028: [NFC] Refactor DeferredDiagsEmitter and skip redundant visit

2020-04-03 Thread Yaxun Liu via Phabricator via cfe-commits
yaxunl updated this revision to Diff 254840.
yaxunl marked 2 inline comments as done.
yaxunl added a comment.

revised by John's comments


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

https://reviews.llvm.org/D77028

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp

Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -1436,63 +1436,87 @@
 }
 
 // Print notes showing how we can reach FD starting from an a priori
-// known-callable function.
-static void emitCallStackNotes(Sema , FunctionDecl *FD) {
+// known-callable function. If \HasDiags is not null pointer, the flag
+// about whether diagnostics emitted for a function is set.
+static void emitCallStackNotes(
+Sema , FunctionDecl *FD,
+llvm::DenseMap, bool> *HasDiags = nullptr) {
   auto FnIt = S.DeviceKnownEmittedFns.find(FD);
   while (FnIt != S.DeviceKnownEmittedFns.end()) {
 DiagnosticBuilder Builder(
 S.Diags.Report(FnIt->second.Loc, diag::note_called_by));
 Builder << FnIt->second.FD;
 Builder.setForceEmit();
+// Set the flag about whether deferred diagnostics directly or indirectly
+// triggered by a function.
+if (HasDiags)
+  (*HasDiags)[FnIt->second.FD] = true;
 
 FnIt = S.DeviceKnownEmittedFns.find(FnIt->second.FD);
   }
 }
 
-// Emit any deferred diagnostics for FD and erase them from the map in which
-// they're stored.
-void Sema::emitDeferredDiags(FunctionDecl *FD, bool ShowCallStack) {
-  auto It = DeviceDeferredDiags.find(FD);
-  if (It == DeviceDeferredDiags.end())
-return;
-  bool HasWarningOrError = false;
-  bool FirstDiag = true;
-  for (PartialDiagnosticAt  : It->second) {
-const SourceLocation  = PDAt.first;
-const PartialDiagnostic  = PDAt.second;
-HasWarningOrError |= getDiagnostics().getDiagnosticLevel(
- PD.getDiagID(), Loc) >= DiagnosticsEngine::Warning;
-{
-  DiagnosticBuilder Builder(Diags.Report(Loc, PD.getDiagID()));
-  Builder.setForceEmit();
-  PD.Emit(Builder);
-}
-
-// Emit the note on the first diagnostic in case too many diagnostics cause
-// the note not emitted.
-if (FirstDiag && HasWarningOrError && ShowCallStack) {
-  emitCallStackNotes(*this, FD);
-  FirstDiag = false;
-}
-  }
-
-}
-
 namespace {
+
 /// Helper class that emits deferred diagnostic messages if an entity directly
 /// or indirectly using the function that causes the deferred diagnostic
 /// messages is known to be emitted.
+///
+/// During parsing of AST, certain diagnostic messages are recorded as deferred
+/// diagnostics since it is unknown whether the functions containing such
+/// diagnostics will be emitted. A list of potentially emitted functions and
+/// variables that may potentially trigger emission of functions are also
+/// recorded. DeferredDiagnosticsEmitter recursively visits used functions
+/// by each function to emit deferred diagnostics.
+///
+/// During the visit, certain OpenMP directives or initializer of variables
+/// with certain OpenMP attributes will cause subsequent visiting of any
+/// functions enter a state which is called OpenMP device context in this
+/// implementation. The state is exited when the directive or initializer is
+/// exited. This state can change the emission states of subsequent uses
+/// of functions.
+///
+/// Conceptually the functions or variables to be visited form a use graph
+/// where the parent node uses the child node. At any point of the visit,
+/// the tree nodes traversed from the tree root to the current node form a use
+/// stack. The emission state of the current node depends on two factors:
+///1. the emission state of the root node
+///2. whether the current node is in OpenMP device context
+/// If the function is decided to be emitted, its contained deferred diagnostics
+/// are emitted, together with the information about the use stack.
+///
+/// The flag about whether deferred diagnostics directly or indirectly triggered
+/// by a function is added. A function is visited at least once to add the flag
+/// about whether it triggers diagnostics. For subsequent visits, if the flag
+/// for deferred diagnostics triggered by the function is false, the function
+/// is skipped, since the subtree starting at this node does not trigger any
+/// deferred diagnostics and does not trigger any OpenMP device context,
+/// otherwise the flag cannot be false.
+///
 class DeferredDiagnosticsEmitter
 : public UsedDeclVisitor {
 public:
   typedef UsedDeclVisitor Inherited;
   llvm::SmallSet, 4> Visited;
   llvm::SmallVector, 4> UseStack;
-  bool ShouldEmit;
+
+  // Whether deferred diagnostics are triggered directly or indirectly by
+  // a declaration. HasDiags[0] is for the case not in OpenMP device context.
+  // HasDiags[1] is for the case in OpenMP device context. We need two maps
+  // because 

[PATCH] D77028: [NFC] Refactor DeferredDiagsEmitter and skip redundant visit

2020-04-03 Thread 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:1508
   void checkFunc(SourceLocation Loc, FunctionDecl *FD) {
+auto DiagsCountIt = DiagsCount.find(FD);
 FunctionDecl *Caller = UseStack.empty() ? nullptr : UseStack.back();

rjmccall wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > It makes me a little uncomfortable to be holding an iterator this long 
> > > while calling a fair amount of other stuff in the meantime.
> > > 
> > > Your use of DiagsCount is subtle enough that it really needs to be 
> > > explained in some comments.  You're doing stuff conditionally based on 
> > > both whether the entry exists but also whether it's non-zero.
> > added comments
> Okay, thanks for that.  Two points, then.  First, it looks like the count is 
> really just a boolean for whether the function recursively triggers any 
> diagnostics.   And second, can't it just be as simple as whether we've 
> visited that function at all in a context that's forcing diagnostics to be 
> emitted?  The logic seems to be to try to emit the diagnostics for each 
> use-path, but why would we want that?
For the second comment, we need to visit a function again for each use-path 
because we need to report each use-path that triggers a diagnostic, otherwise 
users will see a new error after they fix one error, instead of seeing all the 
errors at once.

For the first comment, I will change the count to two flags: one for the case 
where the function is not in device context, the other is for the case where 
the function is in device context. This will allow us to avoid redundant visits 
whether or not we are in a device context.



Comment at: clang/lib/Sema/Sema.cpp:1553
 if (!Caller)
   ShouldEmit = IsKnownEmitted;
 if ((!ShouldEmit && !S.getLangOpts().OpenMP && !Caller) ||

rjmccall wrote:
> This becomes global state for the visitor; that doesn't seem like it can be 
> right.
This state is for the root node of each traversal initiated from the items in 
DeclsToCheckForDeferredDiags. It only needs to be set once. Will rename it as 
ShouldEmitRootNode and move it to checkRecordedDecl.


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

https://reviews.llvm.org/D77028



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


[PATCH] D77028: [NFC] Refactor DeferredDiagsEmitter and skip redundant visit

2020-04-02 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1508
   void checkFunc(SourceLocation Loc, FunctionDecl *FD) {
+auto DiagsCountIt = DiagsCount.find(FD);
 FunctionDecl *Caller = UseStack.empty() ? nullptr : UseStack.back();

yaxunl wrote:
> rjmccall wrote:
> > It makes me a little uncomfortable to be holding an iterator this long 
> > while calling a fair amount of other stuff in the meantime.
> > 
> > Your use of DiagsCount is subtle enough that it really needs to be 
> > explained in some comments.  You're doing stuff conditionally based on both 
> > whether the entry exists but also whether it's non-zero.
> added comments
Okay, thanks for that.  Two points, then.  First, it looks like the count is 
really just a boolean for whether the function recursively triggers any 
diagnostics.   And second, can't it just be as simple as whether we've visited 
that function at all in a context that's forcing diagnostics to be emitted?  
The logic seems to be to try to emit the diagnostics for each use-path, but why 
would we want that?



Comment at: clang/lib/Sema/Sema.cpp:1553
 if (!Caller)
   ShouldEmit = IsKnownEmitted;
 if ((!ShouldEmit && !S.getLangOpts().OpenMP && !Caller) ||

This becomes global state for the visitor; that doesn't seem like it can be 
right.


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

https://reviews.llvm.org/D77028



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


[PATCH] D77028: [NFC] Refactor DeferredDiagsEmitter and skip redundant visit

2020-04-02 Thread Yaxun Liu via Phabricator via cfe-commits
yaxunl updated this revision to Diff 254537.
yaxunl marked 2 inline comments as done.
yaxunl added a comment.
Herald added a reviewer: jdoerfert.

added comments


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

https://reviews.llvm.org/D77028

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp

Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -1436,59 +1436,79 @@
 }
 
 // Print notes showing how we can reach FD starting from an a priori
-// known-callable function.
-static void emitCallStackNotes(Sema , FunctionDecl *FD) {
+// known-callable function. If \DiagsCount is not null pointer, the number
+// of diagnostics emitted for a function is accumulated.
+static void emitCallStackNotes(Sema , FunctionDecl *FD,
+   llvm::DenseMap,
+  unsigned> *DiagsCount = nullptr) {
   auto FnIt = S.DeviceKnownEmittedFns.find(FD);
   while (FnIt != S.DeviceKnownEmittedFns.end()) {
 DiagnosticBuilder Builder(
 S.Diags.Report(FnIt->second.Loc, diag::note_called_by));
 Builder << FnIt->second.FD;
 Builder.setForceEmit();
+// Count the number of deferred diagnostics directly or indirectly
+// triggered by a function.
+if (DiagsCount)
+  (*DiagsCount)[FnIt->second.FD]++;
 
 FnIt = S.DeviceKnownEmittedFns.find(FnIt->second.FD);
   }
 }
 
-// Emit any deferred diagnostics for FD and erase them from the map in which
-// they're stored.
-void Sema::emitDeferredDiags(FunctionDecl *FD, bool ShowCallStack) {
-  auto It = DeviceDeferredDiags.find(FD);
-  if (It == DeviceDeferredDiags.end())
-return;
-  bool HasWarningOrError = false;
-  bool FirstDiag = true;
-  for (PartialDiagnosticAt  : It->second) {
-const SourceLocation  = PDAt.first;
-const PartialDiagnostic  = PDAt.second;
-HasWarningOrError |= getDiagnostics().getDiagnosticLevel(
- PD.getDiagID(), Loc) >= DiagnosticsEngine::Warning;
-{
-  DiagnosticBuilder Builder(Diags.Report(Loc, PD.getDiagID()));
-  Builder.setForceEmit();
-  PD.Emit(Builder);
-}
-
-// Emit the note on the first diagnostic in case too many diagnostics cause
-// the note not emitted.
-if (FirstDiag && HasWarningOrError && ShowCallStack) {
-  emitCallStackNotes(*this, FD);
-  FirstDiag = false;
-}
-  }
-
-}
-
 namespace {
+
 /// Helper class that emits deferred diagnostic messages if an entity directly
 /// or indirectly using the function that causes the deferred diagnostic
 /// messages is known to be emitted.
+///
+/// During parsing of AST, certain diagnostic messages are recorded as deferred
+/// diagnostics since it is unknown whether the functions containing such
+/// diagnostics will be emitted. A list of potentially emitted functions and
+/// variables that may potentially trigger emission of functions are also
+/// recorded. DeferredDiagnosticsEmitter recursively visits used functions
+/// by each function to emit deferred diagnostics.
+///
+/// During the visit, certain OpenMP directives or initializer of variables
+/// with certain OpenMP attributes will cause subsequent visiting of any
+/// functions enter a state which is called OpenMP device context in this
+/// implementation. The state is exited when the directive or initializer is
+/// exited. This state can change the emission states of subsequent uses
+/// of functions.
+///
+/// Conceptually the functions or variables to be visited form a use graph
+/// where the parent node uses the child node. At any point of the visit,
+/// the tree nodes traversed from the tree root to the current node form a use
+/// stack. The emission state of the current node depends on two factors:
+///1. the emission state of the root node
+///2. whether the current node is in OpenMP device context
+/// If the function is decided to be emitted, its contained deferred diagnostics
+/// are emitted, together with the information about the use stack.
+///
+/// The number of deferred diagnostics directly or indirectly triggered by
+/// a function is counted. A function is visited at least once to count
+/// the triggered diagnostics. For subsequent visits, if the number of deferred
+/// diagnostics triggered by the function is 0 and currently is not in OpenMP
+/// device context, the function is skipped, since the subtree starting at this
+/// node does not trigger any deferred diagnostics and does not trigger any
+/// OpenMP device context, otherwise the counter cannot be 0.
+///
 class DeferredDiagnosticsEmitter
 : public UsedDeclVisitor {
 public:
   typedef UsedDeclVisitor Inherited;
   llvm::SmallSet, 4> Visited;
   llvm::SmallVector, 4> UseStack;
+
+  // Deferred diagnostics triggered by a declaration.
+  llvm::DenseMap, unsigned> DiagsCount;
+
+  // Emission state of the root node of the current use graph.
   

[PATCH] D77028: [NFC] Refactor DeferredDiagsEmitter and skip redundant visit

2020-04-02 Thread Yaxun Liu via Phabricator via cfe-commits
yaxunl added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1508
   void checkFunc(SourceLocation Loc, FunctionDecl *FD) {
+auto DiagsCountIt = DiagsCount.find(FD);
 FunctionDecl *Caller = UseStack.empty() ? nullptr : UseStack.back();

rjmccall wrote:
> It makes me a little uncomfortable to be holding an iterator this long while 
> calling a fair amount of other stuff in the meantime.
> 
> Your use of DiagsCount is subtle enough that it really needs to be explained 
> in some comments.  You're doing stuff conditionally based on both whether the 
> entry exists but also whether it's non-zero.
added comments


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

https://reviews.llvm.org/D77028



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


[PATCH] D77028: [NFC] Refactor DeferredDiagsEmitter and skip redundant visit

2020-04-01 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1508
   void checkFunc(SourceLocation Loc, FunctionDecl *FD) {
+auto DiagsCountIt = DiagsCount.find(FD);
 FunctionDecl *Caller = UseStack.empty() ? nullptr : UseStack.back();

It makes me a little uncomfortable to be holding an iterator this long while 
calling a fair amount of other stuff in the meantime.

Your use of DiagsCount is subtle enough that it really needs to be explained in 
some comments.  You're doing stuff conditionally based on both whether the 
entry exists but also whether it's non-zero.


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

https://reviews.llvm.org/D77028



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


[PATCH] D77028: [NFC] Refactor DeferredDiagsEmitter and skip redundant visit

2020-04-01 Thread Yaxun Liu via Phabricator via cfe-commits
yaxunl updated this revision to Diff 254407.
yaxunl added a comment.

rebase


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

https://reviews.llvm.org/D77028

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp

Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -1436,48 +1436,24 @@
 }
 
 // Print notes showing how we can reach FD starting from an a priori
-// known-callable function.
-static void emitCallStackNotes(Sema , FunctionDecl *FD) {
+// known-callable function. If \DiagsCount is not null pointer, the number
+// of diagnostics emitted for a function is accumulated.
+static void emitCallStackNotes(Sema , FunctionDecl *FD,
+   llvm::DenseMap,
+  unsigned> *DiagsCount = nullptr) {
   auto FnIt = S.DeviceKnownEmittedFns.find(FD);
   while (FnIt != S.DeviceKnownEmittedFns.end()) {
 DiagnosticBuilder Builder(
 S.Diags.Report(FnIt->second.Loc, diag::note_called_by));
 Builder << FnIt->second.FD;
 Builder.setForceEmit();
+if (DiagsCount)
+  (*DiagsCount)[FnIt->second.FD]++;
 
 FnIt = S.DeviceKnownEmittedFns.find(FnIt->second.FD);
   }
 }
 
-// Emit any deferred diagnostics for FD and erase them from the map in which
-// they're stored.
-void Sema::emitDeferredDiags(FunctionDecl *FD, bool ShowCallStack) {
-  auto It = DeviceDeferredDiags.find(FD);
-  if (It == DeviceDeferredDiags.end())
-return;
-  bool HasWarningOrError = false;
-  bool FirstDiag = true;
-  for (PartialDiagnosticAt  : It->second) {
-const SourceLocation  = PDAt.first;
-const PartialDiagnostic  = PDAt.second;
-HasWarningOrError |= getDiagnostics().getDiagnosticLevel(
- PD.getDiagID(), Loc) >= DiagnosticsEngine::Warning;
-{
-  DiagnosticBuilder Builder(Diags.Report(Loc, PD.getDiagID()));
-  Builder.setForceEmit();
-  PD.Emit(Builder);
-}
-
-// Emit the note on the first diagnostic in case too many diagnostics cause
-// the note not emitted.
-if (FirstDiag && HasWarningOrError && ShowCallStack) {
-  emitCallStackNotes(*this, FD);
-  FirstDiag = false;
-}
-  }
-
-}
-
 namespace {
 /// Helper class that emits deferred diagnostic messages if an entity directly
 /// or indirectly using the function that causes the deferred diagnostic
@@ -1488,6 +1464,10 @@
   typedef UsedDeclVisitor Inherited;
   llvm::SmallSet, 4> Visited;
   llvm::SmallVector, 4> UseStack;
+
+  // Deferred diagnostics triggered by a declaration.
+  llvm::DenseMap, unsigned> DiagsCount;
+
   bool ShouldEmit;
   unsigned InOMPDeviceContext;
 
@@ -1525,6 +1505,7 @@
   }
 
   void checkFunc(SourceLocation Loc, FunctionDecl *FD) {
+auto DiagsCountIt = DiagsCount.find(FD);
 FunctionDecl *Caller = UseStack.empty() ? nullptr : UseStack.back();
 auto IsKnownEmitted = S.getEmissionStatus(FD, /*Final=*/true) ==
   Sema::FunctionEmissionStatus::Emitted;
@@ -1536,10 +1517,15 @@
 // Finalize analysis of OpenMP-specific constructs.
 if (Caller && S.LangOpts.OpenMP && UseStack.size() == 1)
   S.finalizeOpenMPDelayedAnalysis(Caller, FD, Loc);
+else if (DiagsCountIt != DiagsCount.end() && DiagsCountIt->second == 0 &&
+ !InOMPDeviceContext)
+  return;
+if (DiagsCountIt == DiagsCount.end())
+  DiagsCount[FD] = 0;
 if (Caller)
   S.DeviceKnownEmittedFns[FD] = {Caller, Loc};
 if (ShouldEmit || InOMPDeviceContext)
-  S.emitDeferredDiags(FD, Caller);
+  emitDeferredDiags(FD, Caller);
 Visited.insert(FD);
 UseStack.push_back(FD);
 if (auto *S = FD->getBody()) {
@@ -1555,6 +1541,35 @@
 else
   checkVar(cast(D));
   }
+
+  // Emit any deferred diagnostics for FD
+  void emitDeferredDiags(FunctionDecl *FD, bool ShowCallStack) {
+auto It = S.DeviceDeferredDiags.find(FD);
+if (It == S.DeviceDeferredDiags.end())
+  return;
+bool HasWarningOrError = false;
+bool FirstDiag = true;
+for (PartialDiagnosticAt  : It->second) {
+  const SourceLocation  = PDAt.first;
+  const PartialDiagnostic  = PDAt.second;
+  HasWarningOrError |=
+  S.getDiagnostics().getDiagnosticLevel(PD.getDiagID(), Loc) >=
+  DiagnosticsEngine::Warning;
+  {
+DiagnosticBuilder Builder(S.Diags.Report(Loc, PD.getDiagID()));
+Builder.setForceEmit();
+PD.Emit(Builder);
+  }
+
+  DiagsCount[FD]++;
+  // Emit the note on the first diagnostic in case too many diagnostics
+  // cause the note not emitted.
+  if (FirstDiag && HasWarningOrError && ShowCallStack) {
+emitCallStackNotes(S, FD, );
+FirstDiag = false;
+  }
+}
+  }
 };
 } // namespace
 
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h

[PATCH] D77028: [NFC] Refactor DeferredDiagsEmitter and skip redundant visit

2020-03-30 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment.

I made a suggestion in the other patch about restructuring things out of 
`visitUsedDecl`.  I'll review this when that's done.


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

https://reviews.llvm.org/D77028



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


[PATCH] D77028: [NFC] Refactor DeferredDiagsEmitter and skip redundant visit

2020-03-29 Thread Yaxun Liu via Phabricator via cfe-commits
yaxunl created this revision.
yaxunl added reviewers: rjmccall, ABataev.

Move function emitDeferredDiags from Sema to DeferredDiagsEmitter since it
is only used by DeferredDiagsEmitter.

Also record number of diagnostics triggered by each function and skip a function
if it is known not to emit any diagnostics.


https://reviews.llvm.org/D77028

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp

Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -1436,48 +1436,24 @@
 }
 
 // Print notes showing how we can reach FD starting from an a priori
-// known-callable function.
-static void emitCallStackNotes(Sema , FunctionDecl *FD) {
+// known-callable function. If \DiagsCount is not null pointer, the number
+// of diagnostics emitted for a function is accumulated.
+static void emitCallStackNotes(Sema , FunctionDecl *FD,
+   llvm::DenseMap,
+  unsigned> *DiagsCount = nullptr) {
   auto FnIt = S.DeviceKnownEmittedFns.find(FD);
   while (FnIt != S.DeviceKnownEmittedFns.end()) {
 DiagnosticBuilder Builder(
 S.Diags.Report(FnIt->second.Loc, diag::note_called_by));
 Builder << FnIt->second.FD;
 Builder.setForceEmit();
+if (DiagsCount)
+  (*DiagsCount)[FnIt->second.FD]++;
 
 FnIt = S.DeviceKnownEmittedFns.find(FnIt->second.FD);
   }
 }
 
-// Emit any deferred diagnostics for FD and erase them from the map in which
-// they're stored.
-void Sema::emitDeferredDiags(FunctionDecl *FD, bool ShowCallStack) {
-  auto It = DeviceDeferredDiags.find(FD);
-  if (It == DeviceDeferredDiags.end())
-return;
-  bool HasWarningOrError = false;
-  bool FirstDiag = true;
-  for (PartialDiagnosticAt  : It->second) {
-const SourceLocation  = PDAt.first;
-const PartialDiagnostic  = PDAt.second;
-HasWarningOrError |= getDiagnostics().getDiagnosticLevel(
- PD.getDiagID(), Loc) >= DiagnosticsEngine::Warning;
-{
-  DiagnosticBuilder Builder(Diags.Report(Loc, PD.getDiagID()));
-  Builder.setForceEmit();
-  PD.Emit(Builder);
-}
-
-// Emit the note on the first diagnostic in case too many diagnostics cause
-// the note not emitted.
-if (FirstDiag && HasWarningOrError && ShowCallStack) {
-  emitCallStackNotes(*this, FD);
-  FirstDiag = false;
-}
-  }
-
-}
-
 namespace {
 /// Helper class that emits deferred diagnostic messages if an entity directly
 /// or indirectly using the function that causes the deferred diagnostic
@@ -1488,6 +1464,10 @@
   typedef UsedDeclVisitor Inherited;
   llvm::SmallSet, 4> Visited;
   llvm::SmallVector, 4> UseStack;
+
+  // Deferred diagnostics triggered by a declaration.
+  llvm::DenseMap, unsigned> DiagsCount;
+
   bool ShouldEmit;
   unsigned InOMPDeviceContext;
 
@@ -1509,6 +1489,8 @@
 
   void visitUsedDecl(SourceLocation Loc, Decl *D) {
 if (auto *FD = dyn_cast(D)) {
+
+  auto DiagsCountIt = DiagsCount.find(FD);
   FunctionDecl *Caller = UseStack.empty() ? nullptr : UseStack.back();
   auto IsKnownEmitted = S.getEmissionStatus(FD, /*Final=*/true) ==
 Sema::FunctionEmissionStatus::Emitted;
@@ -1520,10 +1502,15 @@
   // Finalize analysis of OpenMP-specific constructs.
   if (Caller && S.LangOpts.OpenMP && UseStack.size() == 1)
 S.finalizeOpenMPDelayedAnalysis(Caller, FD, Loc);
+  else if (DiagsCountIt != DiagsCount.end() && DiagsCountIt->second == 0 &&
+   !InOMPDeviceContext)
+return;
+  if (DiagsCountIt == DiagsCount.end())
+DiagsCount[FD] = 0;
   if (Caller)
 S.DeviceKnownEmittedFns[FD] = {Caller, Loc};
   if (ShouldEmit || InOMPDeviceContext)
-S.emitDeferredDiags(FD, Caller);
+emitDeferredDiags(FD, Caller);
   Visited.insert(D);
   UseStack.push_back(FD);
   if (auto *S = FD->getBody()) {
@@ -1551,6 +1538,35 @@
 } else
   Inherited::visitUsedDecl(Loc, D);
   }
+
+  // Emit any deferred diagnostics for FD
+  void emitDeferredDiags(FunctionDecl *FD, bool ShowCallStack) {
+auto It = S.DeviceDeferredDiags.find(FD);
+if (It == S.DeviceDeferredDiags.end())
+  return;
+bool HasWarningOrError = false;
+bool FirstDiag = true;
+for (PartialDiagnosticAt  : It->second) {
+  const SourceLocation  = PDAt.first;
+  const PartialDiagnostic  = PDAt.second;
+  HasWarningOrError |=
+  S.getDiagnostics().getDiagnosticLevel(PD.getDiagID(), Loc) >=
+  DiagnosticsEngine::Warning;
+  {
+DiagnosticBuilder Builder(S.Diags.Report(Loc, PD.getDiagID()));
+Builder.setForceEmit();
+PD.Emit(Builder);
+  }
+
+  DiagsCount[FD]++;
+  // Emit the note on the first diagnostic in case too many diagnostics
+  // cause the note not emitted.
+  if (FirstDiag && HasWarningOrError && ShowCallStack) {