Skip to content
This repository was archived by the owner on Apr 23, 2020. It is now read-only.

Commit be07815

Browse files
committed
Merging r332342:
------------------------------------------------------------------------ r332342 | whitequark | 2018-05-15 04:31:07 -0700 (Tue, 15 May 2018) | 23 lines [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/branches/release_60@332662 91177308-0d34-0410-b5e6-96231b3b80d8
1 parent 3d22b2c commit be07815

File tree

2 files changed

+65
-35
lines changed

2 files changed

+65
-35
lines changed

lib/Transforms/IPO/MergeFunctions.cpp

+49-35
Original file line numberDiff line numberDiff line change
@@ -638,6 +638,19 @@ void MergeFunctions::filterInstsUnrelatedToPDI(
638638
DEBUG(dbgs() << " }\n");
639639
}
640640

641+
// Don't merge tiny functions using a thunk, since it can just end up
642+
// making the function larger.
643+
static bool isThunkProfitable(Function * F) {
644+
if (F->size() == 1) {
645+
if (F->front().size() <= 2) {
646+
DEBUG(dbgs() << "isThunkProfitable: " << F->getName()
647+
<< " is too small to bother creating a thunk for\n");
648+
return false;
649+
}
650+
}
651+
return true;
652+
}
653+
641654
// Replace G with a simple tail call to bitcast(F). Also (unless
642655
// MergeFunctionsPDI holds) replace direct uses of G with bitcast(F),
643656
// delete G. Under MergeFunctionsPDI, we use G itself for creating
@@ -647,39 +660,6 @@ void MergeFunctions::filterInstsUnrelatedToPDI(
647660
// For better debugability, under MergeFunctionsPDI, we do not modify G's
648661
// call sites to point to F even when within the same translation unit.
649662
void MergeFunctions::writeThunk(Function *F, Function *G) {
650-
if (!G->isInterposable() && !MergeFunctionsPDI) {
651-
if (G->hasGlobalUnnamedAddr()) {
652-
// G might have been a key in our GlobalNumberState, and it's illegal
653-
// to replace a key in ValueMap<GlobalValue *> with a non-global.
654-
GlobalNumbers.erase(G);
655-
// If G's address is not significant, replace it entirely.
656-
Constant *BitcastF = ConstantExpr::getBitCast(F, G->getType());
657-
G->replaceAllUsesWith(BitcastF);
658-
} else {
659-
// Redirect direct callers of G to F. (See note on MergeFunctionsPDI
660-
// above).
661-
replaceDirectCallers(G, F);
662-
}
663-
}
664-
665-
// If G was internal then we may have replaced all uses of G with F. If so,
666-
// stop here and delete G. There's no need for a thunk. (See note on
667-
// MergeFunctionsPDI above).
668-
if (G->hasLocalLinkage() && G->use_empty() && !MergeFunctionsPDI) {
669-
G->eraseFromParent();
670-
return;
671-
}
672-
673-
// Don't merge tiny functions using a thunk, since it can just end up
674-
// making the function larger.
675-
if (F->size() == 1) {
676-
if (F->front().size() <= 2) {
677-
DEBUG(dbgs() << "writeThunk: " << F->getName()
678-
<< " is too small to bother creating a thunk for\n");
679-
return;
680-
}
681-
}
682-
683663
BasicBlock *GEntryBlock = nullptr;
684664
std::vector<Instruction *> PDIUnrelatedWL;
685665
BasicBlock *BB = nullptr;
@@ -754,6 +734,10 @@ void MergeFunctions::mergeTwoFunctions(Function *F, Function *G) {
754734
if (F->isInterposable()) {
755735
assert(G->isInterposable());
756736

737+
if (!isThunkProfitable(F)) {
738+
return;
739+
}
740+
757741
// Make them both thunks to the same internal function.
758742
Function *H = Function::Create(F->getFunctionType(), F->getLinkage(), "",
759743
F->getParent());
@@ -770,11 +754,41 @@ void MergeFunctions::mergeTwoFunctions(Function *F, Function *G) {
770754
F->setAlignment(MaxAlignment);
771755
F->setLinkage(GlobalValue::PrivateLinkage);
772756
++NumDoubleWeak;
757+
++NumFunctionsMerged;
773758
} else {
759+
// For better debugability, under MergeFunctionsPDI, we do not modify G's
760+
// call sites to point to F even when within the same translation unit.
761+
if (!G->isInterposable() && !MergeFunctionsPDI) {
762+
if (G->hasGlobalUnnamedAddr()) {
763+
// G might have been a key in our GlobalNumberState, and it's illegal
764+
// to replace a key in ValueMap<GlobalValue *> with a non-global.
765+
GlobalNumbers.erase(G);
766+
// If G's address is not significant, replace it entirely.
767+
Constant *BitcastF = ConstantExpr::getBitCast(F, G->getType());
768+
G->replaceAllUsesWith(BitcastF);
769+
} else {
770+
// Redirect direct callers of G to F. (See note on MergeFunctionsPDI
771+
// above).
772+
replaceDirectCallers(G, F);
773+
}
774+
}
775+
776+
// If G was internal then we may have replaced all uses of G with F. If so,
777+
// stop here and delete G. There's no need for a thunk. (See note on
778+
// MergeFunctionsPDI above).
779+
if (G->hasLocalLinkage() && G->use_empty() && !MergeFunctionsPDI) {
780+
G->eraseFromParent();
781+
++NumFunctionsMerged;
782+
return;
783+
}
784+
785+
if (!isThunkProfitable(F)) {
786+
return;
787+
}
788+
774789
writeThunk(F, G);
790+
++NumFunctionsMerged;
775791
}
776-
777-
++NumFunctionsMerged;
778792
}
779793

780794
/// Replace function F by function G.
+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
; RUN: opt -mergefunc -S < %s | FileCheck %s
2+
3+
; Weak functions too small for merging to be profitable
4+
5+
; CHECK: define weak i32 @foo(i8*, i32)
6+
; CHECK-NEXT: ret i32 %1
7+
; CHECK: define weak i32 @bar(i8*, i32)
8+
; CHECK-NEXT: ret i32 %1
9+
10+
define weak i32 @foo(i8*, i32) #0 {
11+
ret i32 %1
12+
}
13+
14+
define weak i32 @bar(i8*, i32) #0 {
15+
ret i32 %1
16+
}

0 commit comments

Comments
 (0)