CloneFunction: Do not delete blocks with address taken #134209
Conversation
If a block with a single predecessor also had its address taken, it was getting deleted in this post-inline cleanup step. This would result in the blockaddress in the resulting function getting deleted and replaced with inttoptr 1. This fixes one of (at least?) two bugs required to permit inlining of functions with blockaddress uses. At the moment this is not testable (at least without an annoyingly complex unit test), and is a pre-bug fix for future patches. Functions with blockaddress uses are rejected in isInlineViable, so we don't get this far with the current InlineFunction uses (some of the existing cases seem to reproduce this part of the rejection logic, like PartialInliner). This will be tested in a pending llvm-reduce change. Prerequisite for #38908
This stack of pull requests is managed by Graphite. Learn more about stacking. |
arsenm
requested review from
aeubanks,
bwendling,
dexonsmith,
fhahn,
jdoerfert,
nickdesaulniers,
nikic and
regehr
|
@llvm/pr-subscribers-llvm-transforms Author: Matt Arsenault (arsenm) ChangesIf a block with a single predecessor also had its address taken, This fixes one of (at least?) two bugs required to permit inlining of At the moment this is not testable (at least without an annoyingly complex Prerequisite for #38908 Full diff: https://github.com/llvm/llvm-project/pull/134209.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/Utils/CloneFunction.cpp b/llvm/lib/Transforms/Utils/CloneFunction.cpp
index e58585705e82f..9387797019023 100644
--- a/llvm/lib/Transforms/Utils/CloneFunction.cpp
+++ b/llvm/lib/Transforms/Utils/CloneFunction.cpp
@@ -928,7 +928,7 @@ void llvm::CloneAndPruneIntoFromInst(Function *NewFunc, const Function *OldFunc,
}
BasicBlock *Dest = BI->getSuccessor(0);
- if (!Dest->getSinglePredecessor()) {
+ if (!Dest->getSinglePredecessor() || Dest->hasAddressTaken()) {
++I;
continue;
}
|
There was a problem hiding this comment.
LGTM, in that this is consistent with MergeBlockIntoPredecessor behavior.
(I think technically it is legal to ignore the blockaddress in this case, because it is only specified to be meaningful in conjunction with indirectbr, where this fold would not apply. But given the ways the linux kernel abuses block labels, that ship has already sailed.)
For the ultimate usage, but it still needs to be passed around as an opaque value to be delivered to an eventual indirectbr use. The langref also says you can use as an inlineasm operand |
This might have been just one bug, the main issue I was trying to fix disappears with this alone |
|
LGTM! |
arsenm
deleted the
users/arsenm/clone-function-fix-folding-blocks-with-address-taken
branch
| @@ -928,7 +928,7 @@ void llvm::CloneAndPruneIntoFromInst(Function *NewFunc, const Function *OldFunc, | |||
| } | |||
|
|
|||
| BasicBlock *Dest = BI->getSuccessor(0); | |||
| if (!Dest->getSinglePredecessor()) { | |||
| if (!Dest->getSinglePredecessor() || Dest->hasAddressTaken()) { | |||
There was a problem hiding this comment.
perhaps worth updating the comment block on L918-L921?
If a block with a single predecessor also had its address taken,
it was getting deleted in this post-inline cleanup step. This would
result in the blockaddress in the resulting function getting deleted
and replaced with inttoptr 1.
This fixes one bug required to permit inlining of functions with blockaddress
uses.
At the moment this is not testable (at least without an annoyingly complex
unit test), and is a pre-bug fix for future patches. Functions with
blockaddress uses are rejected in isInlineViable, so we don't get this far
with the current InlineFunction uses (some of the existing cases seem to
reproduce this part of the rejection logic, like PartialInliner). This
will be tested in a pending llvm-reduce change.
Prerequisite for #38908