ssquare08 marked an inline comment as not done.
ssquare08 added inline comments.
================
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:
> > 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
Thanks for the information, I'll take a look
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
