[PATCH] D127751: [MergeFunctions] Preserve symbols used llvm.used/llvm.compiler.used

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/MergeFunctions.cpp:841
+      // usually from inline asm.
+      if (G->hasGlobalUnnamedAddr() && !Used.contains(G)) {
         // G might have been a key in our GlobalNumberState, and it's illegal
----------------
Amanieu wrote:
> efriedma wrote:
> > I'm a bit suspicious of the hasGlobalUnnamedAddr() check here.  hasGlobalUnnamedAddr() generally implies it's legal to merge two constants/functions, but we don't actually figure out if we're going to perform an actual merge at this point; that doesn't happen until we try to discard it, or call writeThunkOrAlias.  (unnamed_addr only allows rewriting all the uses of a variable/function at once, not just some of them.  Otherwise you end up with weird inconsistencies where a value isn't equal to itself.)
> I think the code is valid as it is:
> - If hasGlobalUnnamedAddr() == true then we replace all uses of the function with replaceAllUsesWith.
> - Otherwise we only replace direct caller, which is equivalent to inlining the thunk/alias.
replaceAllUsesWith() only replaces uses in the current module, not uses in other modules.  So despite the name, it's not replacing "all" the uses in the sense we need here.

If the function has internal linkage, we're fine because we're actually rewriting all the uses.  If we replace the function with an alias, we're fine because every use refers to the same address.  Otherwise, there's a problem: we're replacing the uses in the current module, but not the uses in other modules.

(Making sure unnamed_addr works correctly in this respect is more likely to be a practical issue with constant variables, as opposed to functions, but I'd rather keep this consistent if possible.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127751