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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added a comment.

Thanks for the patch. I still think this is a silly feature to support, but users will probably expect it. See 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.
----------------
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?


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


================
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
----------------
Formatting looks weird, did you do `git clang-format HEAD~1`?


================
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
----------------
Just spitballing, is it possible to do this when we make the global instead?


================
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
----------------
If there are no updates between the host and device we can keep these static without emitting an offloading entry.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129694