[llvm] 31ec0a6 - [SimplifyCFG] Improve the way hoisting skips over non-matching instructions
Jay Foad via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 28 02:03:42 PDT 2023
Author: Jay Foad
Date: 2023-04-28T10:03:32+01:00
New Revision: 31ec0a6845299dc3486c1339d40342d858a42e3f
URL: https://github.com/llvm/llvm-project/commit/31ec0a6845299dc3486c1339d40342d858a42e3f
DIFF: https://github.com/llvm/llvm-project/commit/31ec0a6845299dc3486c1339d40342d858a42e3f.diff
LOG: [SimplifyCFG] Improve the way hoisting skips over non-matching instructions
D129370 introduced the idea that hoisting could skip over non-matching
instructions and continue to look for matching (hoistable) instructions,
but certain types of mismatch still aborted the whole hoisting attempt.
Fix this by splitting out some of the instruction matching checks into a
helper function.
Also forbid hoisting allocas past stacksave/stackrestore, completing the
fix started in D133730, to avoid regressing tests.
Differential Revision: https://reviews.llvm.org/D149365
Added:
Modified:
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
llvm/test/Transforms/SimplifyCFG/convergent.ll
Removed:
################################################################################
diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index 4128aacff1b4c..2dfc16fecc880 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -1456,7 +1456,7 @@ static bool isSafeToHoistInstr(Instruction *I, unsigned Flags) {
// If we have seen an instruction with side effects, it's unsafe to reorder an
// instruction which reads memory or itself has side effects.
if ((Flags & SkipSideEffect) &&
- (I->mayReadFromMemory() || I->mayHaveSideEffects()))
+ (I->mayReadFromMemory() || I->mayHaveSideEffects() || isa<AllocaInst>(I)))
return false;
// Reordering across an instruction which does not necessarily transfer
@@ -1484,6 +1484,37 @@ static bool isSafeToHoistInstr(Instruction *I, unsigned Flags) {
static bool passingValueIsAlwaysUndefined(Value *V, Instruction *I, bool PtrValueMayBeModified = false);
+/// Helper function for HoistThenElseCodeToIf. Return true if identical
+/// instructions \p I1 and \p I2 can and should be hoisted.
+static bool shouldHoistCommonInstructions(Instruction *I1, Instruction *I2,
+ const TargetTransformInfo &TTI) {
+ // If we're going to hoist a call, make sure that the two instructions
+ // we're commoning/hoisting are both marked with musttail, or neither of
+ // them is marked as such. Otherwise, we might end up in a situation where
+ // we hoist from a block where the terminator is a `ret` to a block where
+ // the terminator is a `br`, and `musttail` calls expect to be followed by
+ // a return.
+ auto *C1 = dyn_cast<CallInst>(I1);
+ auto *C2 = dyn_cast<CallInst>(I2);
+ if (C1 && C2)
+ if (C1->isMustTailCall() != C2->isMustTailCall())
+ return false;
+
+ if (!TTI.isProfitableToHoist(I1) || !TTI.isProfitableToHoist(I2))
+ return false;
+
+ // If any of the two call sites has nomerge or convergent attribute, stop
+ // hoisting.
+ if (const auto *CB1 = dyn_cast<CallBase>(I1))
+ if (CB1->cannotMerge() || CB1->isConvergent())
+ return false;
+ if (const auto *CB2 = dyn_cast<CallBase>(I2))
+ if (CB2->cannotMerge() || CB2->isConvergent())
+ return false;
+
+ return true;
+}
+
/// Given a conditional branch that goes to BB1 and BB2, hoist any common code
/// in the two blocks up into the branch block. The caller of this function
/// guarantees that BI's block dominates BB1 and BB2. If EqTermsOnly is given,
@@ -1564,38 +1595,13 @@ bool SimplifyCFGOpt::HoistThenElseCodeToIf(BranchInst *BI, bool EqTermsOnly) {
goto HoistTerminator;
}
- if (I1->isIdenticalToWhenDefined(I2)) {
- // Even if the instructions are identical, it may not be safe to hoist
- // them if we have skipped over instructions with side effects or their
- // operands weren't hoisted.
- if (!isSafeToHoistInstr(I1, SkipFlagsBB1) ||
- !isSafeToHoistInstr(I2, SkipFlagsBB2))
- return Changed;
-
- // If we're going to hoist a call, make sure that the two instructions
- // we're commoning/hoisting are both marked with musttail, or neither of
- // them is marked as such. Otherwise, we might end up in a situation where
- // we hoist from a block where the terminator is a `ret` to a block where
- // the terminator is a `br`, and `musttail` calls expect to be followed by
- // a return.
- auto *C1 = dyn_cast<CallInst>(I1);
- auto *C2 = dyn_cast<CallInst>(I2);
- if (C1 && C2)
- if (C1->isMustTailCall() != C2->isMustTailCall())
- return Changed;
-
- if (!TTI.isProfitableToHoist(I1) || !TTI.isProfitableToHoist(I2))
- return Changed;
-
- // If any of the two call sites has nomerge or convergent attribute, stop
- // hoisting.
- if (const auto *CB1 = dyn_cast<CallBase>(I1))
- if (CB1->cannotMerge() || CB1->isConvergent())
- return Changed;
- if (const auto *CB2 = dyn_cast<CallBase>(I2))
- if (CB2->cannotMerge() || CB2->isConvergent())
- return Changed;
-
+ if (I1->isIdenticalToWhenDefined(I2) &&
+ // Even if the instructions are identical, it may not be safe to hoist
+ // them if we have skipped over instructions with side effects or their
+ // operands weren't hoisted.
+ isSafeToHoistInstr(I1, SkipFlagsBB1) &&
+ isSafeToHoistInstr(I2, SkipFlagsBB2) &&
+ shouldHoistCommonInstructions(I1, I2, TTI)) {
if (isa<DbgInfoIntrinsic>(I1) || isa<DbgInfoIntrinsic>(I2)) {
assert(isa<DbgInfoIntrinsic>(I1) && isa<DbgInfoIntrinsic>(I2));
// The debug location is an integral part of a debug info intrinsic
diff --git a/llvm/test/Transforms/SimplifyCFG/convergent.ll b/llvm/test/Transforms/SimplifyCFG/convergent.ll
index bbdba53524806..6ba51e06460c2 100644
--- a/llvm/test/Transforms/SimplifyCFG/convergent.ll
+++ b/llvm/test/Transforms/SimplifyCFG/convergent.ll
@@ -82,6 +82,8 @@ define void @test_02(ptr %y.coerce) convergent {
; SINK-NEXT: [[TMP0:%.*]] = tail call i32 @tid()
; SINK-NEXT: [[REM:%.*]] = and i32 [[TMP0]], 1
; SINK-NEXT: [[CMP_NOT:%.*]] = icmp eq i32 [[REM]], 0
+; SINK-NEXT: [[IDXPROM4:%.*]] = zext i32 [[TMP0]] to i64
+; SINK-NEXT: [[ARRAYIDX5:%.*]] = getelementptr inbounds i32, ptr [[Y_COERCE:%.*]], i64 [[IDXPROM4]]
; SINK-NEXT: br i1 [[CMP_NOT]], label [[IF_ELSE:%.*]], label [[IF_THEN:%.*]]
; SINK: if.then:
; SINK-NEXT: [[TMP1:%.*]] = tail call i32 @mbcnt(i32 -1, i32 0)
@@ -101,8 +103,6 @@ define void @test_02(ptr %y.coerce) convergent {
; SINK-NEXT: br label [[IF_END]]
; SINK: if.end:
; SINK-NEXT: [[DOTSINK:%.*]] = phi i32 [ [[TMP6]], [[IF_ELSE]] ], [ [[TMP3]], [[IF_THEN]] ]
-; SINK-NEXT: [[IDXPROM4:%.*]] = zext i32 [[TMP0]] to i64
-; SINK-NEXT: [[ARRAYIDX5:%.*]] = getelementptr inbounds i32, ptr [[Y_COERCE:%.*]], i64 [[IDXPROM4]]
; SINK-NEXT: store i32 [[DOTSINK]], ptr [[ARRAYIDX5]], align 4
; SINK-NEXT: ret void
;
More information about the llvm-commits
mailing list