[llvm] [GuardWidening] Widen widenable conditions instead of widenable branches (PR #66501)
via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 15 05:28:01 PDT 2023
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-llvm-analysis
<details>
<summary>Changes</summary>
None
--
Patch is 82.93 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/66501.diff
19 Files Affected:
- (modified) llvm/include/llvm/Analysis/GuardUtils.h (+2-19)
- (modified) llvm/include/llvm/Transforms/Utils/GuardUtils.h (+4-9)
- (modified) llvm/lib/Analysis/GuardUtils.cpp (+3-66)
- (modified) llvm/lib/Transforms/Scalar/GuardWidening.cpp (+20-41)
- (modified) llvm/lib/Transforms/Scalar/LoopPredication.cpp (+14-23)
- (modified) llvm/lib/Transforms/Utils/GuardUtils.cpp (+8-45)
- (modified) llvm/test/Transforms/GuardWidening/basic-loop.ll (+4-2)
- (modified) llvm/test/Transforms/GuardWidening/basic_widenable_condition_guards.ll (+24-25)
- (modified) llvm/test/Transforms/GuardWidening/hoist-checks.ll (+4-2)
- (modified) llvm/test/Transforms/GuardWidening/loop_invariant_widenable_condition.ll (+4-5)
- (modified) llvm/test/Transforms/GuardWidening/mixed_guards.ll (+2-2)
- (modified) llvm/test/Transforms/GuardWidening/profile-based-profitability_explicit.ll (+10-10)
- (modified) llvm/test/Transforms/GuardWidening/range-check-merging.ll (+18-11)
- (modified) llvm/test/Transforms/GuardWidening/two_forms_behavior_consistency.ll (+13-10)
- (modified) llvm/test/Transforms/GuardWidening/widen-cond-with-operands.ll (+2-2)
- (modified) llvm/test/Transforms/LoopPredication/assumes.ll (+1-1)
- (modified) llvm/test/Transforms/LoopPredication/basic_widenable_branch_guards.ll (+80-26)
- (modified) llvm/test/Transforms/LoopPredication/pr61022.ll (+1-1)
- (modified) llvm/test/Transforms/LoopPredication/predicate-exits.ll (+36-36)
<pre>
diff --git a/llvm/include/llvm/Analysis/GuardUtils.h b/llvm/include/llvm/Analysis/GuardUtils.h
index 208f08b82d98731..585a39f837ba81b 100644
--- a/llvm/include/llvm/Analysis/GuardUtils.h
+++ b/llvm/include/llvm/Analysis/GuardUtils.h
@@ -36,30 +36,13 @@ bool isWidenableBranch(const User *U);
/// widenable conditional branch to deopt block.
bool isGuardAsWidenableBranch(const User *U);
-/// If U is widenable branch looking like:
-/// %cond = ...
-/// %wc = call i1 @llvm.experimental.widenable.condition()
-/// %branch_cond = and i1 %cond, %wc
-/// br i1 %branch_cond, label %if_true_bb, label %if_false_bb ; <--- U
-/// The function returns true, and the values %cond and %wc and blocks
-/// %if_true_bb, if_false_bb are returned in
-/// the parameters (Condition, WidenableCondition, IfTrueBB and IfFalseFF)
-/// respectively. If \p U does not match this pattern, return false.
-bool parseWidenableBranch(const User *U, Value *&Condition,
- Value *&WidenableCondition, BasicBlock *&IfTrueBB,
- BasicBlock *&IfFalseBB);
-
-/// Analogous to the above, but return the Uses so that they can be
-/// modified. Unlike previous version, Condition is optional and may be null.
-bool parseWidenableBranch(User *U, Use *&Cond, Use *&WC, BasicBlock *&IfTrueBB,
- BasicBlock *&IfFalseBB);
-
// The guard condition is expected to be in form of:
// cond1 && cond2 && cond3 ...
// or in case of widenable branch:
// cond1 && cond2 && cond3 && widenable_contidion ...
// Method collects the list of checks, but skips widenable_condition.
-void parseWidenableGuard(const User *U, llvm::SmallVectorImpl<Value *> &Checks);
+void parseWidenableGuard(const User *U, SmallVectorImpl<Value *> &Checks,
+ bool CollectWidenableConditions = false);
// Returns widenable_condition if it exists in the expression tree rooting from
// \p U and has only one use.
diff --git a/llvm/include/llvm/Transforms/Utils/GuardUtils.h b/llvm/include/llvm/Transforms/Utils/GuardUtils.h
index 7ab5d9ef4f23895..6e9100229fb043c 100644
--- a/llvm/include/llvm/Transforms/Utils/GuardUtils.h
+++ b/llvm/include/llvm/Transforms/Utils/GuardUtils.h
@@ -17,6 +17,7 @@ namespace llvm {
class BranchInst;
class CallInst;
class Function;
+class Instruction;
class Value;
/// Splits control flow at point of \p Guard, replacing it with explicit branch
@@ -29,15 +30,9 @@ class Value;
void makeGuardControlFlowExplicit(Function *DeoptIntrinsic, CallInst *Guard,
bool UseWC);
-/// Given a branch we know is widenable (defined per Analysis/GuardUtils.h),
-/// widen it such that condition 'NewCond' is also known to hold on the taken
-/// path. Branch remains widenable after transform.
-void widenWidenableBranch(BranchInst *WidenableBR, Value *NewCond);
-
-/// Given a branch we know is widenable (defined per Analysis/GuardUtils.h),
-/// *set* it's condition such that (only) 'Cond' is known to hold on the taken
-/// path and that the branch remains widenable after transform.
-void setWidenableBranchCond(BranchInst *WidenableBR, Value *Cond);
+/// Widen \p WidenableCondition with a \p NewCond by replacing its use with a
+/// 'WidenableCondition and NewCond' inserted right after \p WidenableCondition.
+void widenWidenableCondition(Instruction *WidenableCondition, Value *NewCond);
} // llvm
diff --git a/llvm/lib/Analysis/GuardUtils.cpp b/llvm/lib/Analysis/GuardUtils.cpp
index 83734b7eac0c823..ce5fadf35876eaf 100644
--- a/llvm/lib/Analysis/GuardUtils.cpp
+++ b/llvm/lib/Analysis/GuardUtils.cpp
@@ -47,70 +47,6 @@ bool llvm::isGuardAsWidenableBranch(const User *U) {
return false;
}
-bool llvm::parseWidenableBranch(const User *U, Value *&Condition,
- Value *&WidenableCondition,
- BasicBlock *&IfTrueBB, BasicBlock *&IfFalseBB) {
-
- Use *C, *WC;
- if (parseWidenableBranch(const_cast<User*>(U), C, WC, IfTrueBB, IfFalseBB)) {
- if (C)
- Condition = C->get();
- else
- Condition = ConstantInt::getTrue(IfTrueBB->getContext());
- WidenableCondition = WC->get();
- return true;
- }
- return false;
-}
-
-bool llvm::parseWidenableBranch(User *U, Use *&C,Use *&WC,
- BasicBlock *&IfTrueBB, BasicBlock *&IfFalseBB) {
-
- auto *BI = dyn_cast<BranchInst>(U);
- if (!BI || !BI->isConditional())
- return false;
- auto *Cond = BI->getCondition();
- if (!Cond->hasOneUse())
- return false;
-
- IfTrueBB = BI->getSuccessor(0);
- IfFalseBB = BI->getSuccessor(1);
-
- if (match(Cond, m_Intrinsic<Intrinsic::experimental_widenable_condition>())) {
- WC = &BI->getOperandUse(0);
- C = nullptr;
- return true;
- }
-
- // Check for two cases:
- // 1) br (i1 (and A, WC())), label %IfTrue, label %IfFalse
- // 2) br (i1 (and WC(), B)), label %IfTrue, label %IfFalse
- // We do not check for more generalized and trees as we should canonicalize
- // to the form above in instcombine. (TODO)
- Value *A, *B;
- if (!match(Cond, m_And(m_Value(A), m_Value(B))))
- return false;
- auto *And = dyn_cast<Instruction>(Cond);
- if (!And)
- // Could be a constexpr
- return false;
-
- if (match(A, m_Intrinsic<Intrinsic::experimental_widenable_condition>()) &&
- A->hasOneUse()) {
- WC = &And->getOperandUse(0);
- C = &And->getOperandUse(1);
- return true;
- }
-
- if (match(B, m_Intrinsic<Intrinsic::experimental_widenable_condition>()) &&
- B->hasOneUse()) {
- WC = &And->getOperandUse(1);
- C = &And->getOperandUse(0);
- return true;
- }
- return false;
-}
-
template <typename CallbackType>
static void parseCondition(Value *Condition,
CallbackType RecordCheckOrWidenableCond) {
@@ -133,13 +69,14 @@ static void parseCondition(Value *Condition,
}
void llvm::parseWidenableGuard(const User *U,
- llvm::SmallVectorImpl<Value *> &Checks) {
+ llvm::SmallVectorImpl<Value *> &Checks,
+ bool CollectWidenableConditions) {
assert((isGuard(U) || isWidenableBranch(U)) && "Should be");
Value *Condition = isGuard(U) ? cast<IntrinsicInst>(U)->getArgOperand(0)
: cast<BranchInst>(U)->getCondition();
parseCondition(Condition, [&](Value *Check) {
- if (!isWidenableCondition(Check))
+ if (!isWidenableCondition(Check) || CollectWidenableConditions)
Checks.push_back(Check);
return true;
});
diff --git a/llvm/lib/Transforms/Scalar/GuardWidening.cpp b/llvm/lib/Transforms/Scalar/GuardWidening.cpp
index fb1db11e85a0b8f..af2514f1b36ec1c 100644
--- a/llvm/lib/Transforms/Scalar/GuardWidening.cpp
+++ b/llvm/lib/Transforms/Scalar/GuardWidening.cpp
@@ -80,21 +80,6 @@ static cl::opt<bool>
namespace {
-// Get the condition of \p I. It can either be a guard or a conditional branch.
-static Value *getCondition(Instruction *I) {
- if (IntrinsicInst *GI = dyn_cast<IntrinsicInst>(I)) {
- assert(GI->getIntrinsicID() == Intrinsic::experimental_guard &&
- "Bad guard intrinsic?");
- return GI->getArgOperand(0);
- }
- Value *Cond, *WC;
- BasicBlock *IfTrueBB, *IfFalseBB;
- if (parseWidenableBranch(I, Cond, WC, IfTrueBB, IfFalseBB))
- return Cond;
-
- return cast<BranchInst>(I)->getCondition();
-}
-
// Set the condition for \p I to \p NewCond. \p I can either be a guard or a
// conditional branch.
static void setCondition(Instruction *I, Value *NewCond) {
@@ -225,8 +210,9 @@ class GuardWideningImpl {
/// Generate the logical AND of \p ChecksToHoist and \p OldCondition and make
/// it available at InsertPt
- Value *hoistChecks(SmallVectorImpl<Value *> &ChecksToHoist,
- Value *OldCondition, Instruction *InsertPt);
+ void hoistChecks(SmallVectorImpl<Value *> &ChecksToHoist,
+ SmallVectorImpl<Value *> &ChecksToWiden,
+ Instruction *InsertPt);
/// Adds freeze to Orig and push it as far as possible very aggressively.
/// Also replaces all uses of frozen instruction with frozen version.
@@ -305,16 +291,10 @@ class GuardWideningImpl {
SmallVectorImpl<Value *> &ChecksToWiden,
Instruction *ToWiden) {
Instruction *InsertPt = findInsertionPointForWideCondition(ToWiden);
- auto MergedCheck = mergeChecks(ChecksToHoist, ChecksToWiden, InsertPt);
- Value *Result = MergedCheck ? *MergedCheck
- : hoistChecks(ChecksToHoist,
- getCondition(ToWiden), InsertPt);
-
- if (isGuardAsWidenableBranch(ToWiden)) {
- setWidenableBranchCond(cast<BranchInst>(ToWiden), Result);
- return;
- }
- setCondition(ToWiden, Result);
+ if (auto MergedCheck = mergeChecks(ChecksToHoist, ChecksToWiden, InsertPt))
+ setCondition(ToWiden, *MergedCheck);
+ else
+ hoistChecks(ChecksToHoist, ChecksToWiden, InsertPt);
}
public:
@@ -360,7 +340,7 @@ bool GuardWideningImpl::run() {
assert(EliminatedGuardsAndBranches.empty() || Changed);
for (auto *I : EliminatedGuardsAndBranches)
if (!WidenedGuards.count(I)) {
- assert(isa<ConstantInt>(getCondition(I)) && "Should be!");
+ setCondition(I, ConstantInt::getTrue(I->getContext()));
if (isSupportedGuardInstruction(I))
eliminateGuard(I, MSSAU);
else {
@@ -449,8 +429,6 @@ bool GuardWideningImpl::eliminateInstrViaWidening(
SmallVector<Value *> ChecksToWiden;
parseWidenableGuard(BestSoFar, ChecksToWiden);
widenGuard(ChecksToHoist, ChecksToWiden, BestSoFar);
- auto NewGuardCondition = ConstantInt::getTrue(Instr->getContext());
- setCondition(Instr, NewGuardCondition);
EliminatedGuardsAndBranches.push_back(Instr);
WidenedGuards.insert(BestSoFar);
return true;
@@ -476,10 +454,7 @@ GuardWideningImpl::WideningScore GuardWideningImpl::computeWideningScore(
if (!canBeHoistedTo(ChecksToHoist, WideningPoint))
return WS_IllegalOrNegative;
- // Further in the GuardWideningImpl::hoistChecks the entire condition might be
- // widened, not the parsed list of checks. So we need to check the possibility
- // of that condition hoisting.
- if (!canBeHoistedTo(getCondition(ToWiden), WideningPoint))
+ if (!canBeHoistedTo(ChecksToWiden, WideningPoint))
return WS_IllegalOrNegative;
// If the guard was conditional executed, it may never be reached
@@ -771,18 +746,22 @@ GuardWideningImpl::mergeChecks(SmallVectorImpl<Value *> &ChecksToHoist,
return std::nullopt;
}
-Value *GuardWideningImpl::hoistChecks(SmallVectorImpl<Value *> &ChecksToHoist,
- Value *OldCondition,
- Instruction *InsertPt) {
+void GuardWideningImpl::hoistChecks(SmallVectorImpl<Value *> &ChecksToHoist,
+ SmallVectorImpl<Value *> &ChecksToWiden,
+ Instruction *InsertPt) {
assert(!ChecksToHoist.empty());
IRBuilder<> Builder(InsertPt);
makeAvailableAt(ChecksToHoist, InsertPt);
- makeAvailableAt(OldCondition, InsertPt);
Value *Result = Builder.CreateAnd(ChecksToHoist);
Result = freezeAndPush(Result, InsertPt);
- Result = Builder.CreateAnd(OldCondition, Result);
- Result->setName("wide.chk");
- return Result;
+ if (isGuard(InsertPt)) {
+ makeAvailableAt(ChecksToWiden, InsertPt);
+ auto Result2 = Builder.CreateAnd(ChecksToWiden);
+ Result = Builder.CreateAnd(Result2, Result);
+ Result->setName("wide.chk");
+ setCondition(InsertPt, Result);
+ } else
+ widenWidenableCondition(InsertPt, Result);
}
bool GuardWideningImpl::parseRangeChecks(
diff --git a/llvm/lib/Transforms/Scalar/LoopPredication.cpp b/llvm/lib/Transforms/Scalar/LoopPredication.cpp
index a58ab093a1f75d3..8a6ee932e7f1168 100644
--- a/llvm/lib/Transforms/Scalar/LoopPredication.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopPredication.cpp
@@ -805,11 +805,7 @@ bool LoopPredication::widenWidenableBranchGuardConditions(
TotalConsidered++;
SmallVector<Value *, 4> Checks;
SmallVector<Value *> WidenedChecks;
- parseWidenableGuard(BI, Checks);
- // At the moment, our matching logic for wideable conditions implicitly
- // assumes we preserve the form: (br (and Cond, WC())). FIXME
- auto WC = extractWidenableCondition(BI);
- Checks.push_back(WC);
+ parseWidenableGuard(BI, Checks, true);
widenChecks(Checks, WidenedChecks, Expander, BI);
if (WidenedChecks.empty())
return false;
@@ -1009,7 +1005,7 @@ bool LoopPredication::isLoopProfitableToPredicate() {
/// If we can (cheaply) find a widenable branch which controls entry into the
/// loop, return it.
-static BranchInst *FindWidenableTerminatorAboveLoop(Loop *L, LoopInfo &LI) {
+static IntrinsicInst *FindWidenableTerminatorAboveLoop(Loop *L, LoopInfo &LI) {
// Walk back through any unconditional executed blocks and see if we can find
// a widenable condition which seems to control execution of this loop. Note
// that we predict that maythrow calls are likely untaken and thus that it's
@@ -1028,11 +1024,11 @@ static BranchInst *FindWidenableTerminatorAboveLoop(Loop *L, LoopInfo &LI) {
break;
} while (true);
- if (BasicBlock *Pred = BB->getSinglePredecessor()) {
+ if (BasicBlock *Pred = BB->getSinglePredecessor())
if (auto *BI = dyn_cast<BranchInst>(Pred->getTerminator()))
- if (BI->getSuccessor(0) == BB && isWidenableBranch(BI))
- return BI;
- }
+ if (BI->getSuccessor(0) == BB)
+ if (auto WC = extractWidenableCondition(BI))
+ return cast<IntrinsicInst>(WC);
return nullptr;
}
@@ -1095,8 +1091,8 @@ bool LoopPredication::predicateLoopExits(Loop *L, SCEVExpander &Rewriter) {
if (!Latch)
return false;
- auto *WidenableBR = FindWidenableTerminatorAboveLoop(L, *LI);
- if (!WidenableBR)
+ auto *WidenableCondition = FindWidenableTerminatorAboveLoop(L, *LI);
+ if (!WidenableCondition)
return false;
const SCEV *LatchEC = SE->getExitCount(L, Latch);
@@ -1130,11 +1126,6 @@ bool LoopPredication::predicateLoopExits(Loop *L, SCEVExpander &Rewriter) {
if (ChangedLoop)
SE->forgetLoop(L);
- // The insertion point for the widening should be at the widenably call, not
- // at the WidenableBR. If we do this at the widenableBR, we can incorrectly
- // change a loop-invariant condition to a loop-varying one.
- auto *IP = cast<Instruction>(WidenableBR->getCondition());
-
// The use of umin(all analyzeable exits) instead of latch is subtle, but
// important for profitability. We may have a loop which hasn't been fully
// canonicalized just yet. If the exit we chose to widen is provably never
@@ -1144,11 +1135,11 @@ bool LoopPredication::predicateLoopExits(Loop *L, SCEVExpander &Rewriter) {
const SCEV *MinEC = getMinAnalyzeableBackedgeTakenCount(*SE, *DT, L);
if (isa<SCEVCouldNotCompute>(MinEC) || MinEC->getType()->isPointerTy() ||
!SE->isLoopInvariant(MinEC, L) ||
- !Rewriter.isSafeToExpandAt(MinEC, IP))
+ !Rewriter.isSafeToExpandAt(MinEC, WidenableCondition))
return ChangedLoop;
- Rewriter.setInsertPoint(IP);
- IRBuilder<> B(IP);
+ Rewriter.setInsertPoint(WidenableCondition);
+ IRBuilder<> B(WidenableCondition);
bool InvalidateLoop = false;
Value *MinECV = nullptr; // lazily generated if needed
@@ -1171,7 +1162,7 @@ bool LoopPredication::predicateLoopExits(Loop *L, SCEVExpander &Rewriter) {
const SCEV *ExitCount = SE->getExitCount(L, ExitingBB);
if (isa<SCEVCouldNotCompute>(ExitCount) ||
ExitCount->getType()->isPointerTy() ||
- !Rewriter.isSafeToExpandAt(ExitCount, WidenableBR))
+ !Rewriter.isSafeToExpandAt(ExitCount, WidenableCondition))
continue;
const bool ExitIfTrue = !L->contains(*succ_begin(ExitingBB));
@@ -1203,7 +1194,7 @@ bool LoopPredication::predicateLoopExits(Loop *L, SCEVExpander &Rewriter) {
// context.
NewCond = B.CreateFreeze(NewCond);
- widenWidenableBranch(WidenableBR, NewCond);
+ widenWidenableCondition(WidenableCondition, NewCond);
Value *OldCond = BI->getCondition();
BI->setCondition(ConstantInt::get(OldCond->getType(), !ExitIfTrue));
@@ -1217,7 +1208,7 @@ bool LoopPredication::predicateLoopExits(Loop *L, SCEVExpander &Rewriter) {
// should be removed next time the CFG is modified.
SE->forgetLoop(L);
- // Always return `true` since we have moved the WidenableBR's condition.
+ // Always return `true` since we have moved WidenableCondition.
return true;
}
diff --git a/llvm/lib/Transforms/Utils/GuardUtils.cpp b/llvm/lib/Transforms/Utils/GuardUtils.cpp
index 7c310f16d46e219..98ba962093f5460 100644
--- a/llvm/lib/Transforms/Utils/GuardUtils.cpp
+++ b/llvm/lib/Transforms/Utils/GuardUtils.cpp
@@ -78,49 +78,12 @@ void llvm::makeGuardControlFlowExplicit(Function *DeoptIntrinsic,
}
}
-
-void llvm::widenWidenableBranch(BranchInst *WidenableBR, Value *NewCond) {
- assert(isWidenableBranch(WidenableBR) && "precondition");
-
- // The tempting trivially option is to produce something like this:
- // br (and oldcond, newcond) where oldcond is assumed to contain a widenable
- // condition, but that doesn't match the pattern parseWidenableBranch expects
- // so we have to be more sophisticated.
-
- Use *C, *WC;
- BasicBlock *IfTrue...
<truncated>
</pre>
</details>
https://github.com/llvm/llvm-project/pull/66501
More information about the llvm-commits
mailing list