[MergeFunctions] Fix merging of small weak functions

When two interposable functions are merged, we cannot replace
uses and have to emit calls to a common internal function. However,
writeThunk() will not actually emit a thunk if the function is too
small. This leaves us in a broken state where mergeTwoFunctions
already rewired the functions, but writeThunk doesn't do anything.

This patch changes the implementation so that:

 * writeThunk() does just that.
 * The direct replacement of calls is moved into mergeTwoFunctions()
   into the non-interposable case only.
 * isThunkProfitable() is extracted and will be called for
   the non-iterposable case always, and in the interposable case
   only if uses are still left after replacement.

This issue has been introduced in https://reviews.llvm.org/D34806,
where the code for checking thunk profitability has been moved.

Differential Revision: https://reviews.llvm.org/D46804

Reviewed By: whitequark

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@332342 91177308-0d34-0410-b5e6-96231b3b80d8

lib/Transforms/IPO/MergeFunctions.cpp

@@ -638,48 +638,25 @@ void MergeFunctions::filterInstsUnrelatedToPDI(
   DEBUG(dbgs() << " }\n");
 }
 
-// Replace G with a simple tail call to bitcast(F). Also (unless
-// MergeFunctionsPDI holds) replace direct uses of G with bitcast(F),
-// delete G. Under MergeFunctionsPDI, we use G itself for creating
-// the thunk as we preserve the debug info (and associated instructions)
-// from G's entry block pertaining to G's incoming arguments which are
-// passed on as corresponding arguments in the call that G makes to F.
-// For better debugability, under MergeFunctionsPDI, we do not modify G's
-// call sites to point to F even when within the same translation unit.
-void MergeFunctions::writeThunk(Function *F, Function *G) {
-  if (!G->isInterposable() && !MergeFunctionsPDI) {
-    if (G->hasGlobalUnnamedAddr()) {
-      // G might have been a key in our GlobalNumberState, and it's illegal
-      // to replace a key in ValueMap<GlobalValue *> with a non-global.
-      GlobalNumbers.erase(G);
-      // If G's address is not significant, replace it entirely.
-      Constant *BitcastF = ConstantExpr::getBitCast(F, G->getType());
-      G->replaceAllUsesWith(BitcastF);
-    } else {
-      // Redirect direct callers of G to F. (See note on MergeFunctionsPDI
-      // above).
-      replaceDirectCallers(G, F);
-    }
-  }
-
-  // If G was internal then we may have replaced all uses of G with F. If so,
-  // stop here and delete G. There's no need for a thunk. (See note on
-  // MergeFunctionsPDI above).
-  if (G->hasLocalLinkage() && G->use_empty() && !MergeFunctionsPDI) {
-    G->eraseFromParent();
-    return;
-  }
-
-  // Don't merge tiny functions using a thunk, since it can just end up
-  // making the function larger.
+// Don't merge tiny functions using a thunk, since it can just end up
+// making the function larger.
+static bool isThunkProfitable(Function *F) {
   if (F->size() == 1) {
     if (F->front().size() <= 2) {
-      DEBUG(dbgs() << "writeThunk: " << F->getName()
+      DEBUG(dbgs() << "isThunkProfitable: " << F->getName()
                    << " is too small to bother creating a thunk for\n");
-      return;
+      return false;
     }
   }
+  return true;
+}
 
+// Replace G with a simple tail call to bitcast(F). Under MergeFunctionsPDI,
+// we use G itself for creating the thunk as we preserve the debug info
+// (and associated instructions) from G's entry block pertaining to G's
+// incoming arguments which are passed on as corresponding arguments in
+// the call that G makes to F.
+void MergeFunctions::writeThunk(Function *F, Function *G) {
   BasicBlock *GEntryBlock = nullptr;
   std::vector<Instruction *> PDIUnrelatedWL;
   BasicBlock *BB = nullptr;
@@ -754,6 +731,10 @@ void MergeFunctions::mergeTwoFunctions(Function *F, Function *G) {
   if (F->isInterposable()) {
     assert(G->isInterposable());
 
+    if (!isThunkProfitable(F)) {
+      return;
+    }
+
     // Make them both thunks to the same internal function.
     Function *H = Function::Create(F->getFunctionType(), F->getLinkage(), "",
                                    F->getParent());
@@ -770,11 +751,41 @@ void MergeFunctions::mergeTwoFunctions(Function *F, Function *G) {
     F->setAlignment(MaxAlignment);
     F->setLinkage(GlobalValue::PrivateLinkage);
     ++NumDoubleWeak;
+    ++NumFunctionsMerged;
   } else {
+    // For better debugability, under MergeFunctionsPDI, we do not modify G's
+    // call sites to point to F even when within the same translation unit.
+    if (!G->isInterposable() && !MergeFunctionsPDI) {
+      if (G->hasGlobalUnnamedAddr()) {
+        // G might have been a key in our GlobalNumberState, and it's illegal
+        // to replace a key in ValueMap<GlobalValue *> with a non-global.
+        GlobalNumbers.erase(G);
+        // If G's address is not significant, replace it entirely.
+        Constant *BitcastF = ConstantExpr::getBitCast(F, G->getType());
+        G->replaceAllUsesWith(BitcastF);
+      } else {
+        // Redirect direct callers of G to F. (See note on MergeFunctionsPDI
+        // above).
+        replaceDirectCallers(G, F);
+      }
+    }
+
+    // If G was internal then we may have replaced all uses of G with F. If so,
+    // stop here and delete G. There's no need for a thunk. (See note on
+    // MergeFunctionsPDI above).
+    if (G->hasLocalLinkage() && G->use_empty() && !MergeFunctionsPDI) {
+      G->eraseFromParent();
+      ++NumFunctionsMerged;
+      return;
+    }
+
+    if (!isThunkProfitable(F)) {
+      return;
+    }
+
     writeThunk(F, G);
+    ++NumFunctionsMerged;
   }
-
-  ++NumFunctionsMerged;
 }
 
 /// Replace function F by function G.

test/Transforms/MergeFunc/weak-small.ll

@@ -0,0 +1,16 @@
+; RUN: opt -mergefunc -S < %s | FileCheck %s
+
+; Weak functions too small for merging to be profitable
+
+; CHECK: define weak i32 @foo(i8*, i32)
+; CHECK-NEXT: ret i32 %1
+; CHECK: define weak i32 @bar(i8*, i32)
+; CHECK-NEXT: ret i32 %1
+
+define weak i32 @foo(i8*, i32) #0 {
+    ret i32 %1
+}
+
+define weak i32 @bar(i8*, i32) #0 {
+    ret i32 %1
+}