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

Chained comparison of two integers against a constant is not coalesced #102103

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Chained comparison of two integers against a constant is not coalesced #102103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
43dad09
Chained comparison of two integers against a constant is not coalesced
pedrobsaila May 10, 2024
951ce9d
format file
pedrobsaila May 10, 2024
a8dbdbf
optimize only int operands
pedrobsaila May 11, 2024
64a79f5
try to fix segmentation fault on arm
pedrobsaila May 11, 2024
34c6eb7
try fix errors in arm builds
pedrobsaila May 12, 2024
4f37008
fix regression on pattern a != || b==
pedrobsaila May 12, 2024
872a62e
Merge remote-tracking branch 'upstream/main' into 101347
pedrobsaila May 13, 2024
5591e9a
do not optimize for arm
pedrobsaila May 13, 2024
5fbaa7d
Merge remote-tracking branch 'upstream/main' into 101347
pedrobsaila Jul 8, 2024
62e5261
Merge remote-tracking branch 'upstream/main' into 101347
pedrobsaila Jul 24, 2024
99bd794
Merge remote-tracking branch 'upstream/main' into 101347
pedrobsaila Aug 19, 2024
c653261
rollback dev
pedrobsaila Aug 19, 2024
f6b4e8a
rollback test
pedrobsaila Aug 19, 2024
dec1c11
Merge remote-tracking branch 'upstream/main' into 101347
pedrobsaila Aug 24, 2024
0bfee18
fix remarks
pedrobsaila Aug 28, 2024
679bfec
Merge remote-tracking branch 'upstream/main' into 101347
pedrobsaila Aug 28, 2024
e24840d
fix build errors 1
pedrobsaila Aug 28, 2024
781c223
fix build errors 2
pedrobsaila Aug 28, 2024
0d2ba7b
fix build errors 3
pedrobsaila Aug 31, 2024
3e8b530
fix build errors 4
pedrobsaila Aug 31, 2024
6ab5360
fix build errors 5
pedrobsaila Sep 1, 2024
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
4 changes: 3 additions & 1 deletion src/coreclr/jit/lower.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,10 @@ class Lowering final : public Phase
void ContainCheckReturnTrap(GenTreeOp* node);
void ContainCheckLclHeap(GenTreeOp* node);
void ContainCheckRet(GenTreeUnOp* ret);
#if defined(TARGET_ARM64) || defined(TARGET_XARCH)
bool TryLowerAndOrToCCMP(GenTreeOp* tree, GenTree** next);
#endif
#ifdef TARGET_ARM64
bool TryLowerAndOrToCCMP(GenTreeOp* tree, GenTree** next);
insCflags TruthifyingFlags(GenCondition cond);
void ContainCheckConditionalCompare(GenTreeCCMP* ccmp);
void ContainCheckNeg(GenTreeOp* neg);
Expand Down
83 changes: 83 additions & 0 deletions src/coreclr/jit/lowerxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,17 @@ GenTree* Lowering::LowerBinaryArithmetic(GenTreeOp* binOp)
}
#endif

#ifdef TARGET_XARCH
if (binOp->OperIs(GT_OR, GT_AND))
{
GenTree* next;
if (TryLowerAndOrToCCMP(binOp, &next))
{
return next;
}
}
#endif

ContainCheckBinary(binOp);

return binOp->gtNext;
Expand Down Expand Up @@ -8638,6 +8649,78 @@ void Lowering::ContainCheckCast(GenTreeCast* node)
#endif // !defined(TARGET_64BIT)
}

