Skip to content

Commit

Permalink
JIT: some enhancements to redundant branch opts (#60732)
Browse files Browse the repository at this point in the history 
Handle cases where the dominating compare is the reverse of the compare
we're trying to optimize. For example, if `(x > y)` dominates `(y <= x)`
we may be able to optimize the dominated compare.

Addresses aspects of #48115.
  • Loading branch information
@AndyAyersMS
AndyAyersMS authored Oct 23, 2021
1 parent 1bc4b61 commit 63d9790
Show file tree
Hide file tree
Showing 4 changed files with 217 additions and 23 deletions.
2 changes: 1 addition & 1 deletion src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7453,7 +7453,7 @@ class Compiler
//
PhaseStatus optRedundantBranches();
bool optRedundantBranch(BasicBlock* const block);
bool optJumpThread(BasicBlock* const block, BasicBlock* const domBlock);
bool optJumpThread(BasicBlock* const block, BasicBlock* const domBlock, bool domIsSameRelop);
bool optReachable(BasicBlock* const fromBlock, BasicBlock* const toBlock, BasicBlock* const excludedBlock);

#if ASSERTION_PROP
Expand Down
70 changes: 48 additions & 22 deletions src/coreclr/jit/redundantbranchopts.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,23 +128,42 @@ bool Compiler::optRedundantBranch(BasicBlock* const block)

if (domCmpTree->OperKind() & GTK_RELOP)
{
// We can use liberal VNs as bounds checks are not yet
// We can use liberal VNs here, as bounds checks are not yet
// manifest explicitly as relops.
//
ValueNum domCmpVN = domCmpTree->GetVN(VNK_Liberal);

// Note we could also infer the tree relop's value from similar relops higher in the dom tree.
// For example, (x >= 0) dominating (x > 0), or (x < 0) dominating (x > 0).
// 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;

// 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.
//
// For example, (x >= 0) dominating (x > 0).
//
// That is left as a future enhancement.
//
if (domCmpVN == tree->GetVN(VNK_Liberal))
if (domIsSameRelop || domIsRevRelop)
{
// The compare in "tree" is redundant.
// Is there a unique path from the dominating compare?
//
JITDUMP("\nDominator " FMT_BB " of " FMT_BB " has relop with same liberal VN:\n", domBlock->bbNum,
block->bbNum);
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");
DISPTREE(domCmpTree);
JITDUMP(" Redundant compare; current relop:\n");
DISPTREE(tree);
Expand All @@ -162,7 +181,7 @@ bool Compiler::optRedundantBranch(BasicBlock* const block)
// However we may be able to update the flow from block's predecessors so they
// bypass block and instead transfer control to jump's successors (aka jump threading).
//
const bool wasThreaded = optJumpThread(block, domBlock);
const bool wasThreaded = optJumpThread(block, domBlock, domIsSameRelop);

if (wasThreaded)
{
Expand All @@ -171,20 +190,20 @@ bool Compiler::optRedundantBranch(BasicBlock* const block)
}
else if (trueReaches)
{
// Taken jump in dominator reaches, fall through doesn't; relop must be true.
// Taken jump in dominator reaches, fall through doesn't; relop must be true/false.
//
JITDUMP("Jump successor " FMT_BB " of " FMT_BB " reaches, relop must be true\n",
domBlock->bbJumpDest->bbNum, domBlock->bbNum);
relopValue = 1;
JITDUMP("Jump successor " FMT_BB " of " FMT_BB " reaches, relop must be %s\n",
domBlock->bbJumpDest->bbNum, domBlock->bbNum, domIsSameRelop ? "true" : "false");
relopValue = domIsSameRelop ? 1 : 0;
break;
}
else if (falseReaches)
{
// Fall through from dominator reaches, taken jump doesn't; relop must be false.
// Fall through from dominator reaches, taken jump doesn't; relop must be false/true.
//
JITDUMP("Fall through successor " FMT_BB " of " FMT_BB " reaches, relop must be false\n",
domBlock->bbNext->bbNum, domBlock->bbNum);
relopValue = 0;
JITDUMP("Fall through successor " FMT_BB " of " FMT_BB " reaches, relop must be %s\n",
domBlock->bbNext->bbNum, domBlock->bbNum, domIsSameRelop ? "false" : "true");
relopValue = domIsSameRelop ? 0 : 1;
break;
}
else
Expand Down Expand Up @@ -259,6 +278,8 @@ bool Compiler::optRedundantBranch(BasicBlock* const block)
// Arguments:
// block - block with branch to optimize
// domBlock - a dominating block that has an equivalent branch
// domIsSameRelop - if true, dominating block does the same compare;
// if false, dominating block does a reverse compare
//
// Returns:
// True if the branch was optimized.
Expand All @@ -273,7 +294,7 @@ bool Compiler::optRedundantBranch(BasicBlock* const block)
// / \ | |
// C D C D
//
bool Compiler::optJumpThread(BasicBlock* const block, BasicBlock* const domBlock)
bool Compiler::optJumpThread(BasicBlock* const block, BasicBlock* const domBlock, bool domIsSameRelop)
{
assert(block->bbJumpKind == BBJ_COND);
assert(domBlock->bbJumpKind == BBJ_COND);
Expand Down Expand Up @@ -370,8 +391,13 @@ bool Compiler::optJumpThread(BasicBlock* const block, BasicBlock* const domBlock
// are correlated exclusively with a true value for block's relop, and which
// are correlated exclusively with a false value (aka true preds and false preds).
//
// To do this we try and follow the flow from domBlock to block; any block pred
// reachable from domBlock's true edge is a true pred, and vice versa.
// To do this we try and follow the flow from domBlock to block. When domIsSameRelop
// is true, any block pred reachable from domBlock's true edge is a true pred of block,
// and any block pred reachable from domBlock's false edge is a false pred of block.
//
// If domIsSameRelop is false, then the roles of the of the paths from domBlock swap:
// any block pred reachable from domBlock's true edge is a false pred of block,
// and any block pred reachable from domBlock's false edge is a true pred of block.
//
// However, there are some exceptions:
//
Expand Down Expand Up @@ -400,8 +426,8 @@ bool Compiler::optJumpThread(BasicBlock* const block, BasicBlock* const domBlock
int numTruePreds = 0;
int numFalsePreds = 0;
BasicBlock* fallThroughPred = nullptr;
BasicBlock* const trueSuccessor = domBlock->bbJumpDest;
BasicBlock* const falseSuccessor = domBlock->bbNext;
BasicBlock* const trueSuccessor = domIsSameRelop ? domBlock->bbJumpDest : domBlock->bbNext;
BasicBlock* const falseSuccessor = domIsSameRelop ? domBlock->bbNext : domBlock->bbJumpDest;
BasicBlock* const trueTarget = block->bbJumpDest;
BasicBlock* const falseTarget = block->bbNext;
BlockSet truePreds = BlockSetOps::MakeEmpty(this);
Expand Down
155 changes: 155 additions & 0 deletions src/coreclr/jit/valuenum.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4450,6 +4450,161 @@ bool ValueNumStore::IsVNHandle(ValueNum vn)
return c->m_attribs == CEA_Handle;
}

//------------------------------------------------------------------------
// GetRelatedRelop: return value number for reversed/swapped comparison
//
// Arguments:
// vn - vn to base things on
// vrk - whether the new vn should swap, reverse, or both
//
// Returns:
// vn for reversed/swapped comparsion, or NoVN.
//
// Note:
// If "vn" corresponds to (x > y), the resulting VN corresponds to
// VRK_Swap (y < x)
// VRK_Reverse (x <= y)
// VRK_SwapReverse (y >= x)
//
// Will return NoVN for all float comparisons.
//
ValueNum ValueNumStore::GetRelatedRelop(ValueNum vn, VN_RELATION_KIND vrk)
{
if (vn == NoVN)
{
return NoVN;
}

// Pull out any exception set.
//
ValueNum valueVN;
ValueNum excepVN;
VNUnpackExc(vn, &valueVN, &excepVN);

// Verify we have a binary func application
//
VNFuncApp funcAttr;
if (!GetVNFunc(valueVN, &funcAttr))
{
return NoVN;
}

if (funcAttr.m_arity != 2)
{
return NoVN;
}

// Don't try and model float compares.
//
if (varTypeIsFloating(TypeOfVN(funcAttr.m_args[0])))
{
return NoVN;
}

const bool reverse = (vrk == VN_RELATION_KIND::VRK_Reverse) || (vrk == VN_RELATION_KIND::VRK_SwapReverse);
const bool swap = (vrk == VN_RELATION_KIND::VRK_Swap) || (vrk == VN_RELATION_KIND::VRK_SwapReverse);

// Set up the new function
//
VNFunc newFunc = funcAttr.m_func;

// Swap the predicate, if so asked.
//
if (swap)
{
if (newFunc >= VNF_Boundary)
{
switch (newFunc)
{
case VNF_LT_UN:
newFunc = VNF_GT_UN;
break;
case VNF_LE_UN:
newFunc = VNF_GE_UN;
break;
case VNF_GE_UN:
newFunc = VNF_LE_UN;
break;
case VNF_GT_UN:
newFunc = VNF_LT_UN;
break;
default:
return NoVN;
}
}
else
{
const genTreeOps op = (genTreeOps)newFunc;

if (!GenTree::OperIsRelop(op))
{
return NoVN;
}

newFunc = (VNFunc)GenTree::SwapRelop(op);
}
}

// Reverse the predicate, if so asked.
//
if (reverse)
{
if (newFunc >= VNF_Boundary)
{
switch (newFunc)
{
case VNF_LT_UN:
newFunc = VNF_GE_UN;
break;
case VNF_LE_UN:
newFunc = VNF_GT_UN;
break;
case VNF_GE_UN:
newFunc = VNF_LT_UN;
break;
case VNF_GT_UN:
newFunc = VNF_LE_UN;
break;
default:
return NoVN;
}
}
else
{
const genTreeOps op = (genTreeOps)newFunc;

if (!GenTree::OperIsRelop(op))
{
return NoVN;
}

newFunc = (VNFunc)GenTree::ReverseRelop(op);
}
}

// Create the resulting VN, swapping arguments if needed.
//
ValueNum newVN = VNForFunc(TYP_INT, newFunc, funcAttr.m_args[swap ? 1 : 0], funcAttr.m_args[swap ? 0 : 1]);
ValueNum result = VNWithExc(newVN, excepVN);

#ifdef DEBUG
if (m_pComp->verbose)
{
printf("%s of ", swap ? (reverse ? "swap-reverse" : "swap") : "reverse");
m_pComp->vnPrint(vn, 1);
printf(" => ");
m_pComp->vnPrint(newVN, 1);
if (result != newVN)
{
m_pComp->vnPrint(result, 1);
}
printf("\n");
}
#endif

return result;
}

bool ValueNumStore::IsVNConstantBound(ValueNum vn)
{
// Do we have "var < 100"?
Expand Down
13 changes: 13 additions & 0 deletions src/coreclr/jit/valuenum.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -842,6 +842,19 @@ class ValueNumStore
// Returns true iff the VN represents a handle constant.
bool IsVNHandle(ValueNum vn);

// Given VN(x > y), return VN(y > x), VN(x <= y) or VN(y >= x)
//
// If vn is not a relop, return NoVN.
//
enum class VN_RELATION_KIND
{
VRK_Swap, // (y > x)
VRK_Reverse, // (x <= y)
VRK_SwapReverse // (y >= x)
};

ValueNum GetRelatedRelop(ValueNum vn, VN_RELATION_KIND vrk);

// Convert a vartype_t to the value number's storage type for that vartype_t.
// For example, ValueNum of type TYP_LONG are stored in a map of INT64 variables.
// Lang is the language (C++) type for the corresponding vartype_t.
Expand Down

0 comments on commit 63d9790

Please sign in to comment.