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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