//------------------------------------------------------------------------
// TryLowerAndOrToCCMP : Lower AND/OR of two conditions into test + CCMP + SETCC nodes.
//
// Arguments:
// tree - pointer to the node
// next - [out] Next node to lower if this function returns true
//
// Return Value:
// false if no changes were made
//
bool Lowering::TryLowerAndOrToCCMP(GenTreeOp* tree, GenTree** next)
{
assert(tree->OperIs(GT_OR, GT_AND));

if (!comp->opts.OptimizationEnabled())
{
return false;
}

GenTree* lastNode = BlockRange().LastNode();

if (!lastNode->OperIs(GT_JTRUE, GT_RETURN))
{
return false;
}
Comment on lines +8673 to +8676
Copy link
Member

Choose a reason for hiding this comment

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

This does not make sense to me. This transformation should be beneficial to do regardless of how the and/or is used.


GenTree* op1 = tree->gtGetOp1();
GenTree* op2 = tree->gtGetOp2();

if (!op1->OperIs(GT_NE, GT_EQ) || !op2->OperIs(GT_NE, GT_EQ) ||
(lastNode->OperIs(GT_JTRUE) && ((tree->OperIs(GT_OR) && (!op1->OperIs(GT_NE) || !op2->OperIs(GT_NE))) ||
(tree->OperIs(GT_AND) && (!op1->OperIs(GT_EQ) || !op2->OperIs(GT_EQ))))) ||
(lastNode->OperIs(GT_RETURN) && ((tree->OperIs(GT_OR) && (!op1->OperIs(GT_EQ) || !op2->OperIs(GT_EQ))) ||
(tree->OperIs(GT_AND) && (!op1->OperIs(GT_NE) || !op2->OperIs(GT_NE))))))
{
return false;
}
Comment on lines +8681 to +8688
Copy link
Member

@jakobbotsch jakobbotsch Sep 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Here too the JTRUE/RETURN checks do not make sense to me. Imagine having

bool foo = (x == 0) & (y == 0).
bool bar = (x != 0) | (y != 0);

in any basic block. The transformation should be able to kick in for this pattern regardless of what type of basic block it appears in.


GenTree* op11 = op1->gtGetOp1();
GenTree* op12 = op1->gtGetOp2();
GenTree* op21 = op2->gtGetOp1();
GenTree* op22 = op2->gtGetOp2();

if (!op11->TypeIs(TYP_INT) || !op11->OperIs(GT_LCL_VAR, GT_CNS_INT) || !op12->TypeIs(TYP_INT) ||
!op12->OperIs(GT_LCL_VAR, GT_CNS_INT) || !op21->TypeIs(TYP_INT) || !op21->OperIs(GT_LCL_VAR, GT_CNS_INT) ||
!op22->TypeIs(TYP_INT) || !op22->OperIs(GT_LCL_VAR, GT_CNS_INT))
{
return false;
}

op1->gtGetOp1()->ClearContained();
op1->gtGetOp2()->ClearContained();
op2->gtGetOp1()->ClearContained();
op2->gtGetOp2()->ClearContained();

op1->SetOper(GT_XOR);
op2->SetOper(GT_XOR);

if (lastNode->OperIs(GT_JTRUE) && tree->OperIs(GT_AND))
{
tree->SetOper(GT_OR);
tree->gtNext->gtNext->SetOper(GT_EQ);
}

JITDUMP("Conversion was legal. Result:\n");
DISPTREERANGE(BlockRange(), tree);
JITDUMP("\n");

*next = tree->gtNext;
return true;
}

//------------------------------------------------------------------------
// ContainCheckCompare: determine whether the sources of a compare node should be contained.
//
Expand Down
164 changes: 130 additions & 34 deletions src/coreclr/jit/optimizebools.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ class OptBoolsDsc
bool optOptimizeCompareChainCondBlock();
bool optOptimizeRangeTests();
bool optOptimizeBoolsReturnBlock(BasicBlock* b3);
bool optOptimizeCompareChainReturnBlock(BasicBlock* b3);
#ifdef DEBUG
void optOptimizeBoolsGcStress();
#endif
Expand All @@ -106,6 +107,7 @@ class OptBoolsDsc
bool optOptimizeBoolsChkTypeCostCond();
void optOptimizeBoolsUpdateTrees();
bool FindCompareChain(GenTree* condition, bool* isTestCondition);
void optOptimizeBoolsKeepFirstBlockAndFreeTheRest(bool optReturnBlock);
};

//-----------------------------------------------------------------------------
Expand Down Expand Up @@ -400,7 +402,8 @@ bool OptBoolsDsc::FindCompareChain(GenTree* condition, bool* isTestCondition)

if (condition->OperIs(GT_EQ, GT_NE) && condOp2->IsIntegralConst())
{
ssize_t condOp2Value = condOp2->AsIntCon()->IconValue();
INT64 condOp2Value =
condOp2->gtOper == GT_CNS_INT ? condOp2->AsIntCon()->gtIconVal : condOp2->AsLngCon()->gtLconVal;

if (condOp2Value == 0)
{
Expand All @@ -421,7 +424,10 @@ bool OptBoolsDsc::FindCompareChain(GenTree* condition, bool* isTestCondition)
*isTestCondition = true;
}
else if (condOp1->OperIs(GT_AND) && isPow2(static_cast<target_size_t>(condOp2Value)) &&
condOp1->gtGetOp2()->IsIntegralConst(condOp2Value))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't IsIntegralConst() be used here? It looks the same to me.

((condOp1->gtGetOp2()->gtOper == GT_CNS_INT &&
condOp1->gtGetOp2()->AsIntCon()->gtIconVal == condOp2Value) ||
(condOp1->gtGetOp2()->gtOper == GT_CNS_LNG &&
condOp1->gtGetOp2()->AsLngCon()->gtLconVal == condOp2Value)))
{
// Found a EQ/NE(AND(...,n),n) which will be optimized to tbz/tbnz during lowering.
*isTestCondition = true;
Expand Down Expand Up @@ -1235,39 +1241,13 @@ bool OptBoolsDsc::optOptimizeBoolsChkTypeCostCond()
}

