From 226046d3fc47c7dccbd4f5cd2da8ed08ee93db56 Mon Sep 17 00:00:00 2001 From: Jonathan Davies Date: Wed, 26 Feb 2025 09:38:23 +0000 Subject: [PATCH 1/5] arm64: Add return cases into optimise bools * Fixes #96819 --- src/coreclr/jit/optimizebools.cpp | 45 +++++++---- src/tests/JIT/opt/InstructionCombining/Cmp.cs | 79 +++++++++++++++++++ 2 files changed, 108 insertions(+), 16 deletions(-) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 4a6057e122d014..1e33f42ba4854c 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -962,23 +962,26 @@ bool OptBoolsDsc::optOptimizeRangeTests() // bool OptBoolsDsc::optOptimizeCompareChainCondBlock() { - assert((m_b1 != nullptr) && (m_b2 != nullptr) && (m_b3 == nullptr)); + assert((m_b1 != nullptr) && (m_b2 != nullptr)); m_t3 = nullptr; bool foundEndOfOrConditions = false; - if (m_b1->FalseTargetIs(m_b2) && m_b2->FalseTargetIs(m_b1->GetTrueTarget())) - { - // Found the end of two (or more) conditions being ORed together. - // The final condition has been inverted. - foundEndOfOrConditions = true; - } - else if (m_b1->FalseTargetIs(m_b2) && m_b1->TrueTargetIs(m_b2->GetTrueTarget())) + if (m_b3 == nullptr) { - // Found two conditions connected together. - } - else - { - return false; + if (m_b1->FalseTargetIs(m_b2) && m_b2->FalseTargetIs(m_b1->GetTrueTarget())) + { + // Found the end of two (or more) conditions being ORed together. + // The final condition has been inverted. + foundEndOfOrConditions = true; + } + else if (m_b1->FalseTargetIs(m_b2) && m_b1->TrueTargetIs(m_b2->GetTrueTarget())) + { + // Found two conditions connected together. + } + else + { + return false; + } } Statement* const s1 = optOptimizeBoolsChkBlkCond(); @@ -990,7 +993,7 @@ bool OptBoolsDsc::optOptimizeCompareChainCondBlock() assert(m_testInfo1.testTree->OperIs(GT_JTRUE)); GenTree* cond1 = m_testInfo1.testTree->gtGetOp1(); - assert(m_testInfo2.testTree->OperIs(GT_JTRUE)); + assert(m_testInfo2.testTree->OperIs((m_b3 == nullptr) ? GT_JTRUE : GT_RETURN)); GenTree* cond2 = m_testInfo2.testTree->gtGetOp1(); // Ensure both conditions are suitable. @@ -1031,8 +1034,8 @@ bool OptBoolsDsc::optOptimizeCompareChainCondBlock() int op1Cost = cond1->GetCostEx(); int op2Cost = cond2->GetCostEx(); // The cost of combing three simple conditions is 32. - int maxOp1Cost = op1IsCondChain ? 31 : 7; - int maxOp2Cost = op2IsCondChain ? 31 : 7; + int maxOp1Cost = op1IsCondChain ? 31 : 10; + int maxOp2Cost = op2IsCondChain ? 31 : 10; // Cost to allow for chain size of three. if (op1Cost > maxOp1Cost || op2Cost > maxOp2Cost) @@ -2011,6 +2014,16 @@ PhaseStatus Compiler::optOptimizeBools() change = true; numReturn++; } +#ifdef TARGET_ARM64 + else if (optBoolsDsc.optOptimizeCompareChainCondBlock()) + { + // The optimization will have merged b1 and b2. Retry the loop so that + // b1 and b2->bbNext can be tested. + change = true; + retry = true; + numCond++; + } +#endif } else { diff --git a/src/tests/JIT/opt/InstructionCombining/Cmp.cs b/src/tests/JIT/opt/InstructionCombining/Cmp.cs index 0ce7de0faee544..409451ac9f4853 100644 --- a/src/tests/JIT/opt/InstructionCombining/Cmp.cs +++ b/src/tests/JIT/opt/InstructionCombining/Cmp.cs @@ -50,6 +50,31 @@ public static int CheckCompare() fail = true; } + if (!CmpOptimizeBoolsReturn(5, 3, 10)) + { + fail = true; + } + + if (!CmpOptimizeBoolsReturnOr(2, 3, 4)) + { + fail = true; + } + + if (CmpOptimizeBoolsIf(5, 3, 10) != 1) + { + fail = true; + } + + if (CmpOptimizeBoolsIfOr(2, 3, 4) != 1) + { + fail = true; + } + + if (!CmpMultipleOptimizeBools(1, 2, 3, 3, 5, 4)) + { + fail = true; + } + if (fail) { return 101; @@ -134,5 +159,59 @@ static bool CmpLargeShift64Bit(long a, long b) } return false; } + + [MethodImpl(MethodImplOptions.NoInlining)] + static bool CmpOptimizeBoolsReturn(int a, int lower, int upper) + { + //ARM64-FULL-LINE: cmp {{w[0-9]+}}, {{w[0-9]+}} + //ARM64-FULL-LINE: ccmp {{w[0-9]+}}, {{w[0-9]+}}, nzc, ge + //ARM64-FULL-LINE: cset {{x[0-9]+}}, le + return a >= lower && a <= upper; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static bool CmpOptimizeBoolsReturnOr(int a, int lower, int upper) + { + //ARM64-FULL-LINE: cmp {{w[0-9]+}}, {{w[0-9]+}} + //ARM64-FULL-LINE: ccmp {{w[0-9]+}}, {{w[0-9]+}}, nzc, lt + //ARM64-FULL-LINE: cset {{x[0-9]+}}, le + return a >= lower || a <= upper; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int CmpOptimizeBoolsIf(int a, int lower, int upper) + { + //ARM64-FULL-LINE: cmp {{w[0-9]+}}, {{w[0-9]+}} + //ARM64-FULL-LINE: ccmp {{w[0-9]+}}, {{w[0-9]+}}, 0, ge + //ARM64-FULL-LINE: csel {{w[0-9]+}}, {{w[0-9]+}}, {{w[0-9]+}}, gt + if (a >= lower && a <= upper) + { + return 1; + } + return -1; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int CmpOptimizeBoolsIfOr(int a, int lower, int upper) + { + //ARM64-FULL-LINE: cmp {{w[0-9]+}}, {{w[0-9]+}} + //ARM64-FULL-LINE: ccmp {{w[0-9]+}}, {{w[0-9]+}}, nzc, lt + //ARM64-FULL-LINE: csel {{w[0-9]+}}, {{w[0-9]+}}, {{w[0-9]+}}, gt + if (a >= lower || a <= upper) + { + return 1; + } + return -1; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static bool CmpMultipleOptimizeBools(int a, int b, int c, int d, int e, int f) + { + //ARM64-FULL-LINE: cmp {{w[0-9]+}}, {{w[0-9]+}} + //ARM64-FULL-LINE: ccmp {{w[0-9]+}}, {{w[0-9]+}}, 0, lt + //ARM64-FULL-LINE: ccmp {{w[0-9]+}}, {{w[0-9]+}}, 0, le + //ARM64-FULL-LINE: cset {{x[0-9]+}}, gt + return (a < b) && (c <= d) && (e > f); + } } } From dcb022cf6dde6b8d96e475d9794461b5c2c61478 Mon Sep 17 00:00:00 2001 From: Jonathan Davies Date: Thu, 6 Mar 2025 08:59:55 +0000 Subject: [PATCH 2/5] Reverse first condition when 3rd block is returning 0 --- src/coreclr/jit/optimizebools.cpp | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 1e33f42ba4854c..6730b340c5dbe4 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -965,14 +965,14 @@ bool OptBoolsDsc::optOptimizeCompareChainCondBlock() assert((m_b1 != nullptr) && (m_b2 != nullptr)); m_t3 = nullptr; - bool foundEndOfOrConditions = false; + bool reverseFirstCondition = false; if (m_b3 == nullptr) { if (m_b1->FalseTargetIs(m_b2) && m_b2->FalseTargetIs(m_b1->GetTrueTarget())) { // Found the end of two (or more) conditions being ORed together. // The final condition has been inverted. - foundEndOfOrConditions = true; + reverseFirstCondition = true; } else if (m_b1->FalseTargetIs(m_b2) && m_b1->TrueTargetIs(m_b2->GetTrueTarget())) { @@ -983,6 +983,18 @@ bool OptBoolsDsc::optOptimizeCompareChainCondBlock() return false; } } + else + { + GenTree* root = m_b3->firstStmt()->GetRootNode(); + if (root->OperIsSimple()) + { + GenTree* op1 = root->gtGetOp1(); + if (op1 && op1->IsIntegralConst(0)) + { + reverseFirstCondition = true; + } + } + } Statement* const s1 = optOptimizeBoolsChkBlkCond(); if (s1 == nullptr) @@ -1051,14 +1063,14 @@ bool OptBoolsDsc::optOptimizeCompareChainCondBlock() m_comp->fgRemoveStmt(m_b1, s1 DEBUGARG(isUnlink)); // Invert the condition. - if (foundEndOfOrConditions) + if (reverseFirstCondition) { GenTree* revCond = m_comp->gtReverseCond(cond1); assert(cond1 == revCond); // Ensure `gtReverseCond` did not create a new node. } // Join the two conditions together - genTreeOps chainedOper = foundEndOfOrConditions ? GT_AND : GT_OR; + genTreeOps chainedOper = reverseFirstCondition ? GT_AND : GT_OR; GenTree* chainedConditions = m_comp->gtNewOperNode(chainedOper, TYP_INT, cond1, cond2); cond1->gtFlags &= ~GTF_RELOP_JMP_USED; cond2->gtFlags &= ~GTF_RELOP_JMP_USED; From 3dd01c8f5a1462be9846e019ea2e2dc1845111a1 Mon Sep 17 00:00:00 2001 From: Jonathan Davies Date: Thu, 6 Mar 2025 12:56:56 +0000 Subject: [PATCH 3/5] Fix Cmp tests --- src/tests/JIT/opt/InstructionCombining/Cmp.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tests/JIT/opt/InstructionCombining/Cmp.cs b/src/tests/JIT/opt/InstructionCombining/Cmp.cs index 409451ac9f4853..38326308cd8645 100644 --- a/src/tests/JIT/opt/InstructionCombining/Cmp.cs +++ b/src/tests/JIT/opt/InstructionCombining/Cmp.cs @@ -164,7 +164,7 @@ static bool CmpLargeShift64Bit(long a, long b) static bool CmpOptimizeBoolsReturn(int a, int lower, int upper) { //ARM64-FULL-LINE: cmp {{w[0-9]+}}, {{w[0-9]+}} - //ARM64-FULL-LINE: ccmp {{w[0-9]+}}, {{w[0-9]+}}, nzc, ge + //ARM64-FULL-LINE: ccmp {{w[0-9]+}}, {{w[0-9]+}}, 0, ge //ARM64-FULL-LINE: cset {{x[0-9]+}}, le return a >= lower && a <= upper; } @@ -209,7 +209,7 @@ static bool CmpMultipleOptimizeBools(int a, int b, int c, int d, int e, int f) { //ARM64-FULL-LINE: cmp {{w[0-9]+}}, {{w[0-9]+}} //ARM64-FULL-LINE: ccmp {{w[0-9]+}}, {{w[0-9]+}}, 0, lt - //ARM64-FULL-LINE: ccmp {{w[0-9]+}}, {{w[0-9]+}}, 0, le + //ARM64-FULL-LINE: ccmp {{w[0-9]+}}, {{w[0-9]+}}, nzc, le //ARM64-FULL-LINE: cset {{x[0-9]+}}, gt return (a < b) && (c <= d) && (e > f); } From a3a81dfffa4a955047261b23eb566944a3436f82 Mon Sep 17 00:00:00 2001 From: Jonathan Davies Date: Fri, 7 Mar 2025 11:45:37 +0000 Subject: [PATCH 4/5] Moved checking block 3 to reverse condition after calling optOptimizeBoolsChkBlkCond() --- src/coreclr/jit/optimizebools.cpp | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 6730b340c5dbe4..3ce134ec37fde2 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -983,18 +983,6 @@ bool OptBoolsDsc::optOptimizeCompareChainCondBlock() return false; } } - else - { - GenTree* root = m_b3->firstStmt()->GetRootNode(); - if (root->OperIsSimple()) - { - GenTree* op1 = root->gtGetOp1(); - if (op1 && op1->IsIntegralConst(0)) - { - reverseFirstCondition = true; - } - } - } Statement* const s1 = optOptimizeBoolsChkBlkCond(); if (s1 == nullptr) @@ -1008,6 +996,17 @@ bool OptBoolsDsc::optOptimizeCompareChainCondBlock() assert(m_testInfo2.testTree->OperIs((m_b3 == nullptr) ? GT_JTRUE : GT_RETURN)); GenTree* cond2 = m_testInfo2.testTree->gtGetOp1(); + if (m_b3 != nullptr) + { + GenTree* root = m_b3->firstStmt()->GetRootNode(); + assert(root->OperIs(GT_RETURN)); + GenTree* op1 = root->gtGetOp1(); + if (op1 && op1->IsIntegralConst(0)) + { + reverseFirstCondition = true; + } + } + // Ensure both conditions are suitable. if (!cond1->OperIsCompare() || !cond2->OperIsCompare()) { @@ -1172,7 +1171,7 @@ Statement* OptBoolsDsc::optOptimizeBoolsChkBlkCond() } else { - if (!testTree2->OperIs(GT_RETURN, GT_SWIFT_ERROR_RET)) + if (!testTree2->OperIs(GT_RETURN)) { return nullptr; } @@ -1184,7 +1183,7 @@ Statement* OptBoolsDsc::optOptimizeBoolsChkBlkCond() } GenTree* testTree3 = s3->GetRootNode(); - if (!testTree3->OperIs(GT_RETURN, GT_SWIFT_ERROR_RET)) + if (!testTree3->OperIs(GT_RETURN)) { return nullptr; } From dbbbcb9032e4ccf39a7ca9bc325089e58db2737b Mon Sep 17 00:00:00 2001 From: Jonathan Davies Date: Fri, 7 Mar 2025 13:05:23 +0000 Subject: [PATCH 5/5] assert block 2 & 3 are block 1's false & true targets respectively --- src/coreclr/jit/optimizebools.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 3ce134ec37fde2..083dba8eced7d9 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -998,6 +998,7 @@ bool OptBoolsDsc::optOptimizeCompareChainCondBlock() if (m_b3 != nullptr) { + assert(m_b1->FalseTargetIs(m_b2) && m_b1->TrueTargetIs(m_b3)); GenTree* root = m_b3->firstStmt()->GetRootNode(); assert(root->OperIs(GT_RETURN)); GenTree* op1 = root->gtGetOp1();