Reduce duplication from optSwitchDetectLikely.

Author: anthonycanino

src/coreclr/jit/compiler.h

@@ -7198,10 +7198,7 @@ class Compiler
     PhaseStatus optOptimizeBools();
     PhaseStatus optSwitchRecognition();
     bool optSwitchConvert(BasicBlock* firstBlock, int testsCount, ssize_t* testValues, weight_t falseLikelihood, GenTree* nodeToTest);
-    bool optSwitchDetectAndConvert(BasicBlock* firstBlock);
-#if defined(TARGET_AMD64)
-    bool optSwitchDetectLikely(BasicBlock* firstBlock);
-#endif
+    bool optSwitchDetectAndConvert(BasicBlock* firstBlock, bool testingForConversion = false);
 
     PhaseStatus optInvertLoops();    // Invert loops so they're entered at top and tested at bottom.
     PhaseStatus optOptimizeFlow();   // Simplify flow graph and do tail duplication

src/coreclr/jit/emitxarch.cpp

@@ -4451,7 +4451,8 @@ inline unsigned emitter::insEncodeRegSIB(const instrDesc* id, regNumber reg, cod
             }
             else if (hasEvexPrefix(*code))
             {
-                // Note that APX-EVEX use EVEX.X4 as the MSB of the INDEX register to address GPRs, and the original EVEX.V4 is used for VSIB addressing.
+                // Note that APX-EVEX use EVEX.X4 as the MSB of the INDEX register to address GPRs, and the original
+                // EVEX.V4 is used for VSIB addressing.
                 *code &= 0xFFFFFBFFFFFFFFFFULL; // EVEX.X4
             }
             else
@@ -15399,7 +15400,7 @@ BYTE* emitter::emitOutputSV(BYTE* dst, instrDesc* id, code_t code, CnsVal* addc)
                     {
                         code |= EXTENDED_EVEX_PP_BITS;
                     }
-#endif  // TARGET_AMD64
+#endif // TARGET_AMD64
                 }
                 FALLTHROUGH;
 
@@ -17026,15 +17027,14 @@ BYTE* emitter::emitOutputRI(BYTE* dst, instrDesc* id)
         code = insCodeACC(ins);
         assert(code < 0x100);
 
-        
         // This is INS_mov and will not take VEX prefix
         assert(!TakesVexPrefix(ins));
-        
+
         code = AddX86PrefixIfNeededAndNotPresent(id, code, size);
         code |= 0x08; // Set the 'w' bit
         unsigned regcode = insEncodeReg012(id, reg, size, &code);
         code |= regcode;