//-----------------------------------------------------------------------------
// optOptimizeBoolsUpdateTrees: Fold the trees based on fold type and comparison type,
// update the edges, and unlink removed blocks
// optOptimizeBoolsKeepFirstBlockAndFreeTheRest: update the edges, and unlink removed blocks
//
void OptBoolsDsc::optOptimizeBoolsUpdateTrees()
// Arguments:
// optReturnBlock - defines whether to make first block a return or condition block
//
void OptBoolsDsc::optOptimizeBoolsKeepFirstBlockAndFreeTheRest(bool optReturnBlock)
{
assert(m_b1 != nullptr && m_b2 != nullptr);

bool optReturnBlock = false;
if (m_b3 != nullptr)
{
optReturnBlock = true;
}

assert(m_cmpOp != GT_NONE && m_c1 != nullptr && m_c2 != nullptr);

GenTree* cmpOp1 = m_foldOp == GT_NONE ? m_c1 : m_comp->gtNewOperNode(m_foldOp, m_foldType, m_c1, m_c2);

GenTree* t1Comp = m_testInfo1.compTree;
t1Comp->SetOper(m_cmpOp);
t1Comp->AsOp()->gtOp1 = cmpOp1;
t1Comp->AsOp()->gtOp2->gtType = m_foldType; // Could have been varTypeIsGC()
if (optReturnBlock)
{
// Update tree when m_b1 is BBJ_COND and m_b2 and m_b3 are GT_RETURN/GT_SWIFT_ERROR_RET (BBJ_RETURN)
t1Comp->AsOp()->gtOp2->AsIntCon()->gtIconVal = 0;
m_testInfo1.testTree->gtOper = m_testInfo2.testTree->OperGet();
m_testInfo1.testTree->gtType = m_testInfo2.testTree->TypeGet();

// Update the return count of flow graph
assert(m_comp->fgReturnCount >= 2);
--m_comp->fgReturnCount;
}

// Recost/rethread the tree if necessary
//
if (m_comp->fgNodeThreading != NodeThreading::None)
Expand Down Expand Up @@ -1360,6 +1340,39 @@ void OptBoolsDsc::optOptimizeBoolsUpdateTrees()
m_b1->bbCodeOffsEnd = optReturnBlock ? m_b3->bbCodeOffsEnd : m_b2->bbCodeOffsEnd;
}

void OptBoolsDsc::optOptimizeBoolsUpdateTrees()
{
assert(m_b1 != nullptr && m_b2 != nullptr);

bool optReturnBlock = false;
if (m_b3 != nullptr)
{
optReturnBlock = true;
}

assert(m_cmpOp != GT_NONE && m_c1 != nullptr && m_c2 != nullptr);

GenTree* cmpOp1 = m_foldOp == GT_NONE ? m_c1 : m_comp->gtNewOperNode(m_foldOp, m_foldType, m_c1, m_c2);

GenTree* t1Comp = m_testInfo1.compTree;
t1Comp->SetOper(m_cmpOp);
t1Comp->AsOp()->gtOp1 = cmpOp1;
t1Comp->AsOp()->gtOp2->gtType = m_foldType; // Could have been varTypeIsGC()
if (optReturnBlock)
{
// Update tree when m_b1 is BBJ_COND and m_b2 and m_b3 are GT_RETURN/GT_SWIFT_ERROR_RET (BBJ_RETURN)
t1Comp->AsOp()->gtOp2->AsIntCon()->gtIconVal = 0;
m_testInfo1.testTree->gtOper = m_testInfo2.testTree->OperGet();
m_testInfo1.testTree->gtType = m_testInfo2.testTree->TypeGet();

// Update the return count of flow graph
assert(m_comp->fgReturnCount >= 2);
--m_comp->fgReturnCount;
}

optOptimizeBoolsKeepFirstBlockAndFreeTheRest(optReturnBlock);
}

//-----------------------------------------------------------------------------
// optOptimizeBoolsReturnBlock: Optimize boolean when m_b1 is BBJ_COND and m_b2 and m_b3 are BBJ_RETURN
//
Expand Down Expand Up @@ -1737,6 +1750,82 @@ GenTree* OptBoolsDsc::optIsBoolComp(OptTestInfo* pOptTest)
return opr1;
}

