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

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:826
 
+static inline Constant *getFuncAddrForProfData(Function *Fn) {
+  auto *Int8PtrTy = Type::getInt8PtrTy(Fn->getContext());
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137982