[MergeFunctions] Merge small functions if possible without a thunk

[whitequark]

Summary

This can result in significant code size savings in some cases,
e.g. an interrupt table all filled with the same assembly stub
in a certain Cortex-M BSP results in code blowup by a factor of 2.5.

Tests depend on D34805.

[whitequark]

Cosmetic change in debug output.

[jfb]

A few questions, but this looks good.

lib/Transforms/IPO/MergeFunctions.cpp

653 ↗

(On Diff #104596)

So I know that you're just moving code, but why these numbers? What's the usual thunk size? You also need to consider alignment (both of the function and thunks). IIRC there was a bunch of waste with alignment even when merge funcs ran, and I don't think it got fixed.

785 ↗

(On Diff #104596)

Weird that this code was way late here.

This fixme isn't relevant right? It's handled at line 631 it seems like.

[whitequark]

lib/Transforms/IPO/MergeFunctions.cpp

653 ↗

(On Diff #104596)

This checks for one basic block with one instruction in it apart from terminator. I think the idea is that you don't want to write a thunk for a thunk, and also a call instruction is typically larger than simple arithmetics, so you also don't want to write a thunk for that.

Regarding the alignment, I'm not sure I know all implications of changing that.

785 ↗

(On Diff #104596)

Yeah, I fixed it in D34805, on which this patch is based on.

llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp

void MergeFunctions::writeThunk(Function *F, Function *G) {
   // 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.
+  if (F->size() == 1) {
+    if (F->front().size() <= 2) {
+      DEBUG(dbgs() << "writeThunk: " << F->getName()
+                   << " is too small to bother creating a thunk for\n");
+      return;
+    }
+  }
+
   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");
if (Result.second) {
     assert(FNodesInTree.count(NewFunction) == 0);
     FNodesInTree.insert({NewFunction, Result.first});
     DEBUG(dbgs() << "Inserting as unique: " << NewFunction->getName() << '\n');
     return false;
   }
 
   const FunctionNode &OldF = *Result.first;
 
-  // Don't merge tiny functions, since it can just end up making the function
-  // larger.
-  // FIXME: Should still merge them if they are unnamed_addr and produce an
-  // alias.
-  if (NewFunction->size() == 1) {
-    if (NewFunction->front().size() <= 2) {
-      DEBUG(dbgs() << NewFunction->getName()
-                   << " is to small to bother merging\n");
-      return false;
-    }
-  }
-
   // Impose a total order (by name) on the replacement of functions. This is
   // important when operating on more than one module independently to prevent
   // cycles of thunks calling each other when the modules are linked together.
   //
   // First of all, we process strong functions before weak functions.
   if ((OldF.getFunc()->isInterposable() && !NewFunction->isInterposable()) ||
      (OldF.getFunc()->isInterposable() == NewFunction->isInterposable() &&
        OldF.getFunc()->getName() > NewFunction->getName())) {

llvm/trunk/test/Transforms/MergeFunc/merge-small-unnamed-addr.ll

+; RUN: opt -S -mergefunc < %s | FileCheck %s
+
+; CHECK-NOT: @b
+
+@x = constant { void ()*, void ()* } { void ()* @a, void ()* @b }
+; CHECK: { void ()* @a, void ()* @a }
+
+define internal void @a() unnamed_addr {
+  ret void
+}
+
+define internal void @b() unnamed_addr {
+  ret void
+}