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

[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