[llvm-branch-commits] [llvm-branch] r332662 - Merging r332342:
Tom Stellard via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Thu May 17 13:44:32 PDT 2018
Author: tstellar
Date: Thu May 17 13:44:32 2018
New Revision: 332662
URL: http://llvm.org/viewvc/llvm-project?rev=332662&view=rev
Log:
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
------------------------------------------------------------------------
Added:
llvm/branches/release_60/test/Transforms/MergeFunc/weak-small.ll
Modified:
llvm/branches/release_60/lib/Transforms/IPO/MergeFunctions.cpp
Modified: llvm/branches/release_60/lib/Transforms/IPO/MergeFunctions.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/branches/release_60/lib/Transforms/IPO/MergeFunctions.cpp?rev=332662&r1=332661&r2=332662&view=diff
==============================================================================
--- llvm/branches/release_60/lib/Transforms/IPO/MergeFunctions.cpp (original)
+++ llvm/branches/release_60/lib/Transforms/IPO/MergeFunctions.cpp Thu May 17 13:44:32 2018
@@ -638,6 +638,19 @@ void MergeFunctions::filterInstsUnrelate
DEBUG(dbgs() << " }\n");
}
+// 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() << "isThunkProfitable: " << 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
// MergeFunctionsPDI holds) replace direct uses of G with bitcast(F),
// delete G. Under MergeFunctionsPDI, we use G itself for creating
@@ -647,39 +660,6 @@ void MergeFunctions::filterInstsUnrelate
// 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.
- 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;
@@ -754,6 +734,10 @@ void MergeFunctions::mergeTwoFunctions(F
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 +754,41 @@ void MergeFunctions::mergeTwoFunctions(F
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.
Added: llvm/branches/release_60/test/Transforms/MergeFunc/weak-small.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/branches/release_60/test/Transforms/MergeFunc/weak-small.ll?rev=332662&view=auto
==============================================================================
--- llvm/branches/release_60/test/Transforms/MergeFunc/weak-small.ll (added)
+++ llvm/branches/release_60/test/Transforms/MergeFunc/weak-small.ll Thu May 17 13:44:32 2018
@@ -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
+}
More information about the llvm-branch-commits
mailing list