Fix merging of small weak functions

[nikic]

Summary

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.

[whitequark]

Thanks for fixing this!

[whitequark]

LGTM

[nox]

0001-Fix-merging-of-small-weak-functions.patch5 KBDownload

I rebased it against trunk.

lib/Transforms/IPO/MergeFunctions.cpp

if (PDIRelated.find(I) == PDIRelated.end()) {
       DEBUG(dbgs() << "   PDIRelated: ");
       DEBUG(I->print(dbgs()));
       DEBUG(dbgs() << "\n");
     }
   }
   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.
+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;
   Function *NewG = nullptr;
   if (MergeFunctionsPDI) {
     DEBUG(dbgs() << "writeThunk: (MergeFunctionsPDI) Do not create a new "
                     "function as thunk; retain original: "
                  << G->getName() << "()\n");
void MergeFunctions::writeThunk(Function *F, Function *G) {
   ++NumThunksWritten;
 }
 
 // Merge two equivalent functions. Upon completion, Function G is deleted.
 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());
     H->copyAttributesFrom(F);
     H->takeName(F);
     removeUsers(F);
     F->replaceAllUsesWith(H);
 
     unsigned MaxAlignment = std::max(G->getAlignment(), H->getAlignment());
 
     writeThunk(F, G);
     writeThunk(F, H);
 
     F->setAlignment(MaxAlignment);
     F->setLinkage(GlobalValue::PrivateLinkage);
     ++NumDoubleWeak;
+    ++NumFunctionsMerged;
   } else {
-    writeThunk(F, G);
+    // 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;
   }
+}
 
 /// Replace function F by function G.
 void MergeFunctions::replaceFunctionInTree(const FunctionNode &FN,
                                            Function *G) {
   Function *F = FN.getFunc();
   assert(FunctionComparator(F, G, &GlobalNumbers).compare() == 0 &&
          "The two functions must be equal");
   

test/Transforms/MergeFunc/weak-small.ll

+; 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
+}