From ffb5b12257d3f1c0338f3f3dc988bb1836691b00 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 28 Nov 2024 16:51:47 -0500 Subject: [PATCH 1/3] Remove lexical dependencies in optSwitchConvert --- src/coreclr/jit/switchrecognition.cpp | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/switchrecognition.cpp b/src/coreclr/jit/switchrecognition.cpp index 42424514408990..3bf1eb74ebeaaa 100644 --- a/src/coreclr/jit/switchrecognition.cpp +++ b/src/coreclr/jit/switchrecognition.cpp @@ -323,9 +323,16 @@ bool Compiler::optSwitchConvert( // Find the last block in the chain const BasicBlock* lastBlock = firstBlock; - for (int i = 0; i < testsCount - 1; i++) + for (int i = 0; i < (testsCount - 1); i++) { - lastBlock = lastBlock->Next(); + const GenTree* rootNode = lastBlock->lastStmt()->GetRootNode(); + assert(lastBlock->KindIs(BBJ_COND)); + assert(rootNode->OperIs(GT_JTRUE)); + + // We only support reversed tests (GT_NE) in the last block of the chain. + // TODO: Remove this restriction. + assert(rootNode->gtGetOp1()->OperIs(GT_EQ)); + lastBlock = lastBlock->GetFalseTarget(); } BasicBlock* blockIfTrue = nullptr; @@ -360,9 +367,14 @@ bool Compiler::optSwitchConvert( fgRemoveRefPred(falseEdge); BasicBlock* blockToRemove = falseEdge->getDestinationBlock(); assert(firstBlock->NextIs(blockToRemove)); - while (!lastBlock->NextIs(blockToRemove)) + for (int i = 0; i < (testsCount - 1); i++) { - blockToRemove = fgRemoveBlock(blockToRemove, true); + // We always follow the false target because reversed tests are only supported for the last block. + // TODO: Lift this restriction, and follow the true target if the test is reversed. + assert(blockToRemove->KindIs(BBJ_COND)); + BasicBlock* const nextBlockToRemove = blockToRemove->GetFalseTarget(); + fgRemoveBlock(blockToRemove, true); + blockToRemove = nextBlockToRemove; } const unsigned jumpCount = static_cast(maxValue - minValue + 1); @@ -405,6 +417,8 @@ bool Compiler::optSwitchConvert( } } + assert(switchTrueEdge != nullptr); + // Link the 'default' case FlowEdge* const switchDefaultEdge = fgAddRefPred(blockIfFalse, firstBlock); jmpTab[jumpCount] = switchDefaultEdge; From 1d8447b4955387efbf2aaef925df85fd30f47cb8 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 28 Nov 2024 17:06:48 -0500 Subject: [PATCH 2/3] Remove lexical dependencies in optSwitchDetectAndConvert --- src/coreclr/jit/switchrecognition.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/switchrecognition.cpp b/src/coreclr/jit/switchrecognition.cpp index 3bf1eb74ebeaaa..97afc13c3928f5 100644 --- a/src/coreclr/jit/switchrecognition.cpp +++ b/src/coreclr/jit/switchrecognition.cpp @@ -51,8 +51,8 @@ PhaseStatus Compiler::optSwitchRecognition() // Arguments: // block - The block to check // allowSideEffects - is variableNode allowed to have side-effects (COMMA)? -// trueEdge - [out] The successor edge taken if X == CNS -// falseEdge - [out] The successor edge taken if X != CNS +// trueTarget - [out] The successor visited if X == CNS +// falseTarget - [out] The successor visited if X != CNS // isReversed - [out] True if the condition is reversed (GT_NE) // variableNode - [out] The variable node (X in the example above) // cns - [out] The constant value (CNS in the example above) @@ -176,13 +176,12 @@ bool Compiler::optSwitchDetectAndConvert(BasicBlock* firstBlock) weight_t falseLikelihood = firstBlock->GetFalseEdge()->getLikelihood(); const BasicBlock* prevBlock = firstBlock; - // Now walk the next blocks and see if they are basically the same type of test - for (const BasicBlock* currBb = firstBlock->Next(); currBb != nullptr; currBb = currBb->Next()) + // Now walk the chain of test blocks, and see if they are basically the same type of test + for (BasicBlock *currBb = falseTarget, *currFalseTarget; currBb != nullptr; currBb = currFalseTarget) { GenTree* currVariableNode = nullptr; ssize_t currCns = 0; BasicBlock* currTrueTarget = nullptr; - BasicBlock* currFalseTarget = nullptr; if (!currBb->hasSingleStmt()) { @@ -366,7 +365,6 @@ bool Compiler::optSwitchConvert( // Unlink and remove the whole chain of conditional blocks fgRemoveRefPred(falseEdge); BasicBlock* blockToRemove = falseEdge->getDestinationBlock(); - assert(firstBlock->NextIs(blockToRemove)); for (int i = 0; i < (testsCount - 1); i++) { // We always follow the false target because reversed tests are only supported for the last block. From 4cb4be753d01e3463c0f3926fac74e334d0d8047 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 28 Nov 2024 19:26:50 -0500 Subject: [PATCH 3/3] Remove TODOs --- src/coreclr/jit/switchrecognition.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/coreclr/jit/switchrecognition.cpp b/src/coreclr/jit/switchrecognition.cpp index 97afc13c3928f5..7329194cb10cd2 100644 --- a/src/coreclr/jit/switchrecognition.cpp +++ b/src/coreclr/jit/switchrecognition.cpp @@ -329,7 +329,6 @@ bool Compiler::optSwitchConvert( assert(rootNode->OperIs(GT_JTRUE)); // We only support reversed tests (GT_NE) in the last block of the chain. - // TODO: Remove this restriction. assert(rootNode->gtGetOp1()->OperIs(GT_EQ)); lastBlock = lastBlock->GetFalseTarget(); } @@ -368,7 +367,6 @@ bool Compiler::optSwitchConvert( for (int i = 0; i < (testsCount - 1); i++) { // We always follow the false target because reversed tests are only supported for the last block. - // TODO: Lift this restriction, and follow the true target if the test is reversed. assert(blockToRemove->KindIs(BBJ_COND)); BasicBlock* const nextBlockToRemove = blockToRemove->GetFalseTarget(); fgRemoveBlock(blockToRemove, true);