Skip to content
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.

Sign up for GitHub

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

Jump to bottom

JIT: extend redundant relop to handle side effects #61275

Merged
merged 2 commits into from
Nov 9, 2021
Merged

JIT: extend redundant relop to handle side effects #61275

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a954ea6
JIT: extend redundant relop to handle side effects
AndyAyersMS Nov 5, 2021
abc92ea
verify exception set
AndyAyersMS Nov 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
203 changes: 126 additions & 77 deletions src/coreclr/jit/redundantbranchopts.cpp
View file
Open in desktop
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 @@ -813,7 +820,7 @@ 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))
{
Expand All @@ -836,9 +843,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 +872,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 +903,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 +977,30 @@ 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:
//
// (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.
// If the normal liberal VN of RHS is the normal liberal VN of the current tree, or is "related",
// consider forward sub.
//
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));

// 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 +1030,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 +1042,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 we need the reverse compare, make it so
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.
// 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()));
Copy link
Member

@jakobbotsch jakobbotsch Nov 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the comment incomplete?

Also, we can use substitudeTree->SetOper(..., ValueNumberUpdate::PRESERVE_VN); here instead. Nevermind, I misread the code.


// 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 +1108,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
30 changes: 28 additions & 2 deletions src/coreclr/jit/valuenum.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4548,18 +4548,25 @@ bool ValueNumStore::IsVNHandle(ValueNum vn)
// vrk - whether the new vn should swap, reverse, or both
//
// Returns:
// vn for reversed/swapped comparsion, or NoVN.
// vn for related comparsion, or NoVN.
//
// Note:
// If "vn" corresponds to (x > y), the resulting VN corresponds to
// VRK_Same (x > y)
// VRK_Swap (y < x)
// VRK_Reverse (x <= y)
// VRK_SwapReverse (y >= x)
//
// Will return NoVN for all float comparisons.
// VRK_Same will always return the VN passed in.
// For other relations, this method will return NoVN for all float comparisons.
//
ValueNum ValueNumStore::GetRelatedRelop(ValueNum vn, VN_RELATION_KIND vrk)
{
if (vrk == VN_RELATION_KIND::VRK_Same)
{
return vn;
}

if (vn == NoVN)
{
return NoVN;
Expand Down Expand Up @@ -4680,6 +4687,25 @@ ValueNum ValueNumStore::GetRelatedRelop(ValueNum vn, VN_RELATION_KIND vrk)
return result;
}

#ifdef DEBUG
const char* ValueNumStore::VNRelationString(VN_RELATION_KIND vrk)
{
switch (vrk)
{
case VN_RELATION_KIND::VRK_Same:
return "same";
case VN_RELATION_KIND::VRK_Reverse:
return "reversed";
case VN_RELATION_KIND::VRK_Swap:
return "swapped";
case VN_RELATION_KIND::VRK_SwapReverse:
return "swapped and reversed";
default:
return "unknown vn relation";
}
}
#endif

bool ValueNumStore::IsVNRelop(ValueNum vn)
{
VNFuncApp funcAttr;
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/jit/valuenum.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -837,11 +837,16 @@ class ValueNumStore
//
enum class VN_RELATION_KIND
{
VRK_Same, // (x > y)
VRK_Swap, // (y > x)
VRK_Reverse, // (x <= y)
VRK_SwapReverse // (y >= x)
};

#ifdef DEBUG
static const char* VNRelationString(VN_RELATION_KIND vrk);
#endif

ValueNum GetRelatedRelop(ValueNum vn, VN_RELATION_KIND vrk);

// Convert a vartype_t to the value number's storage type for that vartype_t.
Expand Down