-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[X86,SimplifyCFG] Support hoisting load/store with conditional faulting (Part II) #108812
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.
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
Changes from all commits
1077be1
9d73fdd
15e25fb
40e2130
d6d50c4
de28ed9
8ec9409
fc7df9a
dfe6cc6
1ad8714
5a5de39
b1bda56
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1661,21 +1661,43 @@ static bool areIdenticalUpToCommutativity(const Instruction *I1, | |
/// \endcode | ||
/// | ||
/// So we need to turn hoisted load/store into cload/cstore. | ||
/// | ||
/// \param BI The branch instruction. | ||
/// \param SpeculatedConditionalLoadsStores The load/store instructions that | ||
/// will be speculated. | ||
/// \param Invert indicates if speculates FalseBB. Only used in triangle CFG. | ||
static void hoistConditionalLoadsStores( | ||
BranchInst *BI, | ||
SmallVectorImpl<Instruction *> &SpeculatedConditionalLoadsStores, | ||
bool Invert) { | ||
std::optional<bool> Invert) { | ||
auto &Context = BI->getParent()->getContext(); | ||
auto *VCondTy = FixedVectorType::get(Type::getInt1Ty(Context), 1); | ||
auto *Cond = BI->getOperand(0); | ||
// Construct the condition if needed. | ||
BasicBlock *BB = BI->getParent(); | ||
IRBuilder<> Builder(SpeculatedConditionalLoadsStores.back()); | ||
Value *Mask = Builder.CreateBitCast( | ||
Invert ? Builder.CreateXor(Cond, ConstantInt::getTrue(Context)) : Cond, | ||
VCondTy); | ||
IRBuilder<> Builder( | ||
Invert.has_value() ? SpeculatedConditionalLoadsStores.back() : BI); | ||
Value *Mask = nullptr; | ||
Value *MaskFalse = nullptr; | ||
Value *MaskTrue = nullptr; | ||
if (Invert.has_value()) { | ||
Mask = Builder.CreateBitCast( | ||
*Invert ? Builder.CreateXor(Cond, ConstantInt::getTrue(Context)) : Cond, | ||
VCondTy); | ||
} else { | ||
MaskFalse = Builder.CreateBitCast( | ||
Builder.CreateXor(Cond, ConstantInt::getTrue(Context)), VCondTy); | ||
MaskTrue = Builder.CreateBitCast(Cond, VCondTy); | ||
} | ||
auto PeekThroughBitcasts = [](Value *V) { | ||
while (auto *BitCast = dyn_cast<BitCastInst>(V)) | ||
V = BitCast->getOperand(0); | ||
return V; | ||
}; | ||
for (auto *I : SpeculatedConditionalLoadsStores) { | ||
IRBuilder<> Builder(I); | ||
IRBuilder<> Builder(Invert.has_value() ? I : BI); | ||
if (!Invert.has_value()) | ||
Mask = I->getParent() == BI->getSuccessor(0) ? MaskTrue : MaskFalse; | ||
// We currently assume conditional faulting load/store is supported for | ||
// scalar types only when creating new instructions. This can be easily | ||
// extended for vector types in the future. | ||
|
@@ -1687,12 +1709,14 @@ static void hoistConditionalLoadsStores( | |
auto *Ty = I->getType(); | ||
PHINode *PN = nullptr; | ||
Value *PassThru = nullptr; | ||
for (User *U : I->users()) | ||
if ((PN = dyn_cast<PHINode>(U))) { | ||
PassThru = Builder.CreateBitCast(PN->getIncomingValueForBlock(BB), | ||
FixedVectorType::get(Ty, 1)); | ||
break; | ||
} | ||
if (Invert.has_value()) | ||
for (User *U : I->users()) | ||
if ((PN = dyn_cast<PHINode>(U))) { | ||
PassThru = Builder.CreateBitCast( | ||
PeekThroughBitcasts(PN->getIncomingValueForBlock(BB)), | ||
FixedVectorType::get(Ty, 1)); | ||
break; | ||
} | ||
MaskedLoadStore = Builder.CreateMaskedLoad( | ||
FixedVectorType::get(Ty, 1), Op0, LI->getAlign(), Mask, PassThru); | ||
Value *NewLoadStore = Builder.CreateBitCast(MaskedLoadStore, Ty); | ||
|
@@ -1701,8 +1725,8 @@ static void hoistConditionalLoadsStores( | |
I->replaceAllUsesWith(NewLoadStore); | ||
} else { | ||
// Handle Store. | ||
auto *StoredVal = | ||
Builder.CreateBitCast(Op0, FixedVectorType::get(Op0->getType(), 1)); | ||
auto *StoredVal = Builder.CreateBitCast( | ||
PeekThroughBitcasts(Op0), FixedVectorType::get(Op0->getType(), 1)); | ||
MaskedLoadStore = Builder.CreateMaskedStore( | ||
StoredVal, I->getOperand(1), cast<StoreInst>(I)->getAlign(), Mask); | ||
} | ||
|
@@ -3151,7 +3175,8 @@ static bool validateAndCostRequiredSelects(BasicBlock *BB, BasicBlock *ThenBB, | |
return HaveRewritablePHIs; | ||
} | ||
|
||
static bool isProfitableToSpeculate(const BranchInst *BI, bool Invert, | ||
static bool isProfitableToSpeculate(const BranchInst *BI, | ||
std::optional<bool> Invert, | ||
const TargetTransformInfo &TTI) { | ||
// If the branch is non-unpredictable, and is predicted to *not* branch to | ||
// the `then` block, then avoid speculating it. | ||
|
@@ -3162,7 +3187,10 @@ static bool isProfitableToSpeculate(const BranchInst *BI, bool Invert, | |
if (!extractBranchWeights(*BI, TWeight, FWeight) || (TWeight + FWeight) == 0) | ||
return true; | ||
|
||
uint64_t EndWeight = Invert ? TWeight : FWeight; | ||
if (!Invert.has_value()) | ||
return false; | ||
|
||
uint64_t EndWeight = *Invert ? TWeight : FWeight; | ||
BranchProbability BIEndProb = | ||
BranchProbability::getBranchProbability(EndWeight, TWeight + FWeight); | ||
BranchProbability Likely = TTI.getPredictableBranchThreshold(); | ||
|
@@ -7854,6 +7882,35 @@ bool SimplifyCFGOpt::simplifyCondBranch(BranchInst *BI, IRBuilder<> &Builder) { | |
if (HoistCommon && | ||
hoistCommonCodeFromSuccessors(BI, !Options.HoistCommonInsts)) | ||
return requestResimplify(); | ||
|
||
if (BI && HoistLoadsStoresWithCondFaulting && | ||
Options.HoistLoadsStoresWithCondFaulting && | ||
isProfitableToSpeculate(BI, std::nullopt, TTI)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, from you code, it seems the hoist can happen only when TWeight = FWeight = 0. ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we need a meaningful ratio here, but we haven't enabled PGO. So let's leave it when we do PGO tuning. |
||
SmallVector<Instruction *, 2> SpeculatedConditionalLoadsStores; | ||
auto CanSpeculateConditionalLoadsStores = [&]() { | ||
for (auto *Succ : successors(BB)) { | ||
for (Instruction &I : *Succ) { | ||
if (I.isTerminator()) { | ||
if (I.getNumSuccessors() > 1) | ||
return false; | ||
Comment on lines
+7894
to
+7895
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add test case for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
continue; | ||
} else if (!isSafeCheapLoadStore(&I, TTI) || | ||
SpeculatedConditionalLoadsStores.size() == | ||
HoistLoadsStoresWithCondFaultingThreshold) { | ||
Comment on lines
+7898
to
+7899
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should consider branch probability for this, e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea, done! |
||
return false; | ||
} | ||
SpeculatedConditionalLoadsStores.push_back(&I); | ||
} | ||
} | ||
return !SpeculatedConditionalLoadsStores.empty(); | ||
}; | ||
|
||
if (CanSpeculateConditionalLoadsStores()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems the lambda is used once, maybe
looks better? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The advantage to use lambda is we can direct break inner loop by return. We have to use goto or more flags if change to non lambda code. |
||
hoistConditionalLoadsStores(BI, SpeculatedConditionalLoadsStores, | ||
std::nullopt); | ||
return requestResimplify(); | ||
} | ||
} | ||
} else { | ||
// If Successor #1 has multiple preds, we may be able to conditionally | ||
// execute Successor #0 if it branches to Successor #1. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment like
?
It's a little hard to know when it's nullopt w/o searching for the caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.