[PATCH] D129694: [OPENMP] Make declare target static global externally visible

[Sunil Shrestha via Phabricator via cfe-commits]

ssquare08 added inline comments.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:10797
+        Out<<OrigName;
+        CGM.printPostfixForExternalizedDecl(Out, VD);
+        OffloadEntriesInfoManager.enterDeviceGlobalVarMangledName(OrigName, Buffer);
----------------
jhuber6 wrote:
> ssquare08 wrote:
> > jhuber6 wrote:
> > > `CGM.printPostfixForExternalizedDecl` should ideally give the same output on the host and device, but it's somewhat limited since it just checks the file ID and environment, which is technically possible to change. The kernels use `getTargetEntryUniqueInfo`, which might make sense to re-use for this case.
> > That was the point I had raised in one of the Clang meeting but someone had mentioned that kernels names are created on the host side and the device side reads the information though the Host IR. Seems like kernels name could  also run into mismatch issue for some corner cases then?
> So the problem here is that the host and device need to agree on what the name is so that we can register the correct variable. The CUDA / HIP toolchains solved this by either performing a mangling that is stable between the host and device, or by having the driver generate a random hash that gets used on both. OpenMP instead solves this by writing the variable to the host IR first and then reading it on the device to see what the name needs to be. Since we have that dependency we can use any mangling we want, though it's still best for it to be somewhat stable unless we want tests to change every time we run them. It probably won't hurt anything to just use `printPostfixForExternalizedDecl` but it's not as strong of a mangling as what we can do with the OpenMP method since it needs to be common between the host and device.
> `CGM.printPostfixForExternalizedDecl` should ideally give the same output on the host and device, but it's somewhat limited since it just checks the file ID and environment, which is technically possible to change. The kernels use `getTargetEntryUniqueInfo`, which might make sense to re-use for this case.

This has been changed as suggested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129694