[PATCH] D137982: Reland "[pgo] Avoid introducing relocations by using private alias""

[Paul Kirth via Phabricator via llvm-commits]

paulkirth added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:826
 
+static inline Constant *getFuncAddrForProfData(Function *Fn) {
+  auto *Int8PtrTy = Type::getInt8PtrTy(Fn->getContext());
----------------
leonardchan wrote:
> paulkirth wrote:
> > paulkirth wrote:
> > > leonardchan wrote:
> > > > paulkirth wrote:
> > > > > @xur, I think I've worked through a solution that will work here, and solve the issue for Fuchsia that we discussed. 
> > > > > 
> > > > > Does this look correct to you?  The logic is now: if we need the address for profd, get a private alias when we can. If the function is a comdat, only get a hidden one, if possible. 
> > > > > 
> > > > > Do you foresee any issues with this approach?
> > > > I could be wrong, but are there any flaws with simplifying this function to:
> > > > 
> > > > ```
> > > > if (!shouldRecordFunctionAddr(Fn) || Fn->isDeclarationForLinker())
> > > >   return ConstantPointerNull::get(Int8PtrTy);
> > > > 
> > > > if (Fn->isDSOLocal() || Fn->isImplicitDSOLocal())
> > > >   return Fn;
> > > > 
> > > > auto *GA = GlobalAlias::create(Fn->getLinkage(), Fn->getName(), Fn);
> > > > GA->setVisibility(GlobalValue::VisibilityTypes::HiddenVisibility);
> > > > return GA;
> > > > ```
> > > > 
> > > > ?
> > > > 
> > > > I think this would have the following desired results for these interesting cases:
> > > > 
> > > > - Fn is hidden/dso-local regardless of linkage or comdat-ness -> Fn is used resulting in RELATIVE reloc
> > > >   - This also covers local linkage symbols, hidden weak symbols, and hidden global symbols regardless of comdat
> > > > - Fn is global linkage, default visibility, no comdat -> Alias with hidden visibility and global linkage used, resulting in RELATIVE reloc
> > > > - Fn is weak linkage, default visibility, no comdat -> Alias with hidden visibility and weak linkage used, resulting in RELATIVE reloc
> > > > - Fn is weak linkage, default visibility, in comdat -> Alias with hidden visibility and weak linkage used, resulting in RELATIVE reloc, *but* the final value in __llvm_prf_data could be undefined, which I think is ok here still
> > > > 
> > > > I don't *think* there should ever be a case where we have a symbol with global linkage, default visibility, and in a comdat.
> > > The only time we want to insert nullptr is when we should record no address at all.  The case where we can’t emit an alias should still use the function pointer. 
> > > 
> > > I’m also not sure how that is any simpler than the change suggested above. I haven’t uploaded that yet, since I’m still testing it on some larger code bases. But so far it seems to work just as well. 
> > > 
> > > Also as mentioned we need to make sure this still works for COFF. I’m not sure what you’ve suggested is equivalent. 
> > @leonardchan Do you think this is a bit closer to what you were thinking.
> > 
> > Also, FYI, the bitcasting seems to be required, otherwise we run into assertion failures later. 
> Yeah it looks mostly similar. The only differences I can see are that aliases could be made even if the original Fn is hidden. But having an alias emitted in the way described now I think shouldn't break anything. Feel free to take the `Fn->isDSOLocal() || Fn->isImplicitDSOLocal()` check as just a suggestion though. My line of thinking is if we do want to create an alias, then we only do so if we absolutely need to.
Well, whenever we can, I think we try to use the private alias. I //think// that is a better mapping of intent here, but maybe it all boils down to the same thing. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137982