[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:837
+
+  // When possible use a private alias to avoid relocations with addends.
+  auto *GA = GlobalAlias::create(GlobalValue::LinkageTypes::PrivateLinkage,
----------------
abrachet wrote:
> 
Good suggestion. Thank you.


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:849
+  if (Fn->hasComdat()) {
+    GA->setLinkage(Fn->getLinkage());
+    GA->setVisibility(Fn->hasLocalLinkage()
----------------
abrachet wrote:
> I'm not sure what the linkage should be here, but it doesn't feel right that it should correspond with the functions linkage. At least for ELF, {LinkOnce,Weak}{Any,ODR}Linkage (I think) will all be .weak so this will be fine, but presumably there's more nuance on COFF. Maybe something to investigate.
I'm not exactly sure either, but using the same linkage seems acceptable, since it should be no worse than using the function. Its also how we generate aliases for relative vtables (https://github.com/llvm/llvm-project/blob/a602f76a2406cc3edd6b297ede3583b26513a34c/clang/lib/CodeGen/CGVTables.cpp#L982). There we use the existing linkage if the function is comdat. The case for vtables may have a more limited range of possibilities though, so maybe the same approach isn't warranted.

As you mentioned, I'm also not sure what the approach should be for COFF, since it seems there are a number of places where we handle  it specially.

Are you aware of any issues that could arise if we use the Function's linkage?


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:850
+    GA->setLinkage(Fn->getLinkage());
+    GA->setVisibility(Fn->hasLocalLinkage()
+                          ? GlobalValue::VisibilityTypes::DefaultVisibility
----------------
abrachet wrote:
> If it `hasLocalLinkage()` we can just use it's name directly like in the `if (Fn->isDeclarationForLinker())` above and not worry about creating an alias at all.
> 
> I think it's also the case that `Fn->hasComdat()` and `Fn->hasLocalLinkage()` cannot be mutually true per your comment above.
I'm almost 100% certain this case cannot happen based on the check in `ShouldRecordFunctionAddress()`, but I wanted to keep the logic here clear, and not dependent on the internals of that check.

But maybe it makes more sense to move the `hasLocalLinkage()` check up, as you suggested, and simplify this case. I'll test that, and if it works, I can update the patch accordingly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137982