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

[Sunil Shrestha via Phabricator via cfe-commits]

ssquare08 added a comment.

In D129694#3717008 <https://reviews.llvm.org/D129694#3717008>, @ssquare08 wrote:

> In D129694#3708297 <https://reviews.llvm.org/D129694#3708297>, @jhuber6 wrote:
>
>> In D129694#3708214 <https://reviews.llvm.org/D129694#3708214>, @ssquare08 wrote:
>>
>>> If that's the preference I can make changes as suggested. You mentioned CUDA and HIP mangle the declaration directly. To me it looks like they mangle it on host and device separately. Is that not correct? If so, can you point me to the source you are referring to?
>>
>> You're right, they mangle them separately like in https://godbolt.org/z/r6hG4brqx, this is most likely because they already had separate "device side" names. For OpenMP we currently just use the same name for the variable on the host and device side like in https://godbolt.org/z/eaGo9qsW3 where we just use the same kernel names. Thinking again, I'm still wondering if there's any utility in keeping the names separate. Correct me if I'm wrong, but the host-side variable should be able to remain internal so this mangled device name shouldn't show up in the final executable. In that case the only benefit is slightly nicer IR, which I'm not super concerned with.
>
> Yes, the host-side variable should be able to remain internal.

The OpenMP kernel names you mentioned are also generated separately by the host and the device. Would you be okay generating declare target mangle names separately by host and device using the same utility function `getTargetEntryUniqueInfo`?

If you still think it should only be generated only once by the host, what is a good way of doing this since we can't modify the name in VarDecl?



================
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:
> ssquare08 wrote:
> > jhuber6 wrote:
> > > jhuber6 wrote:
> > > > 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.
> > > Were you able to find a place for this when we generate the variable? You should be able to do something similar to the patch above if it's a declare target static to force it to have external visibility, but as mentioned before I would prefer we only do this if necessary which might take some extra analysis.
> > If you are asking about the GV, it is created in 'CodeGenModule::GetOrCreateLLVMGlobal' with external linkage always.
> > ```
> >   auto *GV = new llvm::GlobalVariable(
> >       getModule(), Ty, false, llvm::GlobalValue::ExternalLinkage, nullptr,
> >       MangledName, nullptr, llvm::GlobalVariable::NotThreadLocal,
> >       getContext().getTargetAddressSpace(DAddrSpace));
> > ```
> > The linkage, however, changes in 'CodeGenModule::EmitGlobalVarDefinition' based on the information VarDecl
> > 
> > ```
> >   llvm::GlobalValue::LinkageTypes Linkage =
> >       getLLVMLinkageVarDefinition(D, GV->isConstant());
> > ```
> > 
> > Maybe you are suggesting changing the linkage information in 'VarDecl' itself?
> Yes, the patch I linked previously did something like that where it set the `LinkageValue` based on some information. Although I'm not sure if it would be excessively difficult to try to prune definitions that don't need to be externalized. I haven't looked too deep into this, but I believe CUDA does this inside of `adjustGVALinkageForAttributes`, there we also check some variable called `CUDADeviceVarODRUsedByHost` that I'm assuming tracks if we need to bother externalizing this.
The exter


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129694