From 889d790bc08240059ca81c0d5f6db70968cf73f3 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 14 Oct 2023 18:25:50 +0200 Subject: [PATCH 1/8] Optimize range tests --- src/coreclr/jit/optimizebools.cpp | 228 ++++++++++++++++++++++++++++++ 1 file changed, 228 insertions(+) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 580a64d2e6727..aeede8eb5d1f3 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -66,6 +66,7 @@ class OptBoolsDsc public: bool optOptimizeBoolsCondBlock(); bool optOptimizeCompareChainCondBlock(); + bool optOptimizeRangeTests(); bool optOptimizeBoolsReturnBlock(BasicBlock* b3); #ifdef DEBUG void optOptimizeBoolsGcStress(); @@ -404,6 +405,227 @@ bool OptBoolsDsc::FindCompareChain(GenTree* condition, bool* isTestCondition) return false; } +static bool IsConstantRangeTest(const BasicBlock* block, GenTree** lcl, GenTreeIntCon** cns, genTreeOps* cmpOp) +{ + // NOTE: caller is expected to check that a block has multiple statements or not + const GenTree* rootNode = block->lastStmt()->GetRootNode(); + assert(block->KindIs(BBJ_COND) && rootNode->OperIs(GT_JTRUE)); + const GenTree* tree = rootNode->gtGetOp1(); + + if (!tree->OperIs(GT_LE, GT_LT, GT_GT, GT_GE) || tree->IsUnsigned()) + { + return false; + } + + GenTree* op1 = tree->gtGetOp1(); + GenTree* op2 = tree->gtGetOp2(); + if (varTypeIsIntegral(op1) && varTypeIsIntegral(op2) && op1->TypeIs(op2->TypeGet())) + { + if (op2->IsCnsIntOrI()) + { + // X relop CNS + *lcl = op1; + *cns = op2->AsIntCon(); + *cmpOp = tree->OperGet(); + return true; + } + if (op1->IsCnsIntOrI()) + { + // CNS relop X + *lcl = op2; + *cns = op1->AsIntCon(); + + // Normalize to "X relop CNS" + *cmpOp = GenTree::SwapRelop(tree->OperGet()); + return true; + } + } + return false; +} + +static bool GetClosedRange( + genTreeOps cmp1, genTreeOps cmp2, ssize_t cns1, ssize_t cns2, ssize_t* pRangeStart, ssize_t* pRangeEnd) +{ + const bool range1IsStart = (cmp1 == GT_GE) || (cmp1 == GT_GT); + const bool range1IsEnd = (cmp1 == GT_LE) || (cmp1 == GT_LT); + const bool range1IsIncl = (cmp1 == GT_GE) || (cmp1 == GT_LE); + + const bool range2IsStart = (cmp2 == GT_GE) || (cmp2 == GT_GT); + const bool range2IsEnd = (cmp2 == GT_LE) || (cmp2 == GT_LT); + const bool range2IsIncl = (cmp2 == GT_GE) || (cmp2 == GT_LE); + + // Is it a closed range? + if ((range1IsStart == range2IsStart) || (range1IsEnd == range2IsEnd)) + { + return false; + } + + if (range1IsStart) + { + assert(range2IsEnd); + *pRangeStart = range1IsIncl ? cns1 : cns1 + 1; + *pRangeEnd = range2IsIncl ? cns2 : cns2 - 1; + } + else + { + assert(range1IsEnd && range2IsStart); + *pRangeStart = range2IsIncl ? cns2 : cns2 + 1; + *pRangeEnd = range1IsIncl ? cns1 : cns1 - 1; + } + + if ((*pRangeStart >= *pRangeEnd) || (*pRangeStart < 0) || (*pRangeEnd < 0)) + { + return false; + } + return true; +} + +bool OptBoolsDsc::optOptimizeRangeTests() +{ + assert(m_b1 != nullptr && m_b2 != nullptr && m_b3 == nullptr); + assert(m_b1->KindIs(BBJ_COND) && m_b2->KindIs(BBJ_COND) && m_b1->NextIs(m_b2)); + + GenTree* testVar1; + GenTree* testVar2; + GenTreeIntCon* testCns1; + GenTreeIntCon* testCns2; + genTreeOps testCmp1; + genTreeOps testCmp2; + + if (m_b2->isRunRarely() || !IsConstantRangeTest(m_b1, &testVar1, &testCns1, &testCmp1)) + { + return false; + } + + if (!BasicBlock::sameEHRegion(m_b1, m_b2)) + { + // Conditions aren't in the same EH region + return false; + } + + if (m_b1->HasJumpTo(m_b1) || m_b1->HasJumpTo(m_b2) || m_b2->HasJumpTo(m_b2) || m_b2->HasJumpTo(m_b1)) + { + // Ignoring weird cases like a condition jumping to itself or when JumpDest == Next + return false; + } + + // We're interested in just two shapes for e.g. "X > 10 && X < 100" range test: + // + // Shape 1: both conditions jump to NotInRange + // + // if (X <= 10) + // goto NotInRange; + // + // if (X >= 100) + // goto NotInRange + // + // InRange: + // ... + + // Shape 2: 2nd block jumps to InRange + // + // if (X <= 10) + // goto NotInRange; + // + // if (X > 100) + // goto InRange + // + // NotInRange: + // ... + + BasicBlock* notInRangeBb = m_b1->GetJumpDest(); + BasicBlock* inRangeBb; + + if (notInRangeBb == m_b2->GetJumpDest()) + { + // Shape 1 + inRangeBb = m_b2->Next(); + } + else if (notInRangeBb == m_b2->Next()) + { + // Shape 2 + inRangeBb = m_b2->GetJumpDest(); + } + else + { + // Unknown shape + return false; + } + + if (!m_b2->hasSingleStmt() || !IsConstantRangeTest(m_b2, &testVar2, &testCns2, &testCmp2)) + { + // The 2nd block has to be single-statement to avoid side-effects between the two conditions. + return false; + } + + if (!testVar2->OperIs(GT_LCL_VAR) || !GenTree::Compare(testVar1->gtEffectiveVal(), testVar2)) + { + // Variables don't match in two conditions + return false; + } + + if (m_b2->GetUniquePred(m_comp) != m_b1) + { + // Second block has more than one predecessor - we won't be able to remove it. + return false; + } + + // First condition always jumps-if-true to NotInRange, so let's reverse it to be "InRange" as + // a canonical form. + testCmp1 = GenTree::ReverseRelop(testCmp1); + + // For the 2nd condition it depends on the BB shape (see above). + testCmp2 = m_b2->HasJumpTo(notInRangeBb) ? GenTree::ReverseRelop(testCmp2) : testCmp2; + + ssize_t rangeStartIncl; + ssize_t rangeEndIncl; + if (!GetClosedRange(testCmp1, testCmp2, testCns1->IconValue(), testCns2->IconValue(), &rangeStartIncl, + &rangeEndIncl)) + { + // The range we test via two conditions is not a closed range + // TODO: We should support overlapped ranges here, e.g. "X > 10 && x > 100" -> "X > 100" + return false; + } + + if (!FitsIn(testVar1->TypeGet(), rangeStartIncl) || !FitsIn(testVar1->TypeGet(), rangeEndIncl)) + { + return false; + } + + genTreeOps newCmp = GT_GT; + m_comp->fgAddRefPred(inRangeBb, m_b1); + if (m_b1->HasJumpTo(m_b2->Next())) + { + // Re-direct firstBlock to jump to inRangeBb + newCmp = GenTree::ReverseRelop(newCmp); + m_b1->SetJumpDest(inRangeBb); + } + + // Remove the 2nd condition block as we no longer need it + m_comp->fgRemoveRefPred(m_b2, m_b1); + m_comp->fgRemoveBlock(m_b2, true); + + Statement* stmt = m_b1->lastStmt(); + GenTreeOp* cmp = stmt->GetRootNode()->gtGetOp1()->AsOp(); + if (rangeStartIncl == 0) + { + cmp->gtOp1 = testVar1; + } + else + { + cmp->gtOp1 = m_comp->gtNewOperNode(GT_SUB, testVar1->TypeGet(), testVar1, + m_comp->gtNewIconNode(rangeStartIncl, testVar1->TypeGet())); + } + cmp->gtOp2->BashToConst(rangeEndIncl - rangeStartIncl, testVar1->TypeGet()); + cmp->SetOper(newCmp); + cmp->SetUnsigned(); + + m_comp->gtSetStmtInfo(stmt); + m_comp->fgSetStmtSeq(stmt); + m_comp->gtUpdateStmtSideEffects(stmt); + return true; +} + //----------------------------------------------------------------------------- // optOptimizeCompareChainCondBlock: Create a chain when when both m_b1 and m_b2 are BBJ_COND. // @@ -1506,6 +1728,12 @@ PhaseStatus Compiler::optOptimizeBools() change = true; numCond++; } + else if (optBoolsDsc.optOptimizeRangeTests()) + { + change = true; + retry = true; + numCond++; + } #ifdef TARGET_ARM64 else if (optBoolsDsc.optOptimizeCompareChainCondBlock()) { From 62dc0b372c5639f41a51b3502cac76bf918e8080 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 14 Oct 2023 18:47:00 +0200 Subject: [PATCH 2/8] Respect BBF_DONT_REMOVE --- src/coreclr/jit/optimizebools.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index aeede8eb5d1f3..07681b2fb7d62 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -497,7 +497,7 @@ bool OptBoolsDsc::optOptimizeRangeTests() return false; } - if (!BasicBlock::sameEHRegion(m_b1, m_b2)) + if (!BasicBlock::sameEHRegion(m_b1, m_b2) || ((m_b2->bbFlags & BBF_DONT_REMOVE) != 0)) { // Conditions aren't in the same EH region return false; From 80cdebfdf39273c92e29a2638b228f5d67b60093 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 14 Oct 2023 20:34:27 +0200 Subject: [PATCH 3/8] Add comments --- src/coreclr/jit/optimizebools.cpp | 112 +++++++++++++++++++----------- 1 file changed, 71 insertions(+), 41 deletions(-) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 07681b2fb7d62..521cd5b1b8d43 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -405,6 +405,7 @@ bool OptBoolsDsc::FindCompareChain(GenTree* condition, bool* isTestCondition) return false; } +// Does block represent a conditional branch with a constant range test? E.g. "if (X > 10)" static bool IsConstantRangeTest(const BasicBlock* block, GenTree** lcl, GenTreeIntCon** cns, genTreeOps* cmpOp) { // NOTE: caller is expected to check that a block has multiple statements or not @@ -412,6 +413,7 @@ static bool IsConstantRangeTest(const BasicBlock* block, GenTree** lcl, GenTreeI assert(block->KindIs(BBJ_COND) && rootNode->OperIs(GT_JTRUE)); const GenTree* tree = rootNode->gtGetOp1(); + // We're only interested in continuous range tests if (!tree->OperIs(GT_LE, GT_LT, GT_GT, GT_GE) || tree->IsUnsigned()) { return false; @@ -419,6 +421,8 @@ static bool IsConstantRangeTest(const BasicBlock* block, GenTree** lcl, GenTreeI GenTree* op1 = tree->gtGetOp1(); GenTree* op2 = tree->gtGetOp2(); + + // Floating point comparisons are not supported if (varTypeIsIntegral(op1) && varTypeIsIntegral(op2) && op1->TypeIs(op2->TypeGet())) { if (op2->IsCnsIntOrI()) @@ -443,6 +447,8 @@ static bool IsConstantRangeTest(const BasicBlock* block, GenTree** lcl, GenTreeI return false; } +// Given two ranges, return true if they intersect and form a closed range. +// Returns its bounds in pRangeStart and pRangeEnd (both are inclusive). static bool GetClosedRange( genTreeOps cmp1, genTreeOps cmp2, ssize_t cns1, ssize_t cns2, ssize_t* pRangeStart, ssize_t* pRangeEnd) { @@ -454,7 +460,6 @@ static bool GetClosedRange( const bool range2IsEnd = (cmp2 == GT_LE) || (cmp2 == GT_LT); const bool range2IsIncl = (cmp2 == GT_GE) || (cmp2 == GT_LE); - // Is it a closed range? if ((range1IsStart == range2IsStart) || (range1IsEnd == range2IsEnd)) { return false; @@ -480,11 +485,21 @@ static bool GetClosedRange( return true; } +//------------------------------------------------------------------------------ +// optOptimizeRangeTests : Optimize two conditional blocks representing a constant range test. +// E.g. "X >= 10 && X <= 100" is optimized to "(X - 10) <= 90". +// +// Return Value: +// True if m_b1 and m_b2 are merged. +// bool OptBoolsDsc::optOptimizeRangeTests() { assert(m_b1 != nullptr && m_b2 != nullptr && m_b3 == nullptr); assert(m_b1->KindIs(BBJ_COND) && m_b2->KindIs(BBJ_COND) && m_b1->NextIs(m_b2)); + // Locals representing parts of the range test in both conditions + // if ((testVar1 testCmp1 testCns1) && (testVar2 testCmp2 testCns2)) + // GenTree* testVar1; GenTree* testVar2; GenTreeIntCon* testCns1; @@ -494,6 +509,7 @@ bool OptBoolsDsc::optOptimizeRangeTests() if (m_b2->isRunRarely() || !IsConstantRangeTest(m_b1, &testVar1, &testCns1, &testCmp1)) { + // m_b2 is either cold or not a constant range test return false; } @@ -511,39 +527,34 @@ bool OptBoolsDsc::optOptimizeRangeTests() // We're interested in just two shapes for e.g. "X > 10 && X < 100" range test: // - // Shape 1: both conditions jump to NotInRange - // - // if (X <= 10) - // goto NotInRange; - // - // if (X >= 100) - // goto NotInRange - // - // InRange: - // ... - - // Shape 2: 2nd block jumps to InRange - // - // if (X <= 10) - // goto NotInRange; - // - // if (X > 100) - // goto InRange - // - // NotInRange: - // ... - BasicBlock* notInRangeBb = m_b1->GetJumpDest(); BasicBlock* inRangeBb; - if (notInRangeBb == m_b2->GetJumpDest()) { - // Shape 1 + // Shape 1: both conditions jump to NotInRange + // + // if (X <= 10) + // goto NotInRange; + // + // if (X >= 100) + // goto NotInRange + // + // InRange: + // ... inRangeBb = m_b2->Next(); } else if (notInRangeBb == m_b2->Next()) { - // Shape 2 + // Shape 2: 2nd block jumps to InRange + // + // if (X <= 10) + // goto NotInRange; + // + // if (X > 100) + // goto InRange + // + // NotInRange: + // ... inRangeBb = m_b2->GetJumpDest(); } else @@ -552,21 +563,35 @@ bool OptBoolsDsc::optOptimizeRangeTests() return false; } - if (!m_b2->hasSingleStmt() || !IsConstantRangeTest(m_b2, &testVar2, &testCns2, &testCmp2)) + if (!m_b2->hasSingleStmt() || !IsConstantRangeTest(m_b2, &testVar2, &testCns2, &testCmp2) || + m_b2->GetUniquePred(m_comp) != m_b1) { // The 2nd block has to be single-statement to avoid side-effects between the two conditions. + // Also, make sure m_b2 has no other predecessors. return false; } if (!testVar2->OperIs(GT_LCL_VAR) || !GenTree::Compare(testVar1->gtEffectiveVal(), testVar2)) { // Variables don't match in two conditions - return false; - } - - if (m_b2->GetUniquePred(m_comp) != m_b1) - { - // Second block has more than one predecessor - we won't be able to remove it. + // We use gtEffectiveVal() for the first block's variable to ignore COMMAs, e.g. + // + // m_b1: + // * JTRUE void + // \--* LT int + // +--* COMMA int + // | +--* STORE_LCL_VAR int V03 cse0 + // | | \--* CAST int <- ushort <- int + // | | \--* LCL_VAR int V01 arg1 + // | \--* LCL_VAR int V03 cse0 + // \--* CNS_INT int 97 + // + // m_b2: + // * JTRUE void + // \--* GT int + // +--* LCL_VAR int V03 cse0 + // \--* CNS_INT int 122 + // return false; } @@ -577,18 +602,18 @@ bool OptBoolsDsc::optOptimizeRangeTests() // For the 2nd condition it depends on the BB shape (see above). testCmp2 = m_b2->HasJumpTo(notInRangeBb) ? GenTree::ReverseRelop(testCmp2) : testCmp2; - ssize_t rangeStartIncl; - ssize_t rangeEndIncl; - if (!GetClosedRange(testCmp1, testCmp2, testCns1->IconValue(), testCns2->IconValue(), &rangeStartIncl, - &rangeEndIncl)) + ssize_t rangeStart; + ssize_t rangeEnd; + if (!GetClosedRange(testCmp1, testCmp2, testCns1->IconValue(), testCns2->IconValue(), &rangeStart, &rangeEnd)) { // The range we test via two conditions is not a closed range // TODO: We should support overlapped ranges here, e.g. "X > 10 && x > 100" -> "X > 100" return false; } - if (!FitsIn(testVar1->TypeGet(), rangeStartIncl) || !FitsIn(testVar1->TypeGet(), rangeEndIncl)) + if (!FitsIn(testVar1->TypeGet(), rangeStart) || !FitsIn(testVar1->TypeGet(), rangeEnd)) { + // Make sure constants fit into the types we work with return false; } @@ -607,19 +632,24 @@ bool OptBoolsDsc::optOptimizeRangeTests() Statement* stmt = m_b1->lastStmt(); GenTreeOp* cmp = stmt->GetRootNode()->gtGetOp1()->AsOp(); - if (rangeStartIncl == 0) + if (rangeStart == 0) { + // We don't need to subtract anything, it's already 0-based cmp->gtOp1 = testVar1; } else { + // We need to subtract the rangeStartIncl from the variable to make the range start from 0 cmp->gtOp1 = m_comp->gtNewOperNode(GT_SUB, testVar1->TypeGet(), testVar1, - m_comp->gtNewIconNode(rangeStartIncl, testVar1->TypeGet())); + m_comp->gtNewIconNode(rangeStart, testVar1->TypeGet())); } - cmp->gtOp2->BashToConst(rangeEndIncl - rangeStartIncl, testVar1->TypeGet()); + cmp->gtOp2->BashToConst(rangeEnd - rangeStart, testVar1->TypeGet()); cmp->SetOper(newCmp); cmp->SetUnsigned(); + JITDUMP("Optimizing two conditional blocks representing a constant range test:\n") + DISPTREE(cmp) + m_comp->gtSetStmtInfo(stmt); m_comp->fgSetStmtSeq(stmt); m_comp->gtUpdateStmtSideEffects(stmt); From 02bccd59d9f9ac149710fb356b138c45957b1c4f Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 15 Oct 2023 13:09:42 +0200 Subject: [PATCH 4/8] Refactor --- src/coreclr/jit/optimizebools.cpp | 243 ++++++++++++++++-------------- 1 file changed, 126 insertions(+), 117 deletions(-) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 521cd5b1b8d43..a80b9069a2ed7 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -405,48 +405,6 @@ bool OptBoolsDsc::FindCompareChain(GenTree* condition, bool* isTestCondition) return false; } -// Does block represent a conditional branch with a constant range test? E.g. "if (X > 10)" -static bool IsConstantRangeTest(const BasicBlock* block, GenTree** lcl, GenTreeIntCon** cns, genTreeOps* cmpOp) -{ - // NOTE: caller is expected to check that a block has multiple statements or not - const GenTree* rootNode = block->lastStmt()->GetRootNode(); - assert(block->KindIs(BBJ_COND) && rootNode->OperIs(GT_JTRUE)); - const GenTree* tree = rootNode->gtGetOp1(); - - // We're only interested in continuous range tests - if (!tree->OperIs(GT_LE, GT_LT, GT_GT, GT_GE) || tree->IsUnsigned()) - { - return false; - } - - GenTree* op1 = tree->gtGetOp1(); - GenTree* op2 = tree->gtGetOp2(); - - // Floating point comparisons are not supported - if (varTypeIsIntegral(op1) && varTypeIsIntegral(op2) && op1->TypeIs(op2->TypeGet())) - { - if (op2->IsCnsIntOrI()) - { - // X relop CNS - *lcl = op1; - *cns = op2->AsIntCon(); - *cmpOp = tree->OperGet(); - return true; - } - if (op1->IsCnsIntOrI()) - { - // CNS relop X - *lcl = op2; - *cns = op1->AsIntCon(); - - // Normalize to "X relop CNS" - *cmpOp = GenTree::SwapRelop(tree->OperGet()); - return true; - } - } - return false; -} - // Given two ranges, return true if they intersect and form a closed range. // Returns its bounds in pRangeStart and pRangeEnd (both are inclusive). static bool GetClosedRange( @@ -465,6 +423,7 @@ static bool GetClosedRange( return false; } + // Report the intersection of the two ranges, add +/- 1 to make the bounds inclusive. if (range1IsStart) { assert(range2IsEnd); @@ -480,8 +439,121 @@ static bool GetClosedRange( if ((*pRangeStart >= *pRangeEnd) || (*pRangeStart < 0) || (*pRangeEnd < 0)) { + // TODO: If ranges don't intersect we might be able to fold the condition to true/false. + return false; + } + return true; +} + +// Given a compare node, return true if it is a constant range test. +bool IsConstantRangeTest(GenTreeOp* tree, GenTree** varNode, GenTreeIntCon** cnsNode, genTreeOps* cmp) +{ + if (tree->OperIs(GT_LE, GT_LT, GT_GE, GT_GT) && !tree->IsUnsigned()) + { + GenTree* op1 = tree->gtGetOp1(); + GenTree* op2 = tree->gtGetOp2(); + if (varTypeIsIntegral(op1) && varTypeIsIntegral(op2) && op1->TypeIs(op2->TypeGet())) + { + if (op2->IsCnsIntOrI()) + { + // X relop CNS + *varNode = op1; + *cnsNode = op2->AsIntCon(); + *cmp = tree->OperGet(); + return true; + } + if (op1->IsCnsIntOrI()) + { + // CNS relop X + *varNode = op2; + *cnsNode = op1->AsIntCon(); + + // Normalize to "X relop CNS" + *cmp = GenTree::SwapRelop(tree->OperGet()); + return true; + } + } + } + return false; +} + +// Given two compare nodes, return true if they can be folded into a single range check (and fold them into cmp1) +bool FoldRangeTests(Compiler* comp, GenTreeOp* cmp1, bool cmp1IsReversed, GenTreeOp* cmp2, bool cmp2IsReversed) +{ + GenTree* var1Node; + GenTree* var2Node; + GenTreeIntCon* cns1Node; + GenTreeIntCon* cns2Node; + genTreeOps cmp1Op; + genTreeOps cmp2Op; + + // Make sure both conditions are constant range checks, e.g. "X > CNS" + // TODO: support more cases, e.g. "X >= 0 && X < array.Length" -> "(uint)X < array.Length" + if (!IsConstantRangeTest(cmp1, &var1Node, &cns1Node, &cmp1Op) || + !IsConstantRangeTest(cmp2, &var2Node, &cns2Node, &cmp2Op)) + { + return false; + } + + // Reverse the comparisons if necessary so we'll get a canonical form "cond1 == true && cond2 == true" -> InRange. + cmp1Op = cmp1IsReversed ? GenTree::ReverseRelop(cmp1Op) : cmp1Op; + cmp2Op = cmp2IsReversed ? GenTree::ReverseRelop(cmp2Op) : cmp2Op; + + // Make sure variables are the same: + if (!var2Node->OperIs(GT_LCL_VAR) || !GenTree::Compare(var1Node->gtEffectiveVal(), var2Node)) + { + // Variables don't match in two conditions + // We use gtEffectiveVal() for the first block's variable to ignore COMMAs, e.g. + // + // m_b1: + // * JTRUE void + // \--* LT int + // +--* COMMA int + // | +--* STORE_LCL_VAR int V03 cse0 + // | | \--* CAST int <- ushort <- int + // | | \--* LCL_VAR int V01 arg1 + // | \--* LCL_VAR int V03 cse0 + // \--* CNS_INT int 97 + // + // m_b2: + // * JTRUE void + // \--* GT int + // +--* LCL_VAR int V03 cse0 + // \--* CNS_INT int 122 + // + return false; + } + + ssize_t rangeStart; + ssize_t rangeEnd; + if (!GetClosedRange(cmp1Op, cmp2Op, cns1Node->IconValue(), cns2Node->IconValue(), &rangeStart, &rangeEnd)) + { + // The range we test via two conditions is not a closed range + // TODO: We should support overlapped ranges here, e.g. "X > 10 && x > 100" -> "X > 100" + return false; + } + assert(rangeStart < rangeEnd); + + if (!FitsIn(var1Node->TypeGet(), rangeStart) || !FitsIn(var1Node->TypeGet(), rangeEnd)) + { + // Make sure constants fit into the types we work with return false; } + + if (rangeStart == 0) + { + // We don't need to subtract anything, it's already 0-based + cmp1->gtOp1 = var1Node; + } + else + { + // We need to subtract the rangeStartIncl from the variable to make the range start from 0 + cmp1->gtOp1 = comp->gtNewOperNode(GT_SUB, var1Node->TypeGet(), var1Node, + comp->gtNewIconNode(rangeStart, var1Node->TypeGet())); + } + cmp1->gtOp2->BashToConst(rangeEnd - rangeStart, var1Node->TypeGet()); + cmp1->SetOper(cmp2IsReversed ? GT_GT : GT_LE); + cmp1->SetUnsigned(); return true; } @@ -497,25 +569,16 @@ bool OptBoolsDsc::optOptimizeRangeTests() assert(m_b1 != nullptr && m_b2 != nullptr && m_b3 == nullptr); assert(m_b1->KindIs(BBJ_COND) && m_b2->KindIs(BBJ_COND) && m_b1->NextIs(m_b2)); - // Locals representing parts of the range test in both conditions - // if ((testVar1 testCmp1 testCns1) && (testVar2 testCmp2 testCns2)) - // - GenTree* testVar1; - GenTree* testVar2; - GenTreeIntCon* testCns1; - GenTreeIntCon* testCns2; - genTreeOps testCmp1; - genTreeOps testCmp2; - - if (m_b2->isRunRarely() || !IsConstantRangeTest(m_b1, &testVar1, &testCns1, &testCmp1)) + if (m_b2->isRunRarely()) { - // m_b2 is either cold or not a constant range test + // We don't want to make the first comparison to be slightly slower + // if the 2nd one is rarely executed. return false; } if (!BasicBlock::sameEHRegion(m_b1, m_b2) || ((m_b2->bbFlags & BBF_DONT_REMOVE) != 0)) { - // Conditions aren't in the same EH region + // Conditions aren't in the same EH region or m_b2 can't be removed return false; } @@ -563,66 +626,31 @@ bool OptBoolsDsc::optOptimizeRangeTests() return false; } - if (!m_b2->hasSingleStmt() || !IsConstantRangeTest(m_b2, &testVar2, &testCns2, &testCmp2) || - m_b2->GetUniquePred(m_comp) != m_b1) + if (!m_b2->hasSingleStmt() || m_b2->GetUniquePred(m_comp) != m_b1) { // The 2nd block has to be single-statement to avoid side-effects between the two conditions. // Also, make sure m_b2 has no other predecessors. return false; } - if (!testVar2->OperIs(GT_LCL_VAR) || !GenTree::Compare(testVar1->gtEffectiveVal(), testVar2)) - { - // Variables don't match in two conditions - // We use gtEffectiveVal() for the first block's variable to ignore COMMAs, e.g. - // - // m_b1: - // * JTRUE void - // \--* LT int - // +--* COMMA int - // | +--* STORE_LCL_VAR int V03 cse0 - // | | \--* CAST int <- ushort <- int - // | | \--* LCL_VAR int V01 arg1 - // | \--* LCL_VAR int V03 cse0 - // \--* CNS_INT int 97 - // - // m_b2: - // * JTRUE void - // \--* GT int - // +--* LCL_VAR int V03 cse0 - // \--* CNS_INT int 122 - // - return false; - } + GenTreeOp* cmp1 = m_b1->lastStmt()->GetRootNode()->gtGetOp1()->AsOp(); + GenTreeOp* cmp2 = m_b2->lastStmt()->GetRootNode()->gtGetOp1()->AsOp(); - // First condition always jumps-if-true to NotInRange, so let's reverse it to be "InRange" as - // a canonical form. - testCmp1 = GenTree::ReverseRelop(testCmp1); + // cmp1 is always reversed (see shape1 and shape2 above) + const bool cmp1IsReversed = true; - // For the 2nd condition it depends on the BB shape (see above). - testCmp2 = m_b2->HasJumpTo(notInRangeBb) ? GenTree::ReverseRelop(testCmp2) : testCmp2; + // cmp2 can be either reversed or not + const bool cmp2IsReversed = m_b2->HasJumpTo(notInRangeBb); - ssize_t rangeStart; - ssize_t rangeEnd; - if (!GetClosedRange(testCmp1, testCmp2, testCns1->IconValue(), testCns2->IconValue(), &rangeStart, &rangeEnd)) + if (!FoldRangeTests(m_comp, cmp1, cmp1IsReversed, cmp2, cmp2IsReversed)) { - // The range we test via two conditions is not a closed range - // TODO: We should support overlapped ranges here, e.g. "X > 10 && x > 100" -> "X > 100" return false; } - if (!FitsIn(testVar1->TypeGet(), rangeStart) || !FitsIn(testVar1->TypeGet(), rangeEnd)) - { - // Make sure constants fit into the types we work with - return false; - } - - genTreeOps newCmp = GT_GT; m_comp->fgAddRefPred(inRangeBb, m_b1); if (m_b1->HasJumpTo(m_b2->Next())) { // Re-direct firstBlock to jump to inRangeBb - newCmp = GenTree::ReverseRelop(newCmp); m_b1->SetJumpDest(inRangeBb); } @@ -631,25 +659,6 @@ bool OptBoolsDsc::optOptimizeRangeTests() m_comp->fgRemoveBlock(m_b2, true); Statement* stmt = m_b1->lastStmt(); - GenTreeOp* cmp = stmt->GetRootNode()->gtGetOp1()->AsOp(); - if (rangeStart == 0) - { - // We don't need to subtract anything, it's already 0-based - cmp->gtOp1 = testVar1; - } - else - { - // We need to subtract the rangeStartIncl from the variable to make the range start from 0 - cmp->gtOp1 = m_comp->gtNewOperNode(GT_SUB, testVar1->TypeGet(), testVar1, - m_comp->gtNewIconNode(rangeStart, testVar1->TypeGet())); - } - cmp->gtOp2->BashToConst(rangeEnd - rangeStart, testVar1->TypeGet()); - cmp->SetOper(newCmp); - cmp->SetUnsigned(); - - JITDUMP("Optimizing two conditional blocks representing a constant range test:\n") - DISPTREE(cmp) - m_comp->gtSetStmtInfo(stmt); m_comp->fgSetStmtSeq(stmt); m_comp->gtUpdateStmtSideEffects(stmt); From 2523ee6f955c51e94a5f2492d684e3c7d2c2a911 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 15 Oct 2023 17:45:52 +0200 Subject: [PATCH 5/8] Clean up --- src/coreclr/jit/optimizebools.cpp | 42 +++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index a80b9069a2ed7..08164e2323012 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -407,9 +407,18 @@ bool OptBoolsDsc::FindCompareChain(GenTree* condition, bool* isTestCondition) // Given two ranges, return true if they intersect and form a closed range. // Returns its bounds in pRangeStart and pRangeEnd (both are inclusive). -static bool GetClosedRange( - genTreeOps cmp1, genTreeOps cmp2, ssize_t cns1, ssize_t cns2, ssize_t* pRangeStart, ssize_t* pRangeEnd) +static bool GetIntersection(var_types type, + genTreeOps cmp1, + genTreeOps cmp2, + ssize_t cns1, + ssize_t cns2, + ssize_t* pRangeStart, + ssize_t* pRangeEnd) { + // We have two conditionals: + // "X [>,>=,<,<=] cns1" and "X [>,>=,<,<=] cns2" (X is always LHS) + // Calculate the intersection of the two ranges, if any. + // const bool range1IsStart = (cmp1 == GT_GE) || (cmp1 == GT_GT); const bool range1IsEnd = (cmp1 == GT_LE) || (cmp1 == GT_LT); const bool range1IsIncl = (cmp1 == GT_GE) || (cmp1 == GT_LE); @@ -420,6 +429,7 @@ static bool GetClosedRange( if ((range1IsStart == range2IsStart) || (range1IsEnd == range2IsEnd)) { + // Ranges have the same direction (we don't yet support that yet). return false; } @@ -440,12 +450,19 @@ static bool GetClosedRange( if ((*pRangeStart >= *pRangeEnd) || (*pRangeStart < 0) || (*pRangeEnd < 0)) { // TODO: If ranges don't intersect we might be able to fold the condition to true/false. + // TODO: support negative ranges. + return false; + } + + if (!FitsIn(type, *pRangeStart) || !FitsIn(type, *pRangeEnd)) + { return false; } + return true; } -// Given a compare node, return true if it is a constant range test. +// Given a compare node, return true if it is a constant range test, e.g. "X > 10" bool IsConstantRangeTest(GenTreeOp* tree, GenTree** varNode, GenTreeIntCon** cnsNode, genTreeOps* cmp) { if (tree->OperIs(GT_LE, GT_LT, GT_GE, GT_GT) && !tree->IsUnsigned()) @@ -489,6 +506,7 @@ bool FoldRangeTests(Compiler* comp, GenTreeOp* cmp1, bool cmp1IsReversed, GenTre // Make sure both conditions are constant range checks, e.g. "X > CNS" // TODO: support more cases, e.g. "X >= 0 && X < array.Length" -> "(uint)X < array.Length" + // Basically, we can use GenTree::IsNeverNegative() for it. if (!IsConstantRangeTest(cmp1, &var1Node, &cns1Node, &cmp1Op) || !IsConstantRangeTest(cmp2, &var2Node, &cns2Node, &cmp2Op)) { @@ -521,12 +539,14 @@ bool FoldRangeTests(Compiler* comp, GenTreeOp* cmp1, bool cmp1IsReversed, GenTre // +--* LCL_VAR int V03 cse0 // \--* CNS_INT int 122 // + // For the m_b2 we require the variable to be just a local with no side-effects (hence, no statements) return false; } ssize_t rangeStart; ssize_t rangeEnd; - if (!GetClosedRange(cmp1Op, cmp2Op, cns1Node->IconValue(), cns2Node->IconValue(), &rangeStart, &rangeEnd)) + if (!GetIntersection(var1Node->TypeGet(), cmp1Op, cmp2Op, cns1Node->IconValue(), cns2Node->IconValue(), &rangeStart, + &rangeEnd)) { // The range we test via two conditions is not a closed range // TODO: We should support overlapped ranges here, e.g. "X > 10 && x > 100" -> "X > 100" @@ -534,12 +554,6 @@ bool FoldRangeTests(Compiler* comp, GenTreeOp* cmp1, bool cmp1IsReversed, GenTre } assert(rangeStart < rangeEnd); - if (!FitsIn(var1Node->TypeGet(), rangeStart) || !FitsIn(var1Node->TypeGet(), rangeEnd)) - { - // Make sure constants fit into the types we work with - return false; - } - if (rangeStart == 0) { // We don't need to subtract anything, it's already 0-based @@ -566,7 +580,8 @@ bool FoldRangeTests(Compiler* comp, GenTreeOp* cmp1, bool cmp1IsReversed, GenTre // bool OptBoolsDsc::optOptimizeRangeTests() { - assert(m_b1 != nullptr && m_b2 != nullptr && m_b3 == nullptr); + // At this point we have two consecutive conditional blocks (BBJ_COND): m_b1 and m_b2 + assert((m_b1 != nullptr) && (m_b2 != nullptr) && (m_b3 == nullptr)); assert(m_b1->KindIs(BBJ_COND) && m_b2->KindIs(BBJ_COND) && m_b1->NextIs(m_b2)); if (m_b2->isRunRarely()) @@ -626,13 +641,14 @@ bool OptBoolsDsc::optOptimizeRangeTests() return false; } - if (!m_b2->hasSingleStmt() || m_b2->GetUniquePred(m_comp) != m_b1) + if (!m_b2->hasSingleStmt() || (m_b2->GetUniquePred(m_comp) != m_b1)) { // The 2nd block has to be single-statement to avoid side-effects between the two conditions. // Also, make sure m_b2 has no other predecessors. return false; } + // m_b1 and m_b2 are both BBJ_COND blocks with GT_JTRUE(cmp) root nodes GenTreeOp* cmp1 = m_b1->lastStmt()->GetRootNode()->gtGetOp1()->AsOp(); GenTreeOp* cmp2 = m_b2->lastStmt()->GetRootNode()->gtGetOp1()->AsOp(); @@ -648,7 +664,7 @@ bool OptBoolsDsc::optOptimizeRangeTests() } m_comp->fgAddRefPred(inRangeBb, m_b1); - if (m_b1->HasJumpTo(m_b2->Next())) + if (!cmp2IsReversed) { // Re-direct firstBlock to jump to inRangeBb m_b1->SetJumpDest(inRangeBb); From a90ac297e7662108b4330114a9ee133a81877ee7 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 15 Oct 2023 21:31:28 +0200 Subject: [PATCH 6/8] Clean up --- src/coreclr/jit/optimizebools.cpp | 60 +++++++++++++++++-------------- 1 file changed, 34 insertions(+), 26 deletions(-) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 08164e2323012..ff8c10736625e 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -415,47 +415,55 @@ static bool GetIntersection(var_types type, ssize_t* pRangeStart, ssize_t* pRangeEnd) { - // We have two conditionals: - // "X [>,>=,<,<=] cns1" and "X [>,>=,<,<=] cns2" (X is always LHS) - // Calculate the intersection of the two ranges, if any. - // - const bool range1IsStart = (cmp1 == GT_GE) || (cmp1 == GT_GT); - const bool range1IsEnd = (cmp1 == GT_LE) || (cmp1 == GT_LT); - const bool range1IsIncl = (cmp1 == GT_GE) || (cmp1 == GT_LE); + if ((cns1 < 0) || (cns2 < 0)) + { + // We don't yet support negative ranges. + return false; + } - const bool range2IsStart = (cmp2 == GT_GE) || (cmp2 == GT_GT); - const bool range2IsEnd = (cmp2 == GT_LE) || (cmp2 == GT_LT); - const bool range2IsIncl = (cmp2 == GT_GE) || (cmp2 == GT_LE); + // Convert to a canonical form with GT_GE or GT_LE (inclusive). + auto normalize = [](genTreeOps* cmp, ssize_t* cns) { + if (*cmp == GT_GT) + { + // "X > cns" -> "X >= cns + 1" + *cns = *cns + 1; + *cmp = GT_GE; + } + if (*cmp == GT_LT) + { + // "X < cns" -> "X <= cns - 1" + *cns = *cns - 1; + *cmp = GT_LE; + } + // whether these overflow or not is checked below. + }; + normalize(&cmp1, &cns1); + normalize(&cmp2, &cns2); - if ((range1IsStart == range2IsStart) || (range1IsEnd == range2IsEnd)) + if (cmp1 == cmp2) { // Ranges have the same direction (we don't yet support that yet). return false; } - // Report the intersection of the two ranges, add +/- 1 to make the bounds inclusive. - if (range1IsStart) + if (cmp1 == GT_GE) { - assert(range2IsEnd); - *pRangeStart = range1IsIncl ? cns1 : cns1 + 1; - *pRangeEnd = range2IsIncl ? cns2 : cns2 - 1; + *pRangeStart = cns1; + *pRangeEnd = cns2; } else { - assert(range1IsEnd && range2IsStart); - *pRangeStart = range2IsIncl ? cns2 : cns2 + 1; - *pRangeEnd = range1IsIncl ? cns1 : cns1 - 1; + assert(cmp1 == GT_LE); + *pRangeStart = cns2; + *pRangeEnd = cns1; } - if ((*pRangeStart >= *pRangeEnd) || (*pRangeStart < 0) || (*pRangeEnd < 0)) + if ((*pRangeStart >= *pRangeEnd) || (*pRangeStart < 0) || (*pRangeEnd < 0) || !FitsIn(type, *pRangeStart) || + !FitsIn(type, *pRangeEnd)) { // TODO: If ranges don't intersect we might be able to fold the condition to true/false. - // TODO: support negative ranges. - return false; - } - - if (!FitsIn(type, *pRangeStart) || !FitsIn(type, *pRangeEnd)) - { + // Also, check again if any of the ranges are negative (in case of overflow after normalization) + // and fits into the given type. return false; } From 4381e1410b67d1069d4837f6b72402a284a49806 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 17 Oct 2023 01:46:17 +0200 Subject: [PATCH 7/8] Add comments --- src/coreclr/jit/optimizebools.cpp | 35 ++++++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index ff8c10736625e..0cb4b19679d86 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -469,8 +469,20 @@ static bool GetIntersection(var_types type, return true; } - -// Given a compare node, return true if it is a constant range test, e.g. "X > 10" +//------------------------------------------------------------------------------ +// IsConstantRangeTest: Does the given compare node represent a constant range test? E.g. +// "X relop CNS" or "CNS relop X" where relop is [<, <=, >, >=] +// +// Arguments: +// tree - compare node +// varNode - [OUT] this will be set to the variable part of the constant range test +// cnsNode - [OUT] this will be set to the constant part of the constant range test +// cmp - [OUT] this will be set to a normalized compare operator so that the constant +// is always on the right hand side of the compare. +// +// Returns: +// true if the compare node represents a constant range test. +// bool IsConstantRangeTest(GenTreeOp* tree, GenTree** varNode, GenTreeIntCon** cnsNode, genTreeOps* cmp) { if (tree->OperIs(GT_LE, GT_LT, GT_GE, GT_GT) && !tree->IsUnsigned()) @@ -502,7 +514,24 @@ bool IsConstantRangeTest(GenTreeOp* tree, GenTree** varNode, GenTreeIntCon** cns return false; } -// Given two compare nodes, return true if they can be folded into a single range check (and fold them into cmp1) +//------------------------------------------------------------------------------ +// FoldRangeTests: Given two compare nodes (cmp1 && cmp2) that represent a range check, +// fold them into a single compare node if possible, e.g.: +// 1) "X >= 10 && X <= 100" -> "(X - 10) u<= 90" +// 2) "X >= 0 && X <= 100" -> "X u<= 100" +// where 'u' stands for unsigned comparison. cmp1 is used as the target node for folding. +// It's also guaranteed to be first in the execution order (so can allow some side effects). +// +// Arguments: +// compiler - compiler instance +// cmp1 - first compare node +// cmp1IsReversed - true if cmp1 is in fact reversed +// cmp2 - second compare node +// cmp2IsReversed - true if cmp2 is in fact reversed +// +// Returns: +// true if cmp1 now represents the folded range check and cmp2 can be removed. +// bool FoldRangeTests(Compiler* comp, GenTreeOp* cmp1, bool cmp1IsReversed, GenTreeOp* cmp2, bool cmp2IsReversed) { GenTree* var1Node; From aeac6703cf96bcad21d7421996ff9b1643dc6dc2 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 17 Oct 2023 01:51:49 +0200 Subject: [PATCH 8/8] Add comments for GetIntersection --- src/coreclr/jit/optimizebools.cpp | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 0cb4b19679d86..7f44a53407b7b 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -405,8 +405,25 @@ bool OptBoolsDsc::FindCompareChain(GenTree* condition, bool* isTestCondition) return false; } -// Given two ranges, return true if they intersect and form a closed range. -// Returns its bounds in pRangeStart and pRangeEnd (both are inclusive). +//------------------------------------------------------------------------------ +// GetIntersection: Given two ranges, return true if they intersect and form a closed range. +// Examples: +// >10 and <=20 -> [11,20] +// >10 and >100 -> false +// <10 and >10 -> false +// +// Arguments: +// type - The type of the compare nodes. +// cmp1 - The first compare node. +// cmp2 - The second compare node. +// cns1 - The constant value of the first compare node (always RHS). +// cns2 - The constant value of the second compare node (always RHS). +// pRangeStart - [OUT] The start of the intersection range (inclusive). +// pRangeEnd - [OUT] The end of the intersection range (inclusive). +// +// Returns: +// true if the ranges intersect and form a closed range. +// static bool GetIntersection(var_types type, genTreeOps cmp1, genTreeOps cmp2, @@ -469,6 +486,7 @@ static bool GetIntersection(var_types type, return true; } + //------------------------------------------------------------------------------ // IsConstantRangeTest: Does the given compare node represent a constant range test? E.g. // "X relop CNS" or "CNS relop X" where relop is [<, <=, >, >=]