-        
+
         if (TakesRexWPrefix(id))
         {
             code = AddRexWPrefix(id, code);

src/coreclr/jit/optimizebools.cpp

@@ -1991,7 +1991,7 @@ PhaseStatus Compiler::optOptimizeBools()
                 // trigger or not
                 // else if ((compOpportunisticallyDependsOn(InstructionSet_APX) || JitConfig.JitEnableApxIfConv()) &&
                 // optBoolsDsc.optOptimizeCompareChainCondBlock())
-                else if (JitConfig.EnableApxConditionalChaining() && !optSwitchDetectLikely(b1) &&
+                else if (JitConfig.EnableApxConditionalChaining() && !optSwitchDetectAndConvert(b1, true) &&
                          optBoolsDsc.optOptimizeCompareChainCondBlock())
                 {
                     // The optimization will have merged b1 and b2. Retry the loop so that

src/coreclr/jit/switchrecognition.cpp

@@ -131,120 +131,21 @@ bool IsConstantTestCondBlock(const BasicBlock* block,
     return false;
 }
 
-#if defined(TARGET_AMD64)
-//------------------------------------------------------------------------------
-// optSwitchDetectLikely : Return true if it is likely this block chain
-//    can be converted into a switch at a later optimization pass
-//
-// Arguments:
-//    firstBlock - A block to start the search from
-//
-// Return Value:
-//    True if the conversion was successful, false otherwise
-//
-bool Compiler::optSwitchDetectLikely(BasicBlock* firstBlock)
-{
-    assert(firstBlock->KindIs(BBJ_COND));
-
-    GenTree*    variableNode = nullptr;
-    ssize_t     cns          = 0;
-    BasicBlock* trueTarget   = nullptr;
-    BasicBlock* falseTarget  = nullptr;
-
-    // The algorithm is simple - we check that the given block is a constant test block
-    // and then try to accumulate as many constant test blocks as possible. Once we hit
-    // a block that doesn't match the pattern, we start processing the accumulated blocks.
-    bool isReversed = false;
-    if (IsConstantTestCondBlock(firstBlock, true, &trueTarget, &falseTarget, &isReversed, &variableNode, &cns))
-    {
-        if (isReversed)
-        {
-            // First block uses NE - we don't support this yet. We currently expect all blocks to use EQ
-            // and allow NE for the last one (because it's what Roslyn usually emits).
-            // TODO: make it more flexible and support cases like "x != cns1 && x != cns2 && ..."
-            return false;
-        }
-
-        // No more than SWITCH_MAX_TABLE_SIZE blocks are allowed (arbitrary limit in this context)
-        int     testValueIndex                  = 0;
-        ssize_t testValues[SWITCH_MAX_DISTANCE] = {};
-        testValues[testValueIndex]              = cns;
-        testValueIndex++;
-
-        // Track likelihood of reaching the false block
-        //
-        weight_t          falseLikelihood = firstBlock->GetFalseEdge()->getLikelihood();
-        const BasicBlock* prevBlock       = firstBlock;
-
-        // Follow the same algorithm as below but only peek to the next block
-        const BasicBlock* currBb = firstBlock->Next();
-        if (currBb != nullptr)
-        {
-            GenTree*    currVariableNode = nullptr;
-            ssize_t     currCns          = 0;
-            BasicBlock* currTrueTarget   = nullptr;
-            BasicBlock* currFalseTarget  = nullptr;
-
-            if (!currBb->hasSingleStmt())
-            {
-                // Only the first conditional block can have multiple statements.
-                // Stop searching and process what we already have.
-                return false;
-            }
-
-            // Inspect secondary blocks
-            if (IsConstantTestCondBlock(currBb, false, &currTrueTarget, &currFalseTarget, &isReversed,
-                                        &currVariableNode, &currCns))
-            {
-                if (currTrueTarget != trueTarget)
-                {
-                    // This blocks jumps to a different target, stop searching and process what we already have.
-                    return false;
-                }
-
-                if (!GenTree::Compare(currVariableNode, variableNode->gtEffectiveVal()))
-                {
-                    // A different variable node is used, stop searching and process what we already have.
-                    return false;
-                }
-
-                if (currBb->GetUniquePred(this) != prevBlock)
-                {
-                    return false;
-                }
-
-                if (!BasicBlock::sameEHRegion(prevBlock, currBb))
-                {
-                    // Current block is in a different EH region, stop searching and process what we already have.
-                    return false;
-                }
-
-                return true;
-            }
-            else
-            {
-                // Current block is not a suitable test, stop searching and process what we already have.
-                return false;
-            }
-        }
-    }
-
-    return false;
-}
-#endif
-
 //------------------------------------------------------------------------------
 // optSwitchDetectAndConvert : Try to detect a series of conditional blocks which
 //    can be converted into a switch (jump-table) construct. See optSwitchConvert
 //    for more details.
 //
 // Arguments:
 //    firstBlock - A block to start the search from
+//    testingForConversion - Test if its likely a switch conversion will happen.
+//    Used to prevent a pessimization when optimizing for conditional chaining.
+//    Done in this function to prevent maintaining the check in two places.
 //
 // Return Value:
 //    True if the conversion was successful, false otherwise
 //
-bool Compiler::optSwitchDetectAndConvert(BasicBlock* firstBlock)
+bool Compiler::optSwitchDetectAndConvert(BasicBlock* firstBlock, bool testingForConversion)
 {
     assert(firstBlock->KindIs(BBJ_COND));
 
@@ -289,7 +190,8 @@ bool Compiler::optSwitchDetectAndConvert(BasicBlock* firstBlock)
             {
                 // Only the first conditional block can have multiple statements.
                 // Stop searching and process what we already have.
-                return optSwitchConvert(firstBlock, testValueIndex, testValues, falseLikelihood, variableNode);
+                return !testingForConversion &&
+                       optSwitchConvert(firstBlock, testValueIndex, testValues, falseLikelihood, variableNode);
             }
 
             // Inspect secondary blocks
@@ -299,25 +201,29 @@ bool Compiler::optSwitchDetectAndConvert(BasicBlock* firstBlock)
                 if (currTrueTarget != trueTarget)
                 {
                     // This blocks jumps to a different target, stop searching and process what we already have.
-                    return optSwitchConvert(firstBlock, testValueIndex, testValues, falseLikelihood, variableNode);
+                    return !testingForConversion &&
+                           optSwitchConvert(firstBlock, testValueIndex, testValues, falseLikelihood, variableNode);
                 }
 
                 if (!GenTree::Compare(currVariableNode, variableNode->gtEffectiveVal()))
                 {
                     // A different variable node is used, stop searching and process what we already have.
-                    return optSwitchConvert(firstBlock, testValueIndex, testValues, falseLikelihood, variableNode);
+                    return !testingForConversion &&
+                           optSwitchConvert(firstBlock, testValueIndex, testValues, falseLikelihood, variableNode);
                 }
 
                 if (currBb->GetUniquePred(this) != prevBlock)
                 {
                     // Multiple preds in a secondary block, stop searching and process what we already have.
-                    return optSwitchConvert(firstBlock, testValueIndex, testValues, falseLikelihood, variableNode);
+                    return !testingForConversion &&
+                           optSwitchConvert(firstBlock, testValueIndex, testValues, falseLikelihood, variableNode);
                 }
 
                 if (!BasicBlock::sameEHRegion(prevBlock, currBb))
                 {
                     // Current block is in a different EH region, stop searching and process what we already have.
-                    return optSwitchConvert(firstBlock, testValueIndex, testValues, falseLikelihood, variableNode);
+                    return !testingForConversion &&
+                           optSwitchConvert(firstBlock, testValueIndex, testValues, falseLikelihood, variableNode);
                 }
 
                 // Ok we can work with that, add the test value to the list
@@ -327,21 +233,27 @@ bool Compiler::optSwitchDetectAndConvert(BasicBlock* firstBlock)
                 if (testValueIndex == SWITCH_MAX_DISTANCE)
                 {
                     // Too many suitable tests found - stop and process what we already have.
-                    return optSwitchConvert(firstBlock, testValueIndex, testValues, falseLikelihood, variableNode);
+                    return !testingForConversion &&
+                           optSwitchConvert(firstBlock, testValueIndex, testValues, falseLikelihood, variableNode);
                 }
 
                 if (isReversed)
                 {
                     // We only support reversed test (GT_NE) for the last block.
-                    return optSwitchConvert(firstBlock, testValueIndex, testValues, falseLikelihood, variableNode);
+                    return !testingForConversion &&
+                           optSwitchConvert(firstBlock, testValueIndex, testValues, falseLikelihood, variableNode);
                 }
 
+                if (testingForConversion)
+                    return true;
+
                 prevBlock = currBb;
             }
             else
             {
                 // Current block is not a suitable test, stop searching and process what we already have.
-                return optSwitchConvert(firstBlock, testValueIndex, testValues, falseLikelihood, variableNode);
+                return !testingForConversion &&
+                       optSwitchConvert(firstBlock, testValueIndex, testValues, falseLikelihood, variableNode);
             }
         }
     }