[PATCH] D122734: [CUDA][HIP] Fix mangling number for local struct

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.
Herald added a subscriber: mattd.


================
Comment at: clang/include/clang/AST/ASTContext.h:682
+    /// Current name mangling is for device name in host compilation.
+    bool MangleDeviceNameInHostCompilation = false;
+  } CUDANameMangleCtx;
----------------
It doesn't feel right to store mutable state here in ASTContext. You are effectively using this as a global variable to thread a boolean through to `ASTContext::getManglingNumber`, which is called from the mangler. The boolean indicates of host or device mangling numbers are desired for this particular mangling. Is that more or less correct?

This makes me think that it was perhaps a mistake to store mangling numbers in the AST Context in the first place. If we stored them in the mangling context instead, I think we'd get the right behavior out of the box. Is that correct? It it feasible to make that change?


================
Comment at: clang/include/clang/AST/ASTContext.h:684
+  } CUDANameMangleCtx;
+  struct CUDANameMangleContextRAII {
+    ASTContext &Ctx;
----------------
By the way, this looks like `SaveAndRestore<bool>`. Maybe you can use that in the future.


================
Comment at: clang/lib/AST/ASTContext.cpp:11760
+
+  auto Cutoff = [](unsigned V) { return V > 1 ? V : 1; };
+  if (CUDANameMangleCtx.MangleDeviceNameInHostCompilation)
----------------
tra wrote:
> I think we should change the `MangleNumbers` instead to use uin64_t or, perhaps `unsigned[2]` or even `struct{ unsigned host; unsigned device;}`.
> Reducing the max number of anything to just 64K will be prone to failing sooner or later. 
IMO it would be simpler to set up a ternary instead of a lambda and two returns:
  Res = Cond ? (Res >> 16) : (Res & 0xFFFF);
  return Res > 1 ? Res : 1;


================
Comment at: clang/lib/AST/ASTContext.cpp:11760-11763
+  auto Cutoff = [](unsigned V) { return V > 1 ? V : 1; };
+  if (CUDANameMangleCtx.MangleDeviceNameInHostCompilation)
+    return Cutoff(Res >> 16);
+  return Cutoff(Res & 0xffff);
----------------
rnk wrote:
> tra wrote:
> > I think we should change the `MangleNumbers` instead to use uin64_t or, perhaps `unsigned[2]` or even `struct{ unsigned host; unsigned device;}`.
> > Reducing the max number of anything to just 64K will be prone to failing sooner or later. 
> IMO it would be simpler to set up a ternary instead of a lambda and two returns:
>   Res = Cond ? (Res >> 16) : (Res & 0xFFFF);
>   return Res > 1 ? Res : 1;
I suspect we could live with a 64K mangling number limit, but that is low enough that we need to put in a check for overflow somewhere.


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

https://reviews.llvm.org/D122734