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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/include/clang/Sema/Sema.h:11190
   /// combination, based on their host/device attributes.
-  /// \param Caller function which needs address of \p Callee.
-  ///               nullptr in case of global context.
-  /// \param Callee target function
+  /// \param CallContextDecl the context decl which needs address of \p Callee.
+  ///                        nullptr in case of global context.
----------------
Please capitalize the first word of each of these parameter descriptions, to match the style used elsewhere in Clang.


================
Comment at: clang/include/clang/Sema/Sema.h:11191
+  /// \param CallContextDecl the context decl which needs address of \p Callee.
+  ///                        nullptr in case of global context.
+  /// \param Callee          target function
----------------
"Null" not "nullptr".


================
Comment at: clang/include/clang/Sema/Sema.h:11198-11206
+  SmallVector<const Decl *, 8> CUDANonLocalVariableStack;
+
+  void pushCUDANonLocalVariable(const Decl *D);
+  void popCUDANonLocalVariable(const Decl *D);
+
+  const Decl *getCUDACurrentNonLocalVariable() const {
+    return CUDANonLocalVariableStack.empty() ? nullptr
----------------
Does this really need to be CUDA-specific?

This is (at least) the third time we've needed this. We currently have a `ManglingContextDecl` on `ExpressionEvaluationContextRecord` that tracks the non-local variable whose initializer we're parsing. In addition to using this as a lambda context declaration, we also (hackily) use it as the context declaration for `DiagRuntimeBehavior`. It would seem sensible to use that mechanism here too (and rename it to remove any suggestion that this is specific to lambdas or mangling).

I think we only currently push `ExpressionEvaluationContext`s for variable initializers in C++. That's presumably fine for CUDA's purposes.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:2336
 
+  Actions.pushCUDANonLocalVariable(ThisDecl);
+
----------------
tra wrote:
> @rsmith -- is this sufficient to catch all attempts to call an initializer for a global?
> I wonder if there are other sneaky ways to call an initializer. 
No, this is not sufficient; it's missing (at least) the template instantiation case. (The `ExpressionEvaluationContextRecord` mechanism does handle that case properly.)

You should also consider what should happen in default arguments (which are sometimes parsed before we form a `FunctionDecl` for the function for which they are parameters) and default member initializers (which are parsed after we know whether the enclosing class has a user-declared default constructor, so you could in principle consider the CUDA function kind of the declared constructors, I suppose -- but the constructor bodies are not yet available, so you can't tell which constructors would actually use the initializers). Both of those cases are also tracked by the `ExpressionEvaluationContextRecord` mechanism, though you may need to track additional information to process default arguments in the same mode as the function for which they are supplied.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71227