[GuardWidening] Widen widenable conditions instead of widenable branches #66501
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-analysis ChangesNone --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:
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...
|
aleks-tmb
force-pushed
the
guard-widening-rework
branch
from
c948731
to
f5f2243
Compare
Turn GuardWidening and LoopPredication branch widening scheme when always `br(cond && WC)` form is maintained: ``` %wc = call i1 @widenable.condition() %guard = and i1 %cond0, %wc br i1 %guard ... ``` --> ``` %wide.check = and i1 %cond0, %cond1 %wc = call i1 @widenable.condition() %guard = and i1 %%wide.check, %wc br i1 %guard ... ``` ... into the scheme of widenable conditions widening when WC is being replaced by `NewCheck && WC`: ``` %wc = call i1 @widenable.condition() %guard = and i1 %cond0, %wc br i1 %guard ... ``` --> ``` %wc = call i1 @widenable.condition() %wide.check = and i1 %cond1, %wc %guard = and i1 %cond0, %wide.check br i1 %guard ... ``` The change aims to avoid cases when we turn loop-invarinat branch into a loop-variant one, like in this issue: llvm#60234
aleks-tmb
force-pushed
the
guard-widening-rework
branch
from
f5f2243
to
8acdd42
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Turn GuardWidening and LoopPredication branch widening scheme when
always
br(cond && WC)form is maintained:-->
... into the scheme of widenable conditions widening when WC is being
replaced by
NewCheck && WC:-->
The change aims to avoid cases when we turn loop-invarinat branch into a
loop-variant one, like in this issue:
#60234