#ifdef TARGET_XARCH
bool OptBoolsDsc::optOptimizeCompareChainReturnBlock(BasicBlock* b3)
{
Statement* const s1 = optOptimizeBoolsChkBlkCond();
if (s1 == nullptr)
{
return false;
}

GenTree* cond1 = m_testInfo1.GetTestOp();
GenTree* cond2 = m_testInfo2.GetTestOp();

GenTree* block3ReturnTree = m_b3->firstStmt()->GetRootNode()->AsOp()->GetReturnValue();

if (block3ReturnTree == nullptr || !block3ReturnTree->IsIntegralConst() || !block3ReturnTree->TypeIs(TYP_INT))
{
return false;
}

INT64 block3RetVal = block3ReturnTree->AsIntConCommon()->IntegralValue();

if (!cond1->OperIs(GT_NE) || !cond2->OperIs(GT_EQ, GT_NE) || (block3RetVal != 0 && cond2->OperIs(GT_EQ)) ||
(block3RetVal != 1 && cond2->OperIs(GT_NE)))
{
return false;
};

GenTree* op11 = cond1->AsOp()->gtOp1;
GenTree* op12 = cond1->AsOp()->gtOp2;
GenTree* op21 = cond2->AsOp()->gtOp1;
GenTree* op22 = cond2->AsOp()->gtOp2;

if (!op11->OperIs(GT_LCL_VAR, GT_CNS_INT) || !op12->OperIs(GT_LCL_VAR, GT_CNS_INT) ||
!op21->OperIs(GT_LCL_VAR, GT_CNS_INT) || !op22->OperIs(GT_LCL_VAR, GT_CNS_INT) || !op11->TypeIs(TYP_INT) ||
!op12->TypeIs(TYP_INT) || !op21->TypeIs(TYP_INT) || !op22->TypeIs(TYP_INT))
{
return false;
}

GenTreeOp* xorOp1 = m_comp->gtNewOperNode(GT_EQ, TYP_INT, op11, op12);
xorOp1->gtPrev = op12;
op12->gtNext = xorOp1;
op12->gtPrev = op11;
op11->gtNext = op12;
op11->gtPrev = nullptr;
GenTreeOp* xorOp2 = m_comp->gtNewOperNode(GT_EQ, TYP_INT, op21, op22);
xorOp2->gtPrev = op22;
op22->gtNext = xorOp2;
op22->gtPrev = op21;
op21->gtNext = op22;

GenTreeOp* branchlessOrEqOp = m_comp->gtNewOperNode(GT_OR, TYP_INT, xorOp1, xorOp2);
branchlessOrEqOp->gtPrev = xorOp2;
xorOp2->gtNext = branchlessOrEqOp;
op21->gtPrev = xorOp1;
xorOp1->gtNext = op21;

GenTreeIntCon* trueOp = m_comp->gtNewFalse();
m_testInfo1.compTree->SetOper(cond2->OperIs(GT_EQ) ? GT_EQ : GT_NE);
m_testInfo1.compTree->gtType = TYP_INT;

m_testInfo1.compTree->AsOp()->gtOp1 = branchlessOrEqOp;
m_testInfo1.compTree->AsOp()->gtOp2 = trueOp;
m_testInfo1.compTree->gtPrev = trueOp;
trueOp->gtNext = m_testInfo1.compTree;
trueOp->gtPrev = branchlessOrEqOp;
branchlessOrEqOp->gtNext = trueOp;

m_testInfo1.testTree->gtOper = m_testInfo2.testTree->OperGet();
m_testInfo1.testTree->gtType = m_testInfo2.testTree->TypeGet();

optOptimizeBoolsKeepFirstBlockAndFreeTheRest(true);
return true;
}
#endif

//-----------------------------------------------------------------------------
// optOptimizeBools: Folds boolean conditionals for GT_JTRUE/GT_RETURN/GT_SWIFT_ERROR_RET nodes
//
Expand Down Expand Up @@ -1937,7 +2026,7 @@ PhaseStatus Compiler::optOptimizeBools()
retry = true;
numCond++;
}
#ifdef TARGET_ARM64
#if defined(TARGET_ARM64) || defined(TARGET_XARCH)
else if (optBoolsDsc.optOptimizeCompareChainCondBlock())
{
// The optimization will have merged b1 and b2. Retry the loop so that
Expand Down Expand Up @@ -1972,6 +2061,13 @@ PhaseStatus Compiler::optOptimizeBools()
change = true;
numReturn++;
}
#ifdef TARGET_XARCH
else if (optBoolsDsc.optOptimizeCompareChainReturnBlock(b3))
{
change = true;
numReturn++;
}
#endif
}
else
{
Expand Down
Loading