[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: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.
----------------
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.


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


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:7292
     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
----------------
jhuber6 wrote:
> Formatting looks weird, did you do `git clang-format HEAD~1`?
Looks like I didn't run git clang-format correctly, I'll fix it. Thanks


================
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
----------------
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?


================
Comment at: clang/test/OpenMP/declare_target_visibility_codegen.cpp:11-12
 // HOST: @z = global i32 0
-// HOST-NOT: @.omp_offloading.entry.c
-// HOST-NOT: @.omp_offloading.entry.x
+// HOST: @.omp_offloading.entry.c__static__{{[0-9a-z]+_[0-9a-z]+}}
+// HOST: @.omp_offloading.entry.x__static__{{[0-9a-z]+_[0-9a-z]+}}
 // HOST-NOT: @.omp_offloading.entry.y
----------------
jhuber6 wrote:
> If there are no updates between the host and device we can keep these static without emitting an offloading entry.
That 's a good point. I'll fix that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129694