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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added inline comments.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3284-3286
       // Hidden or internal symbols on the device are not externally visible. We
       // should not attempt to register them by creating an offloading entry.
       if (auto *GV = dyn_cast<llvm::GlobalValue>(CE->getAddress()))
----------------
This comment needs to be adjusted accordingly


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:10790
     Flags = OffloadEntriesInfoManagerTy::OMPTargetGlobalVarEntryTo;
-    VarName = CGM.getMangledName(VD);
+    // We don't need to mangle the host side of declare target global variables but we
+    // need to create offload entry that matches the device side which gets mangled.
----------------
ssquare08 wrote:
> jhuber6 wrote:
> > It might be easier to just mangle the original definition, that would reduce a lot of churn here adding `origName` everywhere. Any reason that's not desirable?
> You are right, it'd have made the code cleaner but we didn't want to mangle the host side if we could avoid it.
Others may want to comment, but personally I'm not too worried about mangling a name that wouldn't have been placed in the symbol table to begin with and it would make the code a lot cleaner.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:10797
+        Out<<OrigName;
+        CGM.printPostfixForExternalizedDecl(Out, VD);
+        OffloadEntriesInfoManager.enterDeviceGlobalVarMangledName(OrigName, Buffer);
----------------
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.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9439
     const Decl *D, llvm::GlobalValue *GV, CodeGen::CodeGenModule &M) const {
+  // Make sure any variable with OpenMP declare target is visible to runtime
+  // except for those with hidden visibility
----------------
ssquare08 wrote:
> jhuber6 wrote:
> > Just spitballing, is it possible to do this when we make the global instead?
> This is something I was wondering as well.  In CodeGenModule::GetOrCreateLLVMGlobal, when it creates a new global variable, it always uses the llvm::GlobalValue::ExternalLinkage. Seems like this changes somewhere later to internal for static globals. Do you know where that would be?
I'm not exactly sure, I remember deleting some code in D117806 that did something like that, albeit incorrectly. But I'm not sure if you'd have the necessary information to check whether or not there are updates attached to it. We don't want to externalize things if we don't need to, otherwise we'd get a lot of our device runtime variables with external visibility that now can't be optimized out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129694