-
Notifications
You must be signed in to change notification settings - Fork 5.1k
JIT: Fold more nullchecks #111985
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
JIT: Fold more nullchecks #111985
Changes from 7 commits
8578b39
16601c4
1aa4aeb
787203a
68bd473
a349dd7
b85ba38
1cf58fb
49a1767
b55fceb
36c355b
e3d03b2
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 |
---|---|---|
|
@@ -4367,6 +4367,60 @@ GenTree* Compiler::optAssertionProp_RelOp(ASSERT_VALARG_TP assertions, GenTree* | |
return optAssertionPropLocal_RelOp(assertions, tree, stmt); | ||
} | ||
|
||
//-------------------------------------------------------------------------------- | ||
// optVisitReachingAssertions: given a tree, call the specified callback function on all | ||
// the assertions that reach it via PHI definitions if any. | ||
// | ||
// Arguments: | ||
// GenTree - The tree to visit all the reaching assertions for | ||
// argVisitor - The callback function to call on the vn and its reaching assertions | ||
// | ||
// Return Value: | ||
// AssertVisit::Aborted - an argVisitor returned AssertVisit::Abort, we stop the walk and return | ||
// AssertVisit::Continue - all argVisitor returned AssertVisit::Continue | ||
// | ||
template <typename TAssertVisitor> | ||
Compiler::AssertVisit Compiler::optVisitReachingAssertions(GenTree* tree, TAssertVisitor argVisitor) | ||
{ | ||
// It is better to rely only on VN and do what VNVisitReachingVNs does, but it is tricky | ||
// to propagate BBs there. So, we do a simple walk here (no nested PHI nodes). | ||
// | ||
// We assume that the caller already checked aggregated assertions for the current block, | ||
// so we're interested only in reaching assertions via PHI nodes. | ||
// | ||
if (!tree->OperIs(GT_LCL_VAR) || !tree->AsLclVar()->HasSsaName()) | ||
{ | ||
return AssertVisit::Abort; | ||
} | ||
|
||
GenTreeLclVarCommon* lcl = tree->AsLclVarCommon(); | ||
LclSsaVarDsc* ssaDef = lvaGetDesc(lcl->GetLclNum())->GetPerSsaData(lcl->GetSsaNum()); | ||
GenTreeLclVarCommon* node = ssaDef->GetDefNode(); | ||
|
||
if ((node == nullptr) || !node->OperIs(GT_STORE_LCL_VAR) || !node->Data()->OperIs(GT_PHI)) | ||
{ | ||
return AssertVisit::Abort; | ||
} | ||
EgorBo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
for (GenTreePhi::Use& use : node->Data()->AsPhi()->Uses()) | ||
{ | ||
GenTreePhiArg* phiArg = use.GetNode()->AsPhiArg(); | ||
const ValueNum phiArgVN = vnStore->VNConservativeNormalValue(phiArg->gtVNPair); | ||
ASSERT_TP assertions = optGetEdgeAssertions(ssaDef->GetBlock(), phiArg->gtPredBB); | ||
if (BitVecOps::MayBeUninit(assertions)) | ||
{ | ||
return AssertVisit::Abort; | ||
} | ||
|
||
if (argVisitor(phiArgVN, assertions) == AssertVisit::Abort) | ||
{ | ||
// The visitor wants to abort the walk. | ||
return AssertVisit::Abort; | ||
} | ||
} | ||
return AssertVisit::Continue; | ||
} | ||
|
||
//------------------------------------------------------------------------ | ||
// optAssertionProp: try and optimize a relop via assertion propagation | ||
// | ||
|
@@ -4380,6 +4434,8 @@ GenTree* Compiler::optAssertionProp_RelOp(ASSERT_VALARG_TP assertions, GenTree* | |
// | ||
GenTree* Compiler::optAssertionPropGlobal_RelOp(ASSERT_VALARG_TP assertions, GenTree* tree, Statement* stmt) | ||
{ | ||
assert(!optLocalAssertionProp); | ||
|
||
GenTree* newTree = tree; | ||
GenTree* op1 = tree->AsOp()->gtOp1; | ||
GenTree* op2 = tree->AsOp()->gtOp2; | ||
|
@@ -4463,6 +4519,22 @@ GenTree* Compiler::optAssertionPropGlobal_RelOp(ASSERT_VALARG_TP assertions, Gen | |
return nullptr; | ||
} | ||
|
||
// See if we have "PHI ==/!= null" tree. If so, we iterate over all PHI's arguments, | ||
// and if all of them are known to be non-null, we can bash the comparison to true/false. | ||
if (op2->IsIntegralConst(0) && op1->TypeIs(TYP_REF)) | ||
{ | ||
auto visitor = [this](ValueNum reachingVN, ASSERT_TP reachingAssertions) { | ||
return optAssertionVNIsNonNull(reachingVN, reachingAssertions) ? AssertVisit::Continue : AssertVisit::Abort; | ||
}; | ||
if (optVisitReachingAssertions(op1, visitor) == AssertVisit::Continue) | ||
{ | ||
JITDUMP("... all of PHI's arguments are never null!\n"); | ||
assert(newTree->OperIs(GT_EQ, GT_NE)); | ||
newTree = tree->OperIs(GT_EQ) ? gtNewIconNode(0) : gtNewIconNode(1); | ||
return optAssertionProp_Update(newTree, tree, stmt); | ||
} | ||
} | ||
|
||
// Find an equal or not equal assertion involving "op1" and "op2". | ||
index = optGlobalAssertionIsEqualOrNotEqual(assertions, op1, op2); | ||
|
||
|
@@ -5083,31 +5155,19 @@ bool Compiler::optAssertionVNIsNonNull(ValueNum vn, ASSERT_VALARG_TP assertions) | |
return true; | ||
} | ||
|
||
// Check each assertion to find if we have a vn != null assertion. | ||
// | ||
BitVecOps::Iter iter(apTraits, assertions); | ||
unsigned index = 0; | ||
while (iter.NextElem(&index)) | ||
if (!BitVecOps::MayBeUninit(assertions)) | ||
{ | ||
AssertionIndex assertionIndex = GetAssertionIndex(index); | ||
if (assertionIndex > optAssertionCount) | ||
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. redundant condition, I guess it wasn't deleted when we moved all "walk asserts" pieces to |
||
{ | ||
break; | ||
} | ||
AssertionDsc* curAssertion = optGetAssertion(assertionIndex); | ||
if (!curAssertion->CanPropNonNull()) | ||
{ | ||
continue; | ||
} | ||
|
||
if (curAssertion->op1.vn != vn) | ||
BitVecOps::Iter iter(apTraits, assertions); | ||
unsigned index = 0; | ||
while (iter.NextElem(&index)) | ||
{ | ||
continue; | ||
AssertionDsc* curAssertion = optGetAssertion(GetAssertionIndex(index)); | ||
if (curAssertion->CanPropNonNull() && curAssertion->op1.vn == vn) | ||
{ | ||
return true; | ||
} | ||
} | ||
|
||
return true; | ||
} | ||
|
||
return false; | ||
} | ||
|
||
|
@@ -5810,6 +5870,35 @@ ASSERT_VALRET_TP Compiler::optGetVnMappedAssertions(ValueNum vn) | |
return BitVecOps::UninitVal(); | ||
} | ||
|
||
//------------------------------------------------------------------------ | ||
// optGetEdgeAssertions: Given a block and its predecessor, get the assertions | ||
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've extracted this function from rangecheck.cpp to re-use in my function |
||
// the predecessor creates for the block. | ||
// | ||
// Arguments: | ||
// block - The block to get the assertions for. | ||
// blockPred - The predecessor of the block (creating the assertions). | ||
// | ||
// Return Value: | ||
// The assertions we have about the value number. | ||
// | ||
ASSERT_VALRET_TP Compiler::optGetEdgeAssertions(const BasicBlock* block, const BasicBlock* blockPred) const | ||
{ | ||
if (blockPred->KindIs(BBJ_COND) && blockPred->FalseTargetIs(block)) | ||
{ | ||
return blockPred->bbAssertionOut; | ||
} | ||
|
||
if ((blockPred->KindIs(BBJ_ALWAYS) && blockPred->TargetIs(block)) || | ||
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 thought for 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. Actually for all other BBJ_KINDs you can use 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. @AndyAyersMS are you sure? e.g. what about BBJ_SWITCH or some EH related blocks? I copied this function from rangecheck 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've partially addressed your feedback as I was not confident I can just unconditionally return 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.
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. @AndyAyersMS I see, ok changed |
||
(blockPred->KindIs(BBJ_COND) && blockPred->TrueTargetIs(block))) | ||
{ | ||
if (bbJtrueAssertionOut != nullptr) | ||
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. Curious if you ever see this being null. 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 assumed it could in rangecheck if assertprop didn't run, but not sure such combination is possible. From a quick SPMI run it's not null. I'll convert it into an assert in my next PR to avoid spinning CI for it |
||
{ | ||
return bbJtrueAssertionOut[blockPred->bbNum]; | ||
} | ||
} | ||
return BitVecOps::UninitVal(); | ||
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. Seems like this should return an empty BV on failure, though I also think (once the above is restructured to handle all pred block kinds) you should never get here. 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. returned empty BV |
||
} | ||
|
||
/***************************************************************************** | ||
* | ||
* Given a const assertion this method computes the set of implied assertions | ||
|
Uh oh!
There was an error while loading. Please reload this page.