[MergeFunc] Allow merging identical vararg functions using aliases

[vsk]

Summary

Thanks to Nikita Popov for pointing out this missed case.

This is a follow-up to r351411, which disabled function merging for
vararg functions outright due to a miscompile (see llvm.org/PR40345).

[rnk]

Crazy idea: use musttail (only some targets support it) to make this work. Of course, first, make it correct.

[vsk]

In D56865#1362011, @rnk wrote:

Crazy idea: use musttail (only some targets support it) to make this work. Of course, first, make it correct.

That's really clever :). As identical vararg functions are a bit of an uncommon case, I'm not sure the added complexity would be worth it, though.

[nikic]

LGTM

On a related note, we have another case where thunks may be invalid due to CC restrictions: https://bugs.llvm.org/show_bug.cgi?id=40232

llvm/lib/Transforms/IPO/MergeFunctions.cpp

#endif
   /// Replace G with a simple tail call to bitcast(F). Also (unless
   /// MergeFunctionsPDI holds) replace direct uses of G with bitcast(F),
   /// delete G.
   void writeThunk(Function *F, Function *G);
 
   // Replace G with an alias to F (deleting function G)
   void writeAlias(Function *F, Function *G);
 
-  // Replace G with an alias to F if possible, or a thunk to F if
-  // profitable. Returns false if neither is the case.
+  // Replace G with an alias to F if possible, or a thunk to F if possible.
+  // Returns false if neither is the case.
   bool writeThunkOrAlias(Function *F, Function *G);
 
   /// Replace function F with function G in the function tree.
   void replaceFunctionInTree(const FunctionNode &FN, Function *G);
 
   /// The set of all distinct functions. Use the insert() and remove() methods
   /// to modify it. The map allows efficient lookup and deferring of Functions.
   FnTreeType FnTree;
if (const unsigned Max = NumFunctionsForSanityCheck) {
     return Valid;
   }
   return true;
 }
 #endif
 
 /// Check whether \p F is eligible for function merging.
 static bool isEligibleForMerging(Function &F) {
-  return !F.isDeclaration() && !F.hasAvailableExternallyLinkage() &&
-         !F.isVarArg();
+  return !F.isDeclaration() && !F.hasAvailableExternallyLinkage();
 }
 
 bool MergeFunctions::runOnModule(Module &M) {
   if (skipModule(M))
     return false;
 
   bool Changed = false;
 
if (PDIRelated.find(I) == PDIRelated.end()) {
       LLVM_DEBUG(dbgs() << "   PDIRelated: ");
       LLVM_DEBUG(I->print(dbgs()));
       LLVM_DEBUG(dbgs() << "\n");
     }
   }
   LLVM_DEBUG(dbgs() << " }\n");
 }
 
+/// Whether this function may be replaced by a forwarding thunk.
+static bool canCreateThunkFor(Function *F) {
+  if (F->isVarArg())
+    return false;
+
   // 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) {
-      LLVM_DEBUG(dbgs() << "isThunkProfitable: " << F->getName()
+      LLVM_DEBUG(dbgs() << "canCreateThunkFor: " << F->getName()
                         << " is too small to bother creating a thunk for\n");
       return false;
     }
   }
   return true;
 }
 
 // Replace G with a simple tail call to bitcast(F). Also (unless

 
 // Replace G with an alias to F if possible, or a thunk to F if
 // profitable. Returns false if neither is the case.
 bool MergeFunctions::writeThunkOrAlias(Function *F, Function *G) {
   if (canCreateAliasFor(G)) {
     writeAlias(F, G);
     return true;
   }
-  if (isThunkProfitable(F)) {
+  if (canCreateThunkFor(F)) {
     writeThunk(F, G);
     return true;
   }
   return false;
 }
 
 // Merge two equivalent functions. Upon completion, Function G is deleted.
 void MergeFunctions::mergeTwoFunctions(Function *F, Function *G) {
   if (F->isInterposable()) {
     assert(G->isInterposable());
 
     // Both writeThunkOrAlias() calls below must succeed, either because we can
     // create aliases for G and NewF, or because a thunk for F is profitable.
     // F here has the same signature as NewF below, so that's what we check.
-    if (!isThunkProfitable(F) && (!canCreateAliasFor(F) || !canCreateAliasFor(G))) {
+    if (!canCreateThunkFor(F) &&
+        (!canCreateAliasFor(F) || !canCreateAliasFor(G)))
       return;
-    }
 
     // Make them both thunks to the same internal function.
     Function *NewF = Function::Create(F->getFunctionType(), F->getLinkage(),
                                       F->getAddressSpace(), "", F->getParent());
     NewF->copyAttributesFrom(F);
     NewF->takeName(F);
     removeUsers(F);
     F->replaceAllUsesWith(NewF);

llvm/test/Transforms/MergeFunc/va_arg.ll

 ; RUN: opt -S -mergefunc < %s | FileCheck %s
+; RUN: opt -S -mergefunc -mergefunc-use-aliases < %s | FileCheck %s -check-prefix=ALIAS
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
+; ALIAS: @_Z9simple_vaPKcz = unnamed_addr alias void (i8*, ...), void (i8*, ...)* @_Z10simple_va2PKcz
+; ALIAS-NOT: @_Z9simple_vaPKcz
+
 %struct.__va_list_tag = type { i32, i32, i8*, i8* }
 
 ; CHECK-LABEL: define {{.*}}@_Z9simple_vaPKcz
 ; CHECK: call void @llvm.va_start
 ; CHECK: call void @llvm.va_end
-define dso_local void @_Z9simple_vaPKcz(i8* nocapture readnone, ...) local_unnamed_addr {
+define dso_local void @_Z9simple_vaPKcz(i8* nocapture readnone, ...) unnamed_addr {
   %2 = alloca [1 x %struct.__va_list_tag], align 16
   %3 = bitcast [1 x %struct.__va_list_tag]* %2 to i8*
   call void @llvm.va_start(i8* nonnull %3)
   %4 = getelementptr inbounds [1 x %struct.__va_list_tag], [1 x %struct.__va_list_tag]* %2, i64 0, i64 0, i32 0
   %5 = load i32, i32* %4, align 16
   %6 = icmp ult i32 %5, 41
   br i1 %6, label %7, label %13
 

 declare dso_local void @_Z6escapei(i32) local_unnamed_addr
 
 ; Function Attrs: nounwind
 declare void @llvm.va_end(i8*)
 
 ; CHECK-LABEL: define {{.*}}@_Z10simple_va2PKcz
 ; CHECK: call void @llvm.va_start
 ; CHECK: call void @llvm.va_end
-define dso_local void @_Z10simple_va2PKcz(i8* nocapture readnone, ...) local_unnamed_addr {
+define dso_local void @_Z10simple_va2PKcz(i8* nocapture readnone, ...) unnamed_addr {
   %2 = alloca [1 x %struct.__va_list_tag], align 16
   %3 = bitcast [1 x %struct.__va_list_tag]* %2 to i8*
   call void @llvm.va_start(i8* nonnull %3)
   %4 = getelementptr inbounds [1 x %struct.__va_list_tag], [1 x %struct.__va_list_tag]* %2, i64 0, i64 0, i32 0
   %5 = load i32, i32* %4, align 16
   %6 = icmp ult i32 %5, 41
   br i1 %6, label %7, label %13