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

[Alex Brachet via Phabricator via llvm-commits]

abrachet 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,
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:849
+  if (Fn->hasComdat()) {
+    GA->setLinkage(Fn->getLinkage());
+    GA->setVisibility(Fn->hasLocalLinkage()
----------------
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.


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:850
+    GA->setLinkage(Fn->getLinkage());
+    GA->setVisibility(Fn->hasLocalLinkage()
+                          ? GlobalValue::VisibilityTypes::DefaultVisibility
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137982