Skip to content

Commit

Permalink
JIT: extend redundant relop to handle side effects (dotnet#61275)
Browse files Browse the repository at this point in the history 
Handle the case where the side-effecting redundant relop appears
just before the jump tree relop.

Also revamp how we search for related VNs so it can be done as a
search loop.
  • Loading branch information
@AndyAyersMS
AndyAyersMS authored Nov 9, 2021
1 parent be2b009 commit 0797038
Show file tree
Hide file tree
Showing 3 changed files with 182 additions and 85 deletions.
232 changes: 149 additions & 83 deletions src/coreclr/jit/redundantbranchopts.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,11 @@ bool Compiler::optRedundantBranch(BasicBlock* const block)
return false;
}

const ValueNumStore::VN_RELATION_KIND vnRelations[] = {ValueNumStore::VN_RELATION_KIND::VRK_Same,
ValueNumStore::VN_RELATION_KIND::VRK_Reverse,
ValueNumStore::VN_RELATION_KIND::VRK_Swap,
ValueNumStore::VN_RELATION_KIND::VRK_SwapReverse};

while (domBlock != nullptr)
{
// Check the current dominator
Expand All @@ -134,21 +139,21 @@ bool Compiler::optRedundantBranch(BasicBlock* const block)
//
// Look for an exact match and also try the various swapped/reversed forms.
//
const ValueNum treeVN = tree->GetVN(VNK_Liberal);
const ValueNum domCmpVN = domCmpTree->GetVN(VNK_Liberal);
const ValueNum domCmpSwpVN =
vnStore->GetRelatedRelop(domCmpVN, ValueNumStore::VN_RELATION_KIND::VRK_Swap);
const ValueNum domCmpRevVN =
vnStore->GetRelatedRelop(domCmpVN, ValueNumStore::VN_RELATION_KIND::VRK_Reverse);
const ValueNum domCmpSwpRevVN =
vnStore->GetRelatedRelop(domCmpVN, ValueNumStore::VN_RELATION_KIND::VRK_SwapReverse);

const bool matchCmp = ((domCmpVN != ValueNumStore::NoVN) && (domCmpVN == treeVN));
const bool matchSwp = ((domCmpSwpVN != ValueNumStore::NoVN) && (domCmpSwpVN == treeVN));
const bool matchRev = ((domCmpRevVN != ValueNumStore::NoVN) && (domCmpRevVN == treeVN));
const bool matchSwpRev = ((domCmpSwpRevVN != ValueNumStore::NoVN) && (domCmpSwpRevVN == treeVN));
const bool domIsSameRelop = matchCmp || matchSwp;
const bool domIsRevRelop = matchRev || matchSwpRev;
const ValueNum treeVN = tree->GetVN(VNK_Liberal);
const ValueNum domCmpVN = domCmpTree->GetVN(VNK_Liberal);
ValueNumStore::VN_RELATION_KIND vnRelationMatch = ValueNumStore::VN_RELATION_KIND::VRK_Same;
bool matched = false;

for (auto vnRelation : vnRelations)
{
const ValueNum relatedVN = vnStore->GetRelatedRelop(domCmpVN, vnRelation);
if ((relatedVN != ValueNumStore::NoVN) && (relatedVN == treeVN))
{
vnRelationMatch = vnRelation;
matched = true;
break;
}
}

// Note we could also infer the tree relop's value from relops higher in the dom tree
// that involve the same operands but are not swaps or reverses.
Expand All @@ -157,18 +162,20 @@ bool Compiler::optRedundantBranch(BasicBlock* const block)
//
// That is left as a future enhancement.
//
if (domIsSameRelop || domIsRevRelop)
if (matched)
{
// The compare in "tree" is redundant.
// Is there a unique path from the dominating compare?
//
JITDUMP("\nDominator " FMT_BB " of " FMT_BB " has %srelop with %s liberal VN\n", domBlock->bbNum,
block->bbNum, matchSwp || matchSwpRev ? "swapped " : "",
domIsSameRelop ? "the same" : "a reverse");
JITDUMP("\nDominator " FMT_BB " of " FMT_BB " has relop with %s liberal VN\n", domBlock->bbNum,
block->bbNum, ValueNumStore::VNRelationString(vnRelationMatch));
DISPTREE(domCmpTree);
JITDUMP(" Redundant compare; current relop:\n");
DISPTREE(tree);

const bool domIsSameRelop = (vnRelationMatch == ValueNumStore::VN_RELATION_KIND::VRK_Same) ||
(vnRelationMatch == ValueNumStore::VN_RELATION_KIND::VRK_Swap);

BasicBlock* const trueSuccessor = domBlock->bbJumpDest;
BasicBlock* const falseSuccessor = domBlock->bbNext;
const bool trueReaches = optReachable(trueSuccessor, block, domBlock);
Expand Down Expand Up @@ -787,6 +794,14 @@ bool Compiler::optRedundantRelop(BasicBlock* const block)
return false;
}

// If there's just one statement, bail.
//
if (stmt == block->firstStmt())
{
JITDUMP(" -- no, no prior stmt\n");
return false;
}

GenTree* const jumpTree = stmt->GetRootNode();

if (!jumpTree->OperIs(GT_JTRUE))
Expand All @@ -813,21 +828,17 @@ bool Compiler::optRedundantRelop(BasicBlock* const block)

// If relop's value is known, bail.
//
const ValueNum treeVN = tree->GetVN(VNK_Liberal);
const ValueNum treeVN = vnStore->VNNormalValue(tree->GetVN(VNK_Liberal));

if (vnStore->IsVNConstant(treeVN))
{
JITDUMP(" -- no, jump tree cond is constant\n");
return false;
}

// If there's just one statement, bail.
// Save off the jump tree's liberal exceptional VN.
//
if (stmt == block->firstStmt())
{
JITDUMP(" -- no, no prior stmt\n");
return false;
}
const ValueNum treeExcVN = vnStore->VNExceptionSet(tree->GetVN(VNK_Liberal));

JITDUMP("\noptRedundantRelop in " FMT_BB "; jump tree is\n", block->bbNum);
DISPTREE(jumpTree);
Expand All @@ -836,9 +847,16 @@ bool Compiler::optRedundantRelop(BasicBlock* const block)
// * makes the current relop redundant;
// * can safely and profitably forward substituted to the jump.
//
Statement* prevStmt = stmt;
GenTree* candidateTree = nullptr;
bool reverse = false;
Statement* prevStmt = stmt;
GenTree* candidateTree = nullptr;
Statement* candidateStmt = nullptr;
ValueNumStore::VN_RELATION_KIND vnRelationMatch = ValueNumStore::VN_RELATION_KIND::VRK_Same;
bool sideEffect = false;

const ValueNumStore::VN_RELATION_KIND vnRelations[] = {ValueNumStore::VN_RELATION_KIND::VRK_Same,
ValueNumStore::VN_RELATION_KIND::VRK_Reverse,
ValueNumStore::VN_RELATION_KIND::VRK_Swap,
ValueNumStore::VN_RELATION_KIND::VRK_SwapReverse};

// We need to keep track of which locals might be killed by
// the trees between the expression we want to forward substitute
Expand All @@ -858,6 +876,13 @@ bool Compiler::optRedundantRelop(BasicBlock* const block)

while (true)
{
// If we've run a cross a side effecting pred tree, stop looking.
//
if (sideEffect)
{
break;
}

prevStmt = prevStmt->GetPrevStmt();

// Backwards statement walks wrap around, so if we get
Expand All @@ -882,12 +907,22 @@ bool Compiler::optRedundantRelop(BasicBlock* const block)
continue;
}

// If prevTree has side effects other than GTF_EXCEPT or GTF_ASG, bail.
// If prevTree has side effects other than GTF_EXCEPT or GTF_ASG, bail,
// unless it is in the immediately preceeding statement.
//
// (we'll later show that any exception must come from the RHS as the LHS
// will be a simple local).
//
if ((prevTree->gtFlags & GTF_SIDE_EFFECT) != (prevTree->gtFlags & (GTF_EXCEPT | GTF_ASG)))
{
JITDUMP(" -- prev tree has side effects\n");
break;
if (prevStmt->GetNextStmt() != stmt)
{
JITDUMP(" -- prev tree has side effects and is not next to jumpTree\n");
break;
}

JITDUMP(" -- prev tree has side effects, allowing as prev tree is immediately before jumpTree\n");
sideEffect = true;
}

if (!prevTree->OperIs(GT_ASG))
Expand Down Expand Up @@ -946,46 +981,43 @@ bool Compiler::optRedundantRelop(BasicBlock* const block)

definedLocals[definedLocalsCount++] = prevTreeLcl;

// If the VN of RHS is the VN of the current tree, or is "related", consider forward sub.
//
// Todo: generalize to allow when normal VN of RHS == normal VN of tree, and RHS except set contains tree except
// set.
// The concern here is whether can we forward sub an excepting (but otherwise non-side effecting tree) and
// guarantee
// the same side effect behavior.
//
// A common case we see is that there's a compare of an array element guarded by a bounds check,
// so prevTree is something like:
// If the normal liberal VN of RHS is the normal liberal VN of the current tree, or is "related",
// consider forward sub.
//
// (ASG lcl, (COMMA (bds-check), (NE (array-indir), ...)))
//
// This may have the same normal VN as our tree, but also has an exception set.
//
// Another common pattern is that the prevTree contains a call.
//
const ValueNum domCmpVN = prevTreeRHS->GetVN(VNK_Liberal);
const ValueNum domCmpSwpVN = vnStore->GetRelatedRelop(domCmpVN, ValueNumStore::VN_RELATION_KIND::VRK_Swap);
const ValueNum domCmpRevVN = vnStore->GetRelatedRelop(domCmpVN, ValueNumStore::VN_RELATION_KIND::VRK_Reverse);
const ValueNum domCmpSwpRevVN =
vnStore->GetRelatedRelop(domCmpVN, ValueNumStore::VN_RELATION_KIND::VRK_SwapReverse);

const bool matchCmp = ((domCmpVN != ValueNumStore::NoVN) && (domCmpVN == treeVN));
const bool matchSwp = ((domCmpSwpVN != ValueNumStore::NoVN) && (domCmpSwpVN == treeVN));
const bool matchRev = ((domCmpRevVN != ValueNumStore::NoVN) && (domCmpRevVN == treeVN));
const bool matchSwpRev = ((domCmpSwpRevVN != ValueNumStore::NoVN) && (domCmpSwpRevVN == treeVN));
const bool domIsSameRelop = matchCmp || matchSwp;
const bool domIsRevRelop = matchRev || matchSwpRev;

// If the tree is some unrelated computation, keep looking farther up.
//
if (!domIsSameRelop && !domIsRevRelop)
const ValueNum domCmpVN = vnStore->VNNormalValue(prevTreeRHS->GetVN(VNK_Liberal));
bool matched = false;

for (auto vnRelation : vnRelations)
{
const ValueNum relatedVN = vnStore->GetRelatedRelop(domCmpVN, vnRelation);
if ((relatedVN != ValueNumStore::NoVN) && (relatedVN == treeVN))
{
vnRelationMatch = vnRelation;
matched = true;
break;
}
}

if (!matched)
{
JITDUMP(" -- prev tree VN is not related\n");
continue;
}

JITDUMP(" -- prev tree has %srelop with %s liberal VN\n", matchSwp || matchSwpRev ? "swapped " : "",
domIsSameRelop ? "the same" : "a reverse");
JITDUMP(" -- prev tree has relop with %s liberal VN\n", ValueNumStore::VNRelationString(vnRelationMatch));

// If the jump tree VN has exceptions, verify that the RHS tree has a superset.
//
if (treeExcVN != vnStore->VNForEmptyExcSet())
{
const ValueNum prevTreeExcVN = vnStore->VNExceptionSet(prevTreeRHS->GetVN(VNK_Liberal));

if (!vnStore->VNExcIsSubset(prevTreeExcVN, treeExcVN))
{
JITDUMP(" -- prev tree does not anticipate all jump tree exceptions\n");
break;
}
}

// See if we can safely move a copy of prevTreeRHS later, to replace tree.
// We can, if none of its lcls are killed.
Expand Down Expand Up @@ -1015,9 +1047,9 @@ bool Compiler::optRedundantRelop(BasicBlock* const block)
continue;
}

// Heuristic: if the lcl defined here is live out, forward sub
// won't result in the original becoming dead. If the local is not
// live out that still may happen, but it's less likely.
// If the lcl defined here is live out, forward sub is problematic.
// We'll either create a redundant tree (as the original won't be dead)
// or lose the def (if we actually move the RHS tree).
//
if (VarSetOps::IsMember(this, block->bbLiveOut, prevTreeLclDsc->lvVarIndex))
{
Expand All @@ -1027,35 +1059,62 @@ bool Compiler::optRedundantRelop(BasicBlock* const block)

JITDUMP(" -- prev tree is viable candidate for relop fwd sub!\n");
candidateTree = prevTreeRHS;
reverse = domIsRevRelop;
candidateStmt = prevStmt;
}

if (candidateTree == nullptr)
{
return false;
}

// Looks good -- we are going to forward-sub candidateTree
//
GenTree* const substituteTree = gtCloneExpr(candidateTree);
GenTree* substituteTree = nullptr;
bool usedCopy = false;

if (candidateStmt->GetNextStmt() == stmt)
{
// We are going forward-sub candidateTree
//
substituteTree = candidateTree;
}
else
{
// We going to forward-sub a copy of candidateTree
//
assert(!sideEffect);
substituteTree = gtCloneExpr(candidateTree);
usedCopy = true;
}

// If we need the reverse compare, make it so
// If we need the reverse compare, make it so.
// We also need to set a proper VN.
//
if (reverse)
if ((vnRelationMatch == ValueNumStore::VN_RELATION_KIND::VRK_Reverse) ||
(vnRelationMatch == ValueNumStore::VN_RELATION_KIND::VRK_SwapReverse))
{
// Copy the vn info as it will be trashed when we change the oper.
//
ValueNumPair origVNP = substituteTree->gtVNPair;

// Update the tree. Note we don't actually swap operands...?
//
substituteTree->SetOper(GenTree::ReverseRelop(substituteTree->OperGet()));

// This substitute tree will have the VN the same as the original.
// (modulo excep sets...? hmmm)
substituteTree->SetVNsFromNode(tree);
// Compute the right set of VNs for this new tree.
//
ValueNum origNormConVN = vnStore->VNConservativeNormalValue(origVNP);
ValueNum origNormLibVN = vnStore->VNLiberalNormalValue(origVNP);
ValueNum newNormConVN = vnStore->GetRelatedRelop(origNormConVN, ValueNumStore::VN_RELATION_KIND::VRK_Reverse);
ValueNum newNormLibVN = vnStore->GetRelatedRelop(origNormLibVN, ValueNumStore::VN_RELATION_KIND::VRK_Reverse);
ValueNumPair newNormalVNP(newNormLibVN, newNormConVN);
ValueNumPair origExcVNP = vnStore->VNPExceptionSet(origVNP);
ValueNumPair newVNP = vnStore->VNPWithExc(newNormalVNP, origExcVNP);

substituteTree->SetVNs(newVNP);
}

// We just intentionally created a duplicate -- we don't want CSE to undo this.
// (hopefully, the original is now dead).
//
// Also this relop is now a subtree of a jump.
// This relop is now a subtree of a jump.
//
substituteTree->gtFlags |= (GTF_DONT_CSE | GTF_RELOP_JMP_USED);
substituteTree->gtFlags |= (GTF_RELOP_JMP_USED | GTF_DONT_CSE);

// Swap in the new tree.
//
Expand All @@ -1066,6 +1125,13 @@ bool Compiler::optRedundantRelop(BasicBlock* const block)

DEBUG_DESTROY_NODE(tree);

// If we didn't forward sub a copy, the candidateStmt must be removed.
//
if (!usedCopy)
{
fgRemoveStmt(block, candidateStmt);
}

JITDUMP(" -- done! new jump tree is\n");
DISPTREE(jumpTree);

Expand Down
Loading

0 comments on commit 0797038

Please sign in to comment.