Skip to content

Commit

Permalink
JIT: revise optRelopImpliesRelop to always set reverseSense (#111803
Browse files Browse the repository at this point in the history 
)

Update clients to rely on `reverseSense` rather than parsing `vnRelation`.

Also revised the and/or inference slightly as I found it hard to follow;
hopefully it's clearer now.

Fixes an issue that came up in testing #111766.
  • Loading branch information
@AndyAyersMS
AndyAyersMS authored Jan 24, 2025
1 parent f0de80d commit cedf4e2
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 21 deletions.
48 changes: 33 additions & 15 deletions src/coreclr/jit/redundantbranchopts.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -417,8 +417,10 @@ void Compiler::optRelopImpliesRelop(RelopImplicationInfo* rii)
const ValueNum relatedVN = vnStore->GetRelatedRelop(rii->domCmpNormVN, vnRelation);
if ((relatedVN != ValueNumStore::NoVN) && (relatedVN == rii->treeNormVN))
{
rii->canInfer = true;
rii->vnRelation = vnRelation;
rii->canInfer = true;
rii->vnRelation = vnRelation;
rii->reverseSense = (rii->vnRelation == ValueNumStore::VN_RELATION_KIND::VRK_Reverse) ||
(rii->vnRelation == ValueNumStore::VN_RELATION_KIND::VRK_SwapReverse);
return;
}
}
Expand Down Expand Up @@ -543,21 +545,40 @@ void Compiler::optRelopImpliesRelop(RelopImplicationInfo* rii)
// If dom predicate is wrapped in EQ(*,0) then a true dom
// predicate implies a false branch outcome, and vice versa.
//
// And if the dom predicate is GT_NOT we reverse yet again.
//
rii->reverseSense = (oper == GT_EQ) ^ (predOper == GT_NOT);
rii->reverseSense = (rii->vnRelation == ValueNumStore::VN_RELATION_KIND::VRK_Reverse) ||
(rii->vnRelation == ValueNumStore::VN_RELATION_KIND::VRK_SwapReverse);

// We only get partial knowledge in these cases.
// We only get partial knowledge in the AND/OR cases.
//
// AND(p1,p2) = true ==> both p1 and p2 must be true
// AND(p1,p2) = false ==> don't know p1 or p2
// OR(p1,p2) = true ==> don't know p1 or p2
// OR(p1,p2) = false ==> both p1 and p2 must be false
//
if (predOper != GT_NOT)
if (predOper == GT_AND)
{
// EQ(AND, 0) false ==> AND true ==> AND operands true
rii->canInferFromFalse = (oper == GT_EQ);
// NE(AND, 0) true ==> AND true ==> AND operands true
rii->canInferFromTrue = (oper == GT_NE);
rii->reverseSense ^= (oper == GT_EQ);
}
else if (predOper == GT_OR)
{
rii->canInferFromFalse = rii->reverseSense ^ (predOper == GT_OR);
rii->canInferFromTrue = rii->reverseSense ^ (predOper == GT_AND);
// NE(OR, 0) false ==> OR false ==> OR operands false
rii->canInferFromFalse = (oper == GT_NE);
// EQ(OR, 0) true ==> OR false ==> OR operands false
rii->canInferFromTrue = (oper == GT_EQ);
rii->reverseSense ^= (oper == GT_EQ);
}
else
{
assert(predOper == GT_NOT);
// NE(NOT(x), 0) ==> NOT(X)
// EQ(NOT(x), 0) ==> X
rii->canInferFromTrue = true;
rii->canInferFromFalse = true;
rii->reverseSense ^= (oper == GT_NE);
}

JITDUMP("Inferring predicate value from %s\n", GenTree::OpName(predOper));
Expand Down Expand Up @@ -826,9 +847,6 @@ bool Compiler::optRedundantBranch(BasicBlock* const block)
JITDUMP(" Redundant compare; current relop:\n");
DISPTREE(tree);

const bool domIsSameRelop = (rii.vnRelation == ValueNumStore::VN_RELATION_KIND::VRK_Same) ||
(rii.vnRelation == ValueNumStore::VN_RELATION_KIND::VRK_Swap);

BasicBlock* const trueSuccessor = domBlock->GetTrueTarget();
BasicBlock* const falseSuccessor = domBlock->GetFalseTarget();

Expand All @@ -851,7 +869,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 = optJumpThreadDom(block, domBlock, domIsSameRelop);
const bool wasThreaded = optJumpThreadDom(block, domBlock, !rii.reverseSense);

if (wasThreaded)
{
Expand All @@ -862,7 +880,7 @@ bool Compiler::optRedundantBranch(BasicBlock* const block)
{
// True path in dominator reaches, false path doesn't; relop must be true/false.
//
const bool relopIsTrue = rii.reverseSense ^ (domIsSameRelop | domIsInferredRelop);
const bool relopIsTrue = !rii.reverseSense;
JITDUMP("True successor " FMT_BB " of " FMT_BB " reaches, relop [%06u] must be %s\n",
domBlock->GetTrueTarget()->bbNum, domBlock->bbNum, dspTreeID(tree),
relopIsTrue ? "true" : "false");
Expand All @@ -873,7 +891,7 @@ bool Compiler::optRedundantBranch(BasicBlock* const block)
{
// False path from dominator reaches, true path doesn't; relop must be false/true.
//
const bool relopIsFalse = rii.reverseSense ^ (domIsSameRelop | domIsInferredRelop);
const bool relopIsFalse = !rii.reverseSense;
JITDUMP("False successor " FMT_BB " of " FMT_BB " reaches, relop [%06u] must be %s\n",
domBlock->GetFalseTarget()->bbNum, domBlock->bbNum, dspTreeID(tree),
relopIsFalse ? "false" : "true");
Expand Down
8 changes: 2 additions & 6 deletions src/coreclr/jit/scev.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1559,22 +1559,18 @@ RelopEvaluationResult ScalarEvolutionContext::EvaluateRelop(ValueNum vn)
continue;
}

bool domIsInferredRelop = (rii.vnRelation == ValueNumStore::VN_RELATION_KIND::VRK_Inferred);
bool domIsSameRelop = (rii.vnRelation == ValueNumStore::VN_RELATION_KIND::VRK_Same) ||
(rii.vnRelation == ValueNumStore::VN_RELATION_KIND::VRK_Swap);

bool trueReaches = m_comp->optReachable(idom->GetTrueTarget(), m_loop->GetHeader(), idom);
bool falseReaches = m_comp->optReachable(idom->GetFalseTarget(), m_loop->GetHeader(), idom);

if (trueReaches && !falseReaches && rii.canInferFromTrue)
{
bool relopIsTrue = rii.reverseSense ^ (domIsSameRelop | domIsInferredRelop);
bool relopIsTrue = !rii.reverseSense;
return relopIsTrue ? RelopEvaluationResult::True : RelopEvaluationResult::False;
}

if (falseReaches && !trueReaches && rii.canInferFromFalse)
{
bool relopIsFalse = rii.reverseSense ^ (domIsSameRelop | domIsInferredRelop);
bool relopIsFalse = !rii.reverseSense;
return relopIsFalse ? RelopEvaluationResult::False : RelopEvaluationResult::True;
}
}
Expand Down

0 comments on commit cedf4e2

Please sign in to comment.