Skip to content

[MergeFunc] Remove discardables function before writing alias or thunk. #128865

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 27, 2025
Merged

[MergeFunc] Remove discardables function before writing alias or thunk. #128865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1548cd4
[MergeFunc] Remove discardables function before writing alias or thunk.
fhahn Feb 24, 2025
9697fbf
!fixup fix formatting
fhahn Feb 26, 2025
7e0d0eb
!fixup address latest comments, thanks
fhahn Feb 26, 2025
fb08c27
!fixup Don't use pattern to check unnamed function in call.
fhahn Feb 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 21 additions & 14 deletions llvm/lib/Transforms/IPO/MergeFunctions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,9 +283,10 @@ class MergeFunctions {
// 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 possible.
// Returns false if neither is the case.
bool writeThunkOrAlias(Function *F, Function *G);
// If needed, replace G with an alias to F if possible, or a thunk to F if
// profitable. Returns false if neither is the case. If \p G is not needed
// (i.e. it is discardable and not used), \p G is removed directly.
bool writeThunkOrAliasIfNeeded(Function *F, Function *G);

/// Replace function F with function G in the function tree.
void replaceFunctionInTree(const FunctionNode &FN, Function *G);
Expand Down Expand Up @@ -875,9 +876,14 @@ void MergeFunctions::writeAlias(Function *F, Function *G) {
++NumAliasesWritten;
}

// 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 needed, replace G with an alias to F if possible, or a thunk to F if
// profitable. Returns false if neither is the case. If \p G is not needed (i.e.
// it is discardable and unused), \p G is removed directly.
bool MergeFunctions::writeThunkOrAliasIfNeeded(Function *F, Function *G) {
if (G->isDiscardableIfUnused() && G->use_empty() && !MergeFunctionsPDI) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is G's lack of uses created by this pass? Why not let a subsequent DCE invocation remove it? Also, if MergeFunctionsPDI is enabled, but G is discardable and unused, wouldn't DCE still go ahead and remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is done by the pass, when merging a function we may update all callers of one functions to call another, leaving around some unused functions.

GlobalDCE would pick those up, but MergeFunctions is run after GlobalDCE, presumably to avoid spending time on trying to merge dead functions.

MergeFunc already removes some functions if they become dead.

G->eraseFromParent();
return true;
}
if (canCreateAliasFor(G)) {
writeAlias(F, G);
return true;
Expand All @@ -904,9 +910,10 @@ void MergeFunctions::mergeTwoFunctions(Function *F, Function *G) {
assert((!isODR(G) || isODR(F)) &&
"if G is ODR, F must also be ODR due to ordering");

// 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.
// Both writeThunkOrAliasIfNeeded() 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 (!canCreateThunkFor(F) &&
(!canCreateAliasFor(F) || !canCreateAliasFor(G)))
return;
Expand All @@ -930,13 +937,13 @@ void MergeFunctions::mergeTwoFunctions(Function *F, Function *G) {
if (isODR(F))
replaceDirectCallers(NewF, F);

// We collect alignment before writeThunkOrAlias that overwrites NewF and
// G's content.
// We collect alignment before writeThunkOrAliasIfNeeded that overwrites
// NewF and G's content.
const MaybeAlign NewFAlign = NewF->getAlign();
const MaybeAlign GAlign = G->getAlign();

writeThunkOrAlias(F, G);
writeThunkOrAlias(F, NewF);
writeThunkOrAliasIfNeeded(F, G);
writeThunkOrAliasIfNeeded(F, NewF);

if (NewFAlign || GAlign)
F->setAlignment(std::max(NewFAlign.valueOrOne(), GAlign.valueOrOne()));
Expand Down Expand Up @@ -975,7 +982,7 @@ void MergeFunctions::mergeTwoFunctions(Function *F, Function *G) {
return;
}

if (writeThunkOrAlias(F, G)) {
if (writeThunkOrAliasIfNeeded(F, G)) {
++NumFunctionsMerged;
}
}
Expand Down
11 changes: 2 additions & 9 deletions llvm/test/Transforms/MergeFunc/linkonce.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-globals all --include-generated-funcs --version 5
; RUN: opt -S -passes=mergefunc < %s | FileCheck %s -implicit-check-not=funC
; RUN: opt -S -passes=mergefunc < %s | FileCheck %s -implicit-check-not=funA -implicit-check-not=funC

; Replacments should be totally ordered on the function name.
; If we don't do this we can end up with one module defining a thunk for @funA
Expand Down Expand Up @@ -43,14 +42,8 @@ define linkonce i32 @funA(i32 %x, i32 %y) {
; CHECK-NEXT: ret i32 [[SUM3]]
;
;
; CHECK-LABEL: define linkonce i32 @funC(
; CHECK-SAME: i32 [[TMP0:%.*]], i32 [[TMP1:%.*]]) {
; CHECK-NEXT: [[TMP3:%.*]] = tail call i32 @[[GLOB0:[0-9]+]](i32 [[TMP0]], i32 [[TMP1]])
; CHECK-NEXT: ret i32 [[TMP3]]
;
;
; CHECK-LABEL: define linkonce i32 @funB(
; CHECK-SAME: i32 [[TMP0:%.*]], i32 [[TMP1:%.*]]) {
; CHECK-NEXT: [[TMP3:%.*]] = tail call i32 @[[GLOB0]](i32 [[TMP0]], i32 [[TMP1]])
; CHECK-NEXT: [[TMP3:%.*]] = tail call i32 @0(i32 [[TMP0]], i32 [[TMP1]])
; CHECK-NEXT: ret i32 [[TMP3]]
;
11 changes: 2 additions & 9 deletions llvm/test/Transforms/MergeFunc/linkonce_odr.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-globals all --include-generated-funcs --version 5
; RUN: opt -S -passes=mergefunc < %s | FileCheck %s -implicit-check-not=funC
; RUN: opt -S -passes=mergefunc < %s | FileCheck %s -implicit-check-not=funA -implicit-check-not=funC

; Replacments should be totally ordered on the function name.
; If we don't do this we can end up with one module defining a thunk for @funA
Expand Down Expand Up @@ -43,14 +42,8 @@ define linkonce_odr i32 @funA(i32 %x, i32 %y) {
; CHECK-NEXT: ret i32 [[SUM3]]
;
;
; CHECK-LABEL: define linkonce_odr i32 @funC(
; CHECK-SAME: i32 [[TMP0:%.*]], i32 [[TMP1:%.*]]) {
; CHECK-NEXT: [[TMP3:%.*]] = tail call i32 @[[GLOB0:[0-9]+]](i32 [[TMP0]], i32 [[TMP1]])
; CHECK-NEXT: ret i32 [[TMP3]]
;
;
; CHECK-LABEL: define linkonce_odr i32 @funB(
; CHECK-SAME: i32 [[TMP0:%.*]], i32 [[TMP1:%.*]]) {
; CHECK-NEXT: [[TMP3:%.*]] = tail call i32 @[[GLOB0]](i32 [[TMP0]], i32 [[TMP1]])
; CHECK-NEXT: [[TMP3:%.*]] = tail call i32 @0(i32 [[TMP0]], i32 [[TMP1]])
; CHECK-NEXT: ret i32 [[TMP3]]
;
14 changes: 1 addition & 13 deletions llvm/test/Transforms/MergeFunc/merge-linkonce-odr.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-globals all --include-generated-funcs --version 5
; RUN: opt -p mergefunc -S %s | FileCheck %s
; RUN: opt -p mergefunc -S %s | FileCheck --implicit-check-not=linkonce_odr_caller_of_foo_2 --implicit-check-not=linkonce_odr_caller_of_foo_1 %s

define void @caller_of_callers(ptr %p) {
call void @linkonce_odr_caller_of_foo_1(ptr %p)
Expand Down Expand Up @@ -125,15 +125,3 @@ declare void @zar(ptr)
; CHECK-NEXT: tail call void @zar(ptr [[P]])
; CHECK-NEXT: ret void
;
;
; CHECK-LABEL: define linkonce_odr hidden void @linkonce_odr_caller_of_foo_2(
; CHECK-SAME: ptr [[TMP0:%.*]]) {
; CHECK-NEXT: tail call void @[[GLOB0]](ptr [[TMP0]])
; CHECK-NEXT: ret void
;
;
; CHECK-LABEL: define linkonce_odr hidden void @linkonce_odr_caller_of_foo_1(
; CHECK-SAME: ptr [[TMP0:%.*]]) {
; CHECK-NEXT: tail call void @[[GLOB0]](ptr [[TMP0]])
; CHECK-NEXT: ret void
;