[PATCH] D71227: [cuda][hip] Fix function overload resolution in the global initiailizer.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/Sema/Sema.h:11444
+      return nullptr;
+    const Decl *ContextDecl = dyn_cast<FunctionDecl>(CurContext);
+    if (!ContextDecl)
----------------
rjmccall wrote:
> You really want this to match whenever we're in a local context, right?  How about structuring the function like:
> 
> ```
> if (CurContext->isFunctionOrMethod())
>   return cast<Decl>(CurContext);
> if (!CurContext->isFileContext())
>   return nullptr;
> return getCUDACurrentNonLocalVariable();
> ```
> 
> As a more general solution, I think Sema funnels all changes to CurContext through a small number of places, and you could make those places save and restore the currently initialized variable as well.
Richard, I'd like your opinion about this.  We have three separate patches right now that would all benefit from being able to track that they're currently within a variable/field initializer in Sema.  And it's a general deficiency that it's hard to track declarations in initializers back to their initialized variable.

Swift actually bit the bullet and introduced a kind of `DeclContext` that's a non-local initializer, and that links back to the variable.  That would be hard to bring back to Clang with the current AST because Clang assumes that all `DeclContext`s are `Decl`s, and I don't think we can reasonably remove that assumption; and of course `VarDecl` and `FieldDecl` aren't `DeclContext`s.

Now, we could try to change that latter point.  Making *all* `VarDecl`s and `FieldDecl`s DCs would have prohibitive memory overhead, since the vast majority are local / uninitialized; however, we could introduce a `VarDecl` subclass for global variables (including static member variables, of course), and similarly we could have a `FieldDecl` subclass for fields with initializers, which would nicely move some of the other overhead out-of-line and optimize for the C-style/old-style case.  (We always know whether a field has an in-class initializer at parse time, right?)

Less invasively, we could forget about trying to track this in the AST and just also track a current initialized variable in Sema.  Anything which tried to change the context would have to save and restore that as well.  That might be annoying because of PushDeclContext/PopDeclContext, though, which assume that you can restore the old context by just looking at the current context.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16702
+  if (!D || D->isInvalidDecl() || !isNonlocalVariable(D))
+    return;
+  assert(!CUDANonLocalVariableStack.empty() &&
----------------
The declaration could become invalid while processing its initializer; I think you should drop that condition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71227