[PATCH] D158247: [CUDA][HIP] Fix overloading resolution in global variable initializer

[Yaxun Liu via Phabricator via cfe-commits]

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


================
Comment at: clang/include/clang/Sema/Sema.h:4753
+  /// Otherwise, use \p D to determiine the host/device target.
   bool CheckCallingConvAttr(const ParsedAttr &attr, CallingConv &CC,
+                            const FunctionDecl *FD = nullptr,
----------------
tra wrote:
> It appears that `Declarator D` here is only used as an attribute carrier used to identify CUDA calling target.
> Should we pass `CudaTarget ContextTarget` instead and let the caller figure out how to find it?
> 
> I'm just thinking that we're hardcoding just one specific way to find the target, while there may potentially be more.
> The current way is OK, as we have just one use case at the moment.
> 
> 
will do


================
Comment at: clang/lib/Sema/SemaCUDA.cpp:137
+  // Code that lives outside a function gets the target from CurCUDATargetCtx.
+  if (D == nullptr) {
+    return CurCUDATargetCtx.Target;
----------------
tra wrote:
> Style nit: no braces around single-statement body.
will fix


================
Comment at: clang/test/CodeGenCUDA/global-initializers.cu:11-12
+// Check host/device-based overloding resolution in global variable initializer.
+template<typename T, typename U>
+T pow(T, U) { return 1.0; }
+
----------------
tra wrote:
> We don't really need templates to reproduce the issue. We just need a host function with lower overloading priority. A function requiring type conversion or with an additional default argument should do. E.g.  `float pow(float, int); ` or `double X = pow(double, int, bool lower_priority_host_overload=1);`
> 
> Removing template should unclutter the tests a bit.
> 
will do


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

https://reviews.llvm.org/D158247