From 04e531f2256834cf5fd8431ec2213a616e0d7e30 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 7 Sep 2024 15:23:47 +0200 Subject: [PATCH 01/37] Expand "return condition" into "return condition ? true : false" --- src/coreclr/jit/fgopt.cpp | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 5a71d474b6f365..a670c156fc1af1 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -5274,6 +5274,44 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bDest = nullptr; bFalseDest = nullptr; + // Expand "return " into "return ? true : false" + // since the latter is more friendly to various optimizations. The newly added tails are expected to be + // deduplicated where needed or/and the condition will be transformed back to branch-less version where + // profitable. + if (doTailDuplication && (info.compRetType == TYP_UBYTE) && block->KindIs(BBJ_RETURN) && + (block->lastStmt() != nullptr)) + { + GenTree* rootNode = block->lastStmt()->GetRootNode(); + if (rootNode->OperIs(GT_RETURN) && rootNode->gtGetOp1()->OperIsCompare() && + rootNode->gtGetOp1()->gtGetOp2()->IsIntegralConst()) + { + assert(rootNode->TypeIs(TYP_INT)); + + rootNode->gtGetOp1()->gtFlags |= (GTF_RELOP_JMP_USED | GTF_DONT_CSE); + rootNode->ChangeOper(GT_JTRUE); + + DebugInfo dbgInfo = block->lastStmt()->GetDebugInfo(); + BasicBlock* retTrueBb = + fgNewBBFromTreeAfter(BBJ_RETURN, block, gtNewOperNode(GT_RETURN, TYP_INT, gtNewTrue()), + dbgInfo); + BasicBlock* retFalseBb = + fgNewBBFromTreeAfter(BBJ_RETURN, block, gtNewOperNode(GT_RETURN, TYP_INT, gtNewFalse()), + dbgInfo); + + FlowEdge* trueEdge = fgAddRefPred(retTrueBb, block); + FlowEdge* falseEdge = fgAddRefPred(retFalseBb, block); + trueEdge->setLikelihood(0.5); + falseEdge->setLikelihood(0.5); + block->SetCond(trueEdge, falseEdge); + + change = true; + modified = true; + bDest = block->GetTrueTarget(); + bNext = block->GetFalseTarget(); + bFalseDest = block->GetFalseTarget(); + } + } + if (block->KindIs(BBJ_ALWAYS)) { bDest = block->GetTarget(); From 5b17833e3eb588a1ecef99b7a0a69f77febb5e45 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 7 Sep 2024 15:49:19 +0200 Subject: [PATCH 02/37] remove unnecessary limitation --- src/coreclr/jit/fgopt.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index a670c156fc1af1..ec6fa5a40f3f5c 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -5282,8 +5282,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, (block->lastStmt() != nullptr)) { GenTree* rootNode = block->lastStmt()->GetRootNode(); - if (rootNode->OperIs(GT_RETURN) && rootNode->gtGetOp1()->OperIsCompare() && - rootNode->gtGetOp1()->gtGetOp2()->IsIntegralConst()) + if (rootNode->OperIs(GT_RETURN) && rootNode->gtGetOp1()->OperIsCmpCompare()) { assert(rootNode->TypeIs(TYP_INT)); From 8ca7a223bde09a68b74c36eee76d804557a0ab37 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 7 Sep 2024 16:26:38 +0200 Subject: [PATCH 03/37] clean up --- src/coreclr/jit/fgopt.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index ec6fa5a40f3f5c..932bc55dbf7c0e 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -5279,7 +5279,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, // deduplicated where needed or/and the condition will be transformed back to branch-less version where // profitable. if (doTailDuplication && (info.compRetType == TYP_UBYTE) && block->KindIs(BBJ_RETURN) && - (block->lastStmt() != nullptr)) + (block->lastStmt() != nullptr) && (block != genReturnBB) && !block->isRunRarely()) { GenTree* rootNode = block->lastStmt()->GetRootNode(); if (rootNode->OperIs(GT_RETURN) && rootNode->gtGetOp1()->OperIsCmpCompare()) @@ -5303,11 +5303,13 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, falseEdge->setLikelihood(0.5); block->SetCond(trueEdge, falseEdge); - change = true; - modified = true; - bDest = block->GetTrueTarget(); - bNext = block->GetFalseTarget(); - bFalseDest = block->GetFalseTarget(); + assert(BasicBlock::sameEHRegion(block, retTrueBb)); + assert(BasicBlock::sameEHRegion(block, retFalseBb)); + + change = true; + modified = true; + bDest = block->GetTrueTarget(); + bNext = block->GetFalseTarget(); } } From c1b907d29e1bd163847f57ff1e2645ce7342005e Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 7 Sep 2024 17:38:50 +0200 Subject: [PATCH 04/37] test --- src/coreclr/jit/fgopt.cpp | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 932bc55dbf7c0e..c93b27a6410f4a 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -5286,7 +5286,8 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, { assert(rootNode->TypeIs(TYP_INT)); - rootNode->gtGetOp1()->gtFlags |= (GTF_RELOP_JMP_USED | GTF_DONT_CSE); + GenTree* compare = rootNode->gtGetOp1(); + compare->gtFlags |= (GTF_RELOP_JMP_USED | GTF_DONT_CSE); rootNode->ChangeOper(GT_JTRUE); DebugInfo dbgInfo = block->lastStmt()->GetDebugInfo(); @@ -5303,6 +5304,21 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, falseEdge->setLikelihood(0.5); block->SetCond(trueEdge, falseEdge); + // We expect this expansion to happen earlier + assert(!block->IsLIR()); + + // CI test: + gtUpdateStmtSideEffects(block->lastStmt()); + if (fgNodeThreading != NodeThreading::None) + { + gtSetStmtInfo(block->lastStmt()); + fgSetStmtSeq(block->lastStmt()); + gtSetStmtInfo(retTrueBb->lastStmt()); + fgSetStmtSeq(retTrueBb->lastStmt()); + gtSetStmtInfo(retFalseBb->lastStmt()); + fgSetStmtSeq(retFalseBb->lastStmt()); + } + assert(BasicBlock::sameEHRegion(block, retTrueBb)); assert(BasicBlock::sameEHRegion(block, retFalseBb)); From 15cb4c045d1b710c4334ce9ee9f0c3e455959001 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 7 Sep 2024 19:46:08 +0200 Subject: [PATCH 05/37] fix diffs --- src/coreclr/jit/fgopt.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index c93b27a6410f4a..a9df43c20160b4 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -5278,7 +5278,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, // since the latter is more friendly to various optimizations. The newly added tails are expected to be // deduplicated where needed or/and the condition will be transformed back to branch-less version where // profitable. - if (doTailDuplication && (info.compRetType == TYP_UBYTE) && block->KindIs(BBJ_RETURN) && + if (doTailDuplication && isPhase && (info.compRetType == TYP_UBYTE) && block->KindIs(BBJ_RETURN) && (block->lastStmt() != nullptr) && (block != genReturnBB) && !block->isRunRarely()) { GenTree* rootNode = block->lastStmt()->GetRootNode(); @@ -5304,6 +5304,9 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, falseEdge->setLikelihood(0.5); block->SetCond(trueEdge, falseEdge); + retTrueBb->inheritWeightPercentage(block, 50); + retFalseBb->inheritWeightPercentage(block, 50); + // We expect this expansion to happen earlier assert(!block->IsLIR()); From 09bb50d732749c9e3acfeb7f3fcbcdb3cc83265c Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 7 Sep 2024 20:10:01 +0200 Subject: [PATCH 06/37] run on isPhase --- src/coreclr/jit/fgopt.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index a9df43c20160b4..418f49d1a1c96e 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -5278,7 +5278,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, // since the latter is more friendly to various optimizations. The newly added tails are expected to be // deduplicated where needed or/and the condition will be transformed back to branch-less version where // profitable. - if (doTailDuplication && isPhase && (info.compRetType == TYP_UBYTE) && block->KindIs(BBJ_RETURN) && + if (isPhase && (info.compRetType == TYP_UBYTE) && block->KindIs(BBJ_RETURN) && (block->lastStmt() != nullptr) && (block != genReturnBB) && !block->isRunRarely()) { GenTree* rootNode = block->lastStmt()->GetRootNode(); From 9f717164b8fbceeacccae0429ca0221d265c5d36 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 7 Sep 2024 20:53:19 +0200 Subject: [PATCH 07/37] Fix some size regressions --- src/coreclr/jit/ifconversion.cpp | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/ifconversion.cpp b/src/coreclr/jit/ifconversion.cpp index 6008bb432550b6..e65b8ce3f31834 100644 --- a/src/coreclr/jit/ifconversion.cpp +++ b/src/coreclr/jit/ifconversion.cpp @@ -704,9 +704,30 @@ bool OptIfConversionDsc::optIfConvert() selectType = genActualType(m_thenOperation.node); } - // Create a select node. - GenTreeConditional* select = - m_comp->gtNewConditionalNode(GT_SELECT, m_cond, selectTrueInput, selectFalseInput, selectType); + + GenTree* select = nullptr; + if (selectTrueInput->TypeIs(TYP_INT) && selectFalseInput->TypeIs(TYP_INT)) + { + if (selectTrueInput->IsIntegralConst(1) && selectFalseInput->IsIntegralConst(0)) + { + // compare ? true : false --> compare + select = m_cond; + } + else if (selectTrueInput->IsIntegralConst(0) && selectFalseInput->IsIntegralConst(1)) + { + // compare ? false : true --> reversed_compare + select = m_cond; + select->gtOper = GenTree::ReverseRelop(select->OperGet()); + } + } + + if (select == nullptr) + { + // Create a select node + select = + m_comp->gtNewConditionalNode(GT_SELECT, m_cond, selectTrueInput, selectFalseInput, selectType); + } + m_thenOperation.node->AddAllEffectsFlags(select); // Use the select as the source of the Then operation. From 5d3dba964416067745bac32a92ca1d90ce4003a5 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 7 Sep 2024 21:52:20 +0200 Subject: [PATCH 08/37] fix bug --- src/coreclr/jit/fgdiagnostic.cpp | 5 +++++ src/coreclr/jit/fgopt.cpp | 8 +++----- src/coreclr/jit/ifconversion.cpp | 6 ++---- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index dbe1b4351b0c5f..7e384c1fb8c888 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -3275,6 +3275,11 @@ void Compiler::fgDebugCheckTypes(GenTree* tree) assert(node->TypeIs(TYP_VOID) && "GT_NOP should be TYP_VOID."); } + if (node->OperIs(GT_JTRUE)) + { + assert(node->TypeIs(TYP_VOID) && "GT_JTRUE should be TYP_VOID."); + } + if (varTypeIsSmall(node)) { if (node->OperIs(GT_COMMA)) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 418f49d1a1c96e..07ba72fb618c8f 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -5278,7 +5278,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, // since the latter is more friendly to various optimizations. The newly added tails are expected to be // deduplicated where needed or/and the condition will be transformed back to branch-less version where // profitable. - if (isPhase && (info.compRetType == TYP_UBYTE) && block->KindIs(BBJ_RETURN) && + if (doTailDuplication && (info.compRetType == TYP_UBYTE) && block->KindIs(BBJ_RETURN) && (block->lastStmt() != nullptr) && (block != genReturnBB) && !block->isRunRarely()) { GenTree* rootNode = block->lastStmt()->GetRootNode(); @@ -5289,6 +5289,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, GenTree* compare = rootNode->gtGetOp1(); compare->gtFlags |= (GTF_RELOP_JMP_USED | GTF_DONT_CSE); rootNode->ChangeOper(GT_JTRUE); + rootNode->ChangeType(TYP_VOID); DebugInfo dbgInfo = block->lastStmt()->GetDebugInfo(); BasicBlock* retTrueBb = @@ -5310,8 +5311,6 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, // We expect this expansion to happen earlier assert(!block->IsLIR()); - // CI test: - gtUpdateStmtSideEffects(block->lastStmt()); if (fgNodeThreading != NodeThreading::None) { gtSetStmtInfo(block->lastStmt()); @@ -5327,8 +5326,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, change = true; modified = true; - bDest = block->GetTrueTarget(); - bNext = block->GetFalseTarget(); + goto REPEAT; } } diff --git a/src/coreclr/jit/ifconversion.cpp b/src/coreclr/jit/ifconversion.cpp index e65b8ce3f31834..fb47ba80cd65f4 100644 --- a/src/coreclr/jit/ifconversion.cpp +++ b/src/coreclr/jit/ifconversion.cpp @@ -704,7 +704,6 @@ bool OptIfConversionDsc::optIfConvert() selectType = genActualType(m_thenOperation.node); } - GenTree* select = nullptr; if (selectTrueInput->TypeIs(TYP_INT) && selectFalseInput->TypeIs(TYP_INT)) { @@ -716,7 +715,7 @@ bool OptIfConversionDsc::optIfConvert() else if (selectTrueInput->IsIntegralConst(0) && selectFalseInput->IsIntegralConst(1)) { // compare ? false : true --> reversed_compare - select = m_cond; + select = m_cond; select->gtOper = GenTree::ReverseRelop(select->OperGet()); } } @@ -724,8 +723,7 @@ bool OptIfConversionDsc::optIfConvert() if (select == nullptr) { // Create a select node - select = - m_comp->gtNewConditionalNode(GT_SELECT, m_cond, selectTrueInput, selectFalseInput, selectType); + select = m_comp->gtNewConditionalNode(GT_SELECT, m_cond, selectTrueInput, selectFalseInput, selectType); } m_thenOperation.node->AddAllEffectsFlags(select); From 679a68bfcb44ec78aa8baeb2e7bdf34dfc64f681 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 7 Sep 2024 23:52:44 +0200 Subject: [PATCH 09/37] disable on jit32 gc encoder --- src/coreclr/jit/fgopt.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 07ba72fb618c8f..4b99ef93d49d49 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -5278,6 +5278,9 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, // since the latter is more friendly to various optimizations. The newly added tails are expected to be // deduplicated where needed or/and the condition will be transformed back to branch-less version where // profitable. + // + // NOTE: Avoid spawning more potential epilogues for JIT32_GCENCODER which has a hard limit for them. +#ifndef JIT32_GCENCODER if (doTailDuplication && (info.compRetType == TYP_UBYTE) && block->KindIs(BBJ_RETURN) && (block->lastStmt() != nullptr) && (block != genReturnBB) && !block->isRunRarely()) { @@ -5329,6 +5332,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, goto REPEAT; } } +#endif if (block->KindIs(BBJ_ALWAYS)) { From a7240d925d8dace87f34d09f0f658146c37aa02c Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 8 Sep 2024 01:41:07 +0200 Subject: [PATCH 10/37] mitigate some regressions --- src/coreclr/jit/fgopt.cpp | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 4b99ef93d49d49..c123dde49b4df5 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -5285,7 +5285,9 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, (block->lastStmt() != nullptr) && (block != genReturnBB) && !block->isRunRarely()) { GenTree* rootNode = block->lastStmt()->GetRootNode(); - if (rootNode->OperIs(GT_RETURN) && rootNode->gtGetOp1()->OperIsCmpCompare()) + if (rootNode->OperIs(GT_RETURN) && rootNode->gtGetOp1()->OperIsCmpCompare() && + // The following check is purely a TP-oriented heuristics: + rootNode->gtGetOp1()->gtGetOp2()->IsCnsIntOrI()) { assert(rootNode->TypeIs(TYP_INT)); @@ -5294,20 +5296,21 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, rootNode->ChangeOper(GT_JTRUE); rootNode->ChangeType(TYP_VOID); - DebugInfo dbgInfo = block->lastStmt()->GetDebugInfo(); - BasicBlock* retTrueBb = - fgNewBBFromTreeAfter(BBJ_RETURN, block, gtNewOperNode(GT_RETURN, TYP_INT, gtNewTrue()), - dbgInfo); - BasicBlock* retFalseBb = - fgNewBBFromTreeAfter(BBJ_RETURN, block, gtNewOperNode(GT_RETURN, TYP_INT, gtNewFalse()), - dbgInfo); + GenTree* retTrue = gtNewOperNode(GT_RETURN, TYP_INT, gtNewTrue()); + GenTree* retFalse = gtNewOperNode(GT_RETURN, TYP_INT, gtNewFalse()); + + // Create RETURN 1/0 blocks. We expect fgHeadTailMerge to handle them if there are similar returns. + DebugInfo dbgInfo = block->lastStmt()->GetDebugInfo(); + BasicBlock* retTrueBb = fgNewBBFromTreeAfter(BBJ_RETURN, block, retTrue, dbgInfo); + BasicBlock* retFalseBb = fgNewBBFromTreeAfter(BBJ_RETURN, block, retFalse, dbgInfo); FlowEdge* trueEdge = fgAddRefPred(retTrueBb, block); FlowEdge* falseEdge = fgAddRefPred(retFalseBb, block); - trueEdge->setLikelihood(0.5); - falseEdge->setLikelihood(0.5); block->SetCond(trueEdge, falseEdge); + // We might want to instrument 'return ' too in the future. For now apply 50%/50%. + trueEdge->setLikelihood(0.5); + falseEdge->setLikelihood(0.5); retTrueBb->inheritWeightPercentage(block, 50); retFalseBb->inheritWeightPercentage(block, 50); @@ -5324,12 +5327,9 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, fgSetStmtSeq(retFalseBb->lastStmt()); } - assert(BasicBlock::sameEHRegion(block, retTrueBb)); - assert(BasicBlock::sameEHRegion(block, retFalseBb)); - change = true; modified = true; - goto REPEAT; + bNext = block->Next(); } } #endif @@ -6383,6 +6383,11 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) madeChanges |= fgHeadMerge(block, early); } + if (madeChanges && !early) + { + fgUpdateFlowGraph(); + } + // If we altered flow, reset fgModified. Given where we sit in the // phase list, flow-dependent side data hasn't been built yet, so // nothing needs invalidation. From 69005b786e3dbfbad6d2a631eda531d3e2940882 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 8 Sep 2024 01:59:32 +0200 Subject: [PATCH 11/37] clean up --- src/coreclr/jit/fgopt.cpp | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index c123dde49b4df5..647535a80cf448 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -5314,19 +5314,6 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, retTrueBb->inheritWeightPercentage(block, 50); retFalseBb->inheritWeightPercentage(block, 50); - // We expect this expansion to happen earlier - assert(!block->IsLIR()); - - if (fgNodeThreading != NodeThreading::None) - { - gtSetStmtInfo(block->lastStmt()); - fgSetStmtSeq(block->lastStmt()); - gtSetStmtInfo(retTrueBb->lastStmt()); - fgSetStmtSeq(retTrueBb->lastStmt()); - gtSetStmtInfo(retFalseBb->lastStmt()); - fgSetStmtSeq(retFalseBb->lastStmt()); - } - change = true; modified = true; bNext = block->Next(); From 152ad31f09431516ef79b960543177b2d3444fe6 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 8 Sep 2024 11:07:05 +0200 Subject: [PATCH 12/37] fix size regressions --- src/coreclr/jit/fgopt.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 647535a80cf448..1d01f282648514 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -5286,8 +5286,9 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, { GenTree* rootNode = block->lastStmt()->GetRootNode(); if (rootNode->OperIs(GT_RETURN) && rootNode->gtGetOp1()->OperIsCmpCompare() && - // The following check is purely a TP-oriented heuristics: - rootNode->gtGetOp1()->gtGetOp2()->IsCnsIntOrI()) + // The following check is purely to improve TP and handle some size regressions we fail to handle + // Eventually, we should remove it. + rootNode->gtGetOp1()->gtGetOp1()->OperIsLeaf() && rootNode->gtGetOp1()->gtGetOp2()->OperIsLeaf()) { assert(rootNode->TypeIs(TYP_INT)); @@ -6372,6 +6373,7 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) if (madeChanges && !early) { + // Clean up potential unconditional jumps produced by tail merging fgUpdateFlowGraph(); } From 4775b50e2b82697bb23e6bccb26b93b05e9091fb Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 8 Sep 2024 18:06:44 +0200 Subject: [PATCH 13/37] move to a separate method --- src/coreclr/jit/compiler.h | 2 + src/coreclr/jit/fgopt.cpp | 120 +++++++++++++++++++++++-------------- 2 files changed, 76 insertions(+), 46 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 6fe0555691bd8f..270a4afce6d49d 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6263,6 +6263,8 @@ class Compiler PhaseStatus fgDetermineFirstColdBlock(); + bool fgDedupReturnComparison(BasicBlock* block); + bool fgIsForwardBranch(BasicBlock* bJump, BasicBlock* bDest, BasicBlock* bSrc = nullptr); bool fgUpdateFlowGraph(bool doTailDup = false, bool isPhase = false, bool doAggressiveCompaction = true); diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 1d01f282648514..d47125d07c8a2f 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -5178,6 +5178,75 @@ PhaseStatus Compiler::fgUpdateFlowGraphPhase() return madeChanges ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; } +//------------------------------------------------------------- +// fgDedupReturnComparison: Expands BBJ_RETURN CMP into BBJ_COND CMP with two +// BBJ_RETURN blocks ("return true" and "return false"). Such transformation +// helps other phases to focus only on BBJ_COND CMP. +// +// Arguments: +// block - the BBJ_RETURN block to convert into BBJ_COND CMP +// +// Returns: +// true if the block was converted into BBJ_COND CMP +// +bool Compiler::fgDedupReturnComparison(BasicBlock* block) +{ +#ifdef JIT32_GCENCODER + // JIT32_GCENCODER has a hard limit on the number of epilogues, let's not add more. + return false; +#endif + + assert(block->KindIs(BBJ_RETURN)); + + // We're only interested in boolean returns + if ((info.compRetType != TYP_UBYTE) || (block == genReturnBB) || (block->lastStmt() == nullptr)) + { + return false; + } + + GenTree* rootNode = block->lastStmt()->GetRootNode(); + if (!rootNode->OperIs(GT_RETURN) || !rootNode->gtGetOp1()->OperIsCmpCompare()) + { + return false; + } + + GenTree* cmp = rootNode->gtGetOp1(); + GenTree* cmpOp1 = cmp->gtGetOp1(); + GenTree* cmpOp2 = cmp->gtGetOp2(); + + // The following check is purely to improve TP and handle some size regressions we fail to handle + // Eventually, we should remove it. + bool profitable = (cmpOp1->IsLocal() && cmpOp2->IsCnsIntOrI()) || (cmpOp2->IsLocal() && cmpOp1->IsCnsIntOrI()); + if (!profitable) + { + return false; + } + + cmp->gtFlags |= (GTF_RELOP_JMP_USED | GTF_DONT_CSE); + rootNode->ChangeOper(GT_JTRUE); + rootNode->ChangeType(TYP_VOID); + + GenTree* retTrue = gtNewOperNode(GT_RETURN, TYP_INT, gtNewTrue()); + GenTree* retFalse = gtNewOperNode(GT_RETURN, TYP_INT, gtNewFalse()); + + // Create RETURN 1/0 blocks. We expect fgHeadTailMerge to handle them if there are similar returns. + DebugInfo dbgInfo = block->lastStmt()->GetDebugInfo(); + BasicBlock* retTrueBb = fgNewBBFromTreeAfter(BBJ_RETURN, block, retTrue, dbgInfo); + BasicBlock* retFalseBb = fgNewBBFromTreeAfter(BBJ_RETURN, block, retFalse, dbgInfo); + + FlowEdge* trueEdge = fgAddRefPred(retTrueBb, block); + FlowEdge* falseEdge = fgAddRefPred(retFalseBb, block); + block->SetCond(trueEdge, falseEdge); + + // We might want to instrument 'return ' too in the future. For now apply 50%/50%. + trueEdge->setLikelihood(0.5); + falseEdge->setLikelihood(0.5); + retTrueBb->inheritWeightPercentage(block, 50); + retFalseBb->inheritWeightPercentage(block, 50); + + return true; +} + //------------------------------------------------------------- // fgUpdateFlowGraph: Removes any empty blocks, unreachable blocks, and redundant jumps. // Most of those appear after dead store removal and folding of conditionals. @@ -5274,53 +5343,12 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bDest = nullptr; bFalseDest = nullptr; - // Expand "return " into "return ? true : false" - // since the latter is more friendly to various optimizations. The newly added tails are expected to be - // deduplicated where needed or/and the condition will be transformed back to branch-less version where - // profitable. - // - // NOTE: Avoid spawning more potential epilogues for JIT32_GCENCODER which has a hard limit for them. -#ifndef JIT32_GCENCODER - if (doTailDuplication && (info.compRetType == TYP_UBYTE) && block->KindIs(BBJ_RETURN) && - (block->lastStmt() != nullptr) && (block != genReturnBB) && !block->isRunRarely()) - { - GenTree* rootNode = block->lastStmt()->GetRootNode(); - if (rootNode->OperIs(GT_RETURN) && rootNode->gtGetOp1()->OperIsCmpCompare() && - // The following check is purely to improve TP and handle some size regressions we fail to handle - // Eventually, we should remove it. - rootNode->gtGetOp1()->gtGetOp1()->OperIsLeaf() && rootNode->gtGetOp1()->gtGetOp2()->OperIsLeaf()) - { - assert(rootNode->TypeIs(TYP_INT)); - - GenTree* compare = rootNode->gtGetOp1(); - compare->gtFlags |= (GTF_RELOP_JMP_USED | GTF_DONT_CSE); - rootNode->ChangeOper(GT_JTRUE); - rootNode->ChangeType(TYP_VOID); - - GenTree* retTrue = gtNewOperNode(GT_RETURN, TYP_INT, gtNewTrue()); - GenTree* retFalse = gtNewOperNode(GT_RETURN, TYP_INT, gtNewFalse()); - - // Create RETURN 1/0 blocks. We expect fgHeadTailMerge to handle them if there are similar returns. - DebugInfo dbgInfo = block->lastStmt()->GetDebugInfo(); - BasicBlock* retTrueBb = fgNewBBFromTreeAfter(BBJ_RETURN, block, retTrue, dbgInfo); - BasicBlock* retFalseBb = fgNewBBFromTreeAfter(BBJ_RETURN, block, retFalse, dbgInfo); - - FlowEdge* trueEdge = fgAddRefPred(retTrueBb, block); - FlowEdge* falseEdge = fgAddRefPred(retFalseBb, block); - block->SetCond(trueEdge, falseEdge); - - // We might want to instrument 'return ' too in the future. For now apply 50%/50%. - trueEdge->setLikelihood(0.5); - falseEdge->setLikelihood(0.5); - retTrueBb->inheritWeightPercentage(block, 50); - retFalseBb->inheritWeightPercentage(block, 50); - - change = true; - modified = true; - bNext = block->Next(); - } + if (doTailDuplication && block->KindIs(BBJ_RETURN) && fgDedupReturnComparison(block)) + { + change = true; + modified = true; + bNext = block->Next(); } -#endif if (block->KindIs(BBJ_ALWAYS)) { From 7334bd899ff4662297db767c173e62c6d1c1616d Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 8 Sep 2024 18:13:56 +0200 Subject: [PATCH 14/37] fix reverse --- src/coreclr/jit/ifconversion.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/jit/ifconversion.cpp b/src/coreclr/jit/ifconversion.cpp index fb47ba80cd65f4..178a4c3f670cbe 100644 --- a/src/coreclr/jit/ifconversion.cpp +++ b/src/coreclr/jit/ifconversion.cpp @@ -715,8 +715,7 @@ bool OptIfConversionDsc::optIfConvert() else if (selectTrueInput->IsIntegralConst(0) && selectFalseInput->IsIntegralConst(1)) { // compare ? false : true --> reversed_compare - select = m_cond; - select->gtOper = GenTree::ReverseRelop(select->OperGet()); + select = m_comp->gtReverseCond(m_cond); } } From 34c687e6c614237299e2a657cd09a0f141ea8949 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Wed, 11 Sep 2024 01:13:24 +0200 Subject: [PATCH 15/37] Update fgopt.cpp --- src/coreclr/jit/fgopt.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index d47125d07c8a2f..a3ecf42018702f 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -5343,12 +5343,12 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bDest = nullptr; bFalseDest = nullptr; - if (doTailDuplication && block->KindIs(BBJ_RETURN) && fgDedupReturnComparison(block)) + /*if (doTailDuplication && block->KindIs(BBJ_RETURN) && fgDedupReturnComparison(block)) { change = true; modified = true; bNext = block->Next(); - } + }*/ if (block->KindIs(BBJ_ALWAYS)) { From 8a434f535494b7a69be9b788503b302a6b6197e7 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Wed, 11 Sep 2024 02:22:36 +0200 Subject: [PATCH 16/37] Update fgopt.cpp --- src/coreclr/jit/fgopt.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index a3ecf42018702f..644b5631bb3af0 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -6402,7 +6402,7 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) if (madeChanges && !early) { // Clean up potential unconditional jumps produced by tail merging - fgUpdateFlowGraph(); + //fgUpdateFlowGraph(); } // If we altered flow, reset fgModified. Given where we sit in the From 1a319a9f1e5bdd52a6b63b538a467e9f6ebbb0eb Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Wed, 11 Sep 2024 03:51:31 +0200 Subject: [PATCH 17/37] Update fgopt.cpp --- src/coreclr/jit/fgopt.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 644b5631bb3af0..5289b5d9384147 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -5343,12 +5343,12 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bDest = nullptr; bFalseDest = nullptr; - /*if (doTailDuplication && block->KindIs(BBJ_RETURN) && fgDedupReturnComparison(block)) + if (doTailDuplication && block->KindIs(BBJ_RETURN) && fgDedupReturnComparison(block)) { change = true; modified = true; bNext = block->Next(); - }*/ + } if (block->KindIs(BBJ_ALWAYS)) { From 45b145735d8281890df994d51ca3f6efce890825 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Thu, 10 Oct 2024 16:16:58 +0200 Subject: [PATCH 18/37] Update fgopt.cpp --- src/coreclr/jit/fgopt.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 373c589e24d765..f78a71376b7304 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -5047,11 +5047,11 @@ bool Compiler::fgDedupReturnComparison(BasicBlock* block) // The following check is purely to improve TP and handle some size regressions we fail to handle // Eventually, we should remove it. - bool profitable = (cmpOp1->IsLocal() && cmpOp2->IsCnsIntOrI()) || (cmpOp2->IsLocal() && cmpOp1->IsCnsIntOrI()); + /*bool profitable = (cmpOp1->IsLocal() && cmpOp2->IsCnsIntOrI()) || (cmpOp2->IsLocal() && cmpOp1->IsCnsIntOrI()); if (!profitable) { return false; - } + }*/ cmp->gtFlags |= (GTF_RELOP_JMP_USED | GTF_DONT_CSE); rootNode->ChangeOper(GT_JTRUE); From 042e53cb4b7b50d93b37edf7f3ce8672034e833c Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 31 Jan 2025 03:00:35 +0100 Subject: [PATCH 19/37] clean up --- src/coreclr/jit/fgopt.cpp | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 0351798ba7b055..e7c8014c8684c2 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -5739,13 +5739,6 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bool isPh bDest = nullptr; bFalseDest = nullptr; - if (doTailDuplication && block->KindIs(BBJ_RETURN) && fgDedupReturnComparison(block)) - { - change = true; - modified = true; - bNext = block->Next(); - } - if (block->KindIs(BBJ_ALWAYS)) { bDest = block->GetTarget(); From 2977ea53a7e5938a734a55cf33e0d6ac3916045f Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 31 Jan 2025 03:20:52 +0100 Subject: [PATCH 20/37] clean up --- src/coreclr/jit/fgopt.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index e7c8014c8684c2..0351798ba7b055 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -5739,6 +5739,13 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bool isPh bDest = nullptr; bFalseDest = nullptr; + if (doTailDuplication && block->KindIs(BBJ_RETURN) && fgDedupReturnComparison(block)) + { + change = true; + modified = true; + bNext = block->Next(); + } + if (block->KindIs(BBJ_ALWAYS)) { bDest = block->GetTarget(); From 4039806d9f03156db23b1065934f0b02d0ec7078 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 31 Jan 2025 04:57:09 +0100 Subject: [PATCH 21/37] merge returns back together --- src/coreclr/jit/optimizebools.cpp | 75 ++++++++++++++++++++++++++++++- 1 file changed, 74 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 4a6057e122d014..93c58b9b30fc9a 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -1922,6 +1922,79 @@ PhaseStatus Compiler::optOptimizeBools() unsigned numPasses = 0; unsigned stress = false; + bool modified = false; + if ((info.compRetType == TYP_UBYTE) && (genReturnBB == nullptr)) + { + for (BasicBlock* block = fgFirstBB; block != nullptr; block = block->Next()) + { +#ifdef JIT32_GCENCODER + // JIT32_GCENCODER has a hard limit on the number of epilogues, let's not add more. + break; +#endif + if (!block->KindIs(BBJ_COND)) + { + continue; + } + + BasicBlock* retTrueBb = block->GetTrueEdge()->getDestinationBlock(); + BasicBlock* retFalseBb = block->GetFalseEdge()->getDestinationBlock(); + if (!retTrueBb->KindIs(BBJ_RETURN) || !retFalseBb->KindIs(BBJ_RETURN)) + { + continue; + } + + if ((retTrueBb->GetUniquePred(this) == nullptr) && (retFalseBb->GetUniquePred(this) == nullptr)) + { + // Both return blocks have multiple predecessors - bail out. + // We don't want to introduce a new epilogue. + continue; + } + + auto isReturnBool = [](const BasicBlock* block, bool value) { + if (block->KindIs(BBJ_RETURN) && block->hasSingleStmt() && (block->lastStmt() != nullptr)) + { + GenTree* node = block->lastStmt()->GetRootNode(); + return node->OperIs(GT_RETURN) && node->gtGetOp1()->IsIntegralConst(value ? 1 : 0); + } + return false; + }; + + bool retTrueFalse = isReturnBool(retTrueBb, true) && isReturnBool(retFalseBb, false); + bool retFalseTrue = isReturnBool(retTrueBb, false) && isReturnBool(retFalseBb, true); + if (!retTrueFalse && !retFalseTrue) + { + continue; + } + + GenTree* node = block->lastStmt()->GetRootNode(); + if (!node->gtGetOp1()->OperIsCompare()) + { + continue; + } + assert(node->gtGetOp1()->TypeIs(TYP_INT)); + assert(BasicBlock::sameEHRegion(block, retTrueBb)); + assert(BasicBlock::sameEHRegion(block, retFalseBb)); + fgRemoveRefPred(block->GetTrueEdge()); + fgRemoveRefPred(block->GetFalseEdge()); + block->SetKindAndTargetEdge(BBJ_RETURN); + if (retFalseTrue) + { + node->gtGetOp1()->AsOp()->SetOper(GenTree::ReverseRelop(node->gtGetOp1()->OperGet())); + } + assert(node->OperIs(GT_JTRUE)); + node->ChangeOper(GT_RETURN); + node->ChangeType(TYP_INT); + node->gtGetOp1()->gtFlags &= ~GTF_RELOP_JMP_USED; + fgPgoConsistent = false; + fgInvalidateDfsTree(); + modified = true; + gtSetStmtInfo(block->lastStmt()); + fgSetStmtSeq(block->lastStmt()); + gtUpdateStmtSideEffects(block->lastStmt()); + block->bbCodeOffsEnd = max(retTrueBb->bbCodeOffsEnd, retFalseBb->bbCodeOffsEnd); + } + } + do { numPasses++; @@ -2024,6 +2097,6 @@ PhaseStatus Compiler::optOptimizeBools() JITDUMP("\noptimized %u BBJ_COND cases, %u BBJ_RETURN cases in %u passes\n", numCond, numReturn, numPasses); - const bool modified = stress || ((numCond + numReturn) > 0); + modified |= stress || ((numCond + numReturn) > 0); return modified ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; } From eadf625c1d1b5694ed9359eff2d32884015a719e Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 31 Jan 2025 05:38:04 +0100 Subject: [PATCH 22/37] clean up --- src/coreclr/jit/compiler.h | 2 + src/coreclr/jit/optimizebools.cpp | 159 ++++++++++++++++-------------- 2 files changed, 88 insertions(+), 73 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 7908e72e22b713..89770b64d428ad 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5492,6 +5492,8 @@ class Compiler FoldResult fgFoldConditional(BasicBlock* block); + bool fgFoldCondToReturnBlock(BasicBlock* block); + struct MorphUnreachableInfo { MorphUnreachableInfo(Compiler* comp); diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 93c58b9b30fc9a..51362c79a2783d 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -1776,6 +1776,87 @@ GenTree* OptBoolsDsc::optIsBoolComp(OptTestInfo* pOptTest) return opr1; } +bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) +{ + assert(block->KindIs(BBJ_COND)); + +#ifdef JIT32_GCENCODER + // JIT32_GCENCODER has a hard limit on the number of epilogues, let's not add more. + return false; +#endif + + // Early out if the current method is not returning a boolean. + if ((info.compRetType != TYP_UBYTE) || (genReturnBB != nullptr)) + { + return false; + } + + BasicBlock* retTrueBb = block->GetTrueEdge()->getDestinationBlock(); + BasicBlock* retFalseBb = block->GetFalseEdge()->getDestinationBlock(); + if (!retTrueBb->KindIs(BBJ_RETURN) || !retFalseBb->KindIs(BBJ_RETURN)) + { + return false; + } + + if ((retTrueBb->GetUniquePred(this) == nullptr) && (retFalseBb->GetUniquePred(this) == nullptr)) + { + // Both return blocks have multiple predecessors - bail out. + // We don't want to introduce a new epilogue. + return false; + } + + // Is block a BBJ_RETURN(1/0) ? (single statement) + auto isReturnBool = [](const BasicBlock* block, bool value) { + if (block->KindIs(BBJ_RETURN) && block->hasSingleStmt() && (block->lastStmt() != nullptr)) + { + GenTree* node = block->lastStmt()->GetRootNode(); + return node->OperIs(GT_RETURN) && node->gtGetOp1()->IsIntegralConst(value ? 1 : 0); + } + return false; + }; + + // Make sure we deal with true/false return blocks (or false/true) + bool retTrueFalse = isReturnBool(retTrueBb, true) && isReturnBool(retFalseBb, false); + bool retFalseTrue = isReturnBool(retTrueBb, false) && isReturnBool(retFalseBb, true); + if (!retTrueFalse && !retFalseTrue) + { + return false; + } + + GenTree* node = block->lastStmt()->GetRootNode(); + assert(node->OperIs(GT_JTRUE)); + + GenTree* cond = node->gtGetOp1(); + if (!cond->OperIsCompare()) + { + return false; + } + + // Reverse the condition if we jump to "return false" on true. + if (retFalseTrue) + { + cond->SetOper(GenTree::ReverseRelop(cond->OperGet())); + } + + assert(cond->TypeIs(TYP_INT)); + assert(BasicBlock::sameEHRegion(block, retTrueBb)); + assert(BasicBlock::sameEHRegion(block, retFalseBb)); + + // Unlink the return blocks, someone will pick them up later. + fgRemoveRefPred(block->GetTrueEdge()); + fgRemoveRefPred(block->GetFalseEdge()); + block->SetKindAndTargetEdge(BBJ_RETURN); + node->ChangeOper(GT_RETURN); + node->ChangeType(TYP_INT); + + fgPgoConsistent = false; + block->bbCodeOffsEnd = max(retTrueBb->bbCodeOffsEnd, retFalseBb->bbCodeOffsEnd); + gtSetStmtInfo(block->lastStmt()); + fgSetStmtSeq(block->lastStmt()); + gtUpdateStmtSideEffects(block->lastStmt()); + return true; +} + //----------------------------------------------------------------------------- // optOptimizeBools: Folds boolean conditionals for GT_JTRUE/GT_RETURN/GT_SWIFT_ERROR_RET nodes // @@ -1921,79 +2002,7 @@ PhaseStatus Compiler::optOptimizeBools() unsigned numReturn = 0; unsigned numPasses = 0; unsigned stress = false; - - bool modified = false; - if ((info.compRetType == TYP_UBYTE) && (genReturnBB == nullptr)) - { - for (BasicBlock* block = fgFirstBB; block != nullptr; block = block->Next()) - { -#ifdef JIT32_GCENCODER - // JIT32_GCENCODER has a hard limit on the number of epilogues, let's not add more. - break; -#endif - if (!block->KindIs(BBJ_COND)) - { - continue; - } - - BasicBlock* retTrueBb = block->GetTrueEdge()->getDestinationBlock(); - BasicBlock* retFalseBb = block->GetFalseEdge()->getDestinationBlock(); - if (!retTrueBb->KindIs(BBJ_RETURN) || !retFalseBb->KindIs(BBJ_RETURN)) - { - continue; - } - - if ((retTrueBb->GetUniquePred(this) == nullptr) && (retFalseBb->GetUniquePred(this) == nullptr)) - { - // Both return blocks have multiple predecessors - bail out. - // We don't want to introduce a new epilogue. - continue; - } - - auto isReturnBool = [](const BasicBlock* block, bool value) { - if (block->KindIs(BBJ_RETURN) && block->hasSingleStmt() && (block->lastStmt() != nullptr)) - { - GenTree* node = block->lastStmt()->GetRootNode(); - return node->OperIs(GT_RETURN) && node->gtGetOp1()->IsIntegralConst(value ? 1 : 0); - } - return false; - }; - - bool retTrueFalse = isReturnBool(retTrueBb, true) && isReturnBool(retFalseBb, false); - bool retFalseTrue = isReturnBool(retTrueBb, false) && isReturnBool(retFalseBb, true); - if (!retTrueFalse && !retFalseTrue) - { - continue; - } - - GenTree* node = block->lastStmt()->GetRootNode(); - if (!node->gtGetOp1()->OperIsCompare()) - { - continue; - } - assert(node->gtGetOp1()->TypeIs(TYP_INT)); - assert(BasicBlock::sameEHRegion(block, retTrueBb)); - assert(BasicBlock::sameEHRegion(block, retFalseBb)); - fgRemoveRefPred(block->GetTrueEdge()); - fgRemoveRefPred(block->GetFalseEdge()); - block->SetKindAndTargetEdge(BBJ_RETURN); - if (retFalseTrue) - { - node->gtGetOp1()->AsOp()->SetOper(GenTree::ReverseRelop(node->gtGetOp1()->OperGet())); - } - assert(node->OperIs(GT_JTRUE)); - node->ChangeOper(GT_RETURN); - node->ChangeType(TYP_INT); - node->gtGetOp1()->gtFlags &= ~GTF_RELOP_JMP_USED; - fgPgoConsistent = false; - fgInvalidateDfsTree(); - modified = true; - gtSetStmtInfo(block->lastStmt()); - fgSetStmtSeq(block->lastStmt()); - gtUpdateStmtSideEffects(block->lastStmt()); - block->bbCodeOffsEnd = max(retTrueBb->bbCodeOffsEnd, retFalseBb->bbCodeOffsEnd); - } - } + bool modified = false; do { @@ -2003,6 +2012,10 @@ PhaseStatus Compiler::optOptimizeBools() for (BasicBlock* b1 = fgFirstBB; b1 != nullptr; b1 = retry ? b1 : b1->Next()) { retry = false; + if (b1->KindIs(BBJ_COND)) + { + modified |= fgFoldCondToReturnBlock(b1); + } // We're only interested in conditional jumps here From 915394024995c2aed75b14e45727eb69bb9c81b8 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Fri, 31 Jan 2025 14:00:52 +0100 Subject: [PATCH 23/37] Comment out tail duplication condition --- src/coreclr/jit/fgopt.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 0351798ba7b055..e62c0781c90bb4 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -5739,12 +5739,12 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bool isPh bDest = nullptr; bFalseDest = nullptr; - if (doTailDuplication && block->KindIs(BBJ_RETURN) && fgDedupReturnComparison(block)) + /*if (doTailDuplication && block->KindIs(BBJ_RETURN) && fgDedupReturnComparison(block)) { change = true; modified = true; bNext = block->Next(); - } + }*/ if (block->KindIs(BBJ_ALWAYS)) { From 3628d3249c5576e4c32a88b32858b0d691196632 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Thu, 20 Mar 2025 21:51:05 +0100 Subject: [PATCH 24/37] Enable tail duplication for return blocks --- src/coreclr/jit/fgopt.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index bd19532ea7d494..52ece4ca380c58 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4423,12 +4423,12 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bool isPh bDest = nullptr; bFalseDest = nullptr; - /*if (doTailDuplication && block->KindIs(BBJ_RETURN) && fgDedupReturnComparison(block)) + if (doTailDuplication && block->KindIs(BBJ_RETURN) && fgDedupReturnComparison(block)) { change = true; modified = true; bNext = block->Next(); - }*/ + } if (block->KindIs(BBJ_ALWAYS)) { From f5535ac98522c35b58d2256606f08f5e0cd1f84a Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 23 Mar 2025 02:12:05 +0100 Subject: [PATCH 25/37] Cleanup --- src/coreclr/jit/compiler.h | 2 - src/coreclr/jit/fgdiagnostic.cpp | 18 ++++--- src/coreclr/jit/fgopt.cpp | 25 +++------ src/coreclr/jit/optimizebools.cpp | 88 +------------------------------ 4 files changed, 21 insertions(+), 112 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 859e4f9000eb31..48b7fc24a6723b 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5384,8 +5384,6 @@ class Compiler FoldResult fgFoldConditional(BasicBlock* block); - bool fgFoldCondToReturnBlock(BasicBlock* block); - struct MorphUnreachableInfo { MorphUnreachableInfo(Compiler* comp); diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index be239d85c17bcd..422b6fe6ef1731 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -3278,14 +3278,20 @@ void Compiler::fgDebugCheckTypes(GenTree* tree) assert(!"TYP_ULONG and TYP_UINT are not legal in IR"); } - if (node->OperIs(GT_NOP)) + switch (node->OperGet()) { - assert(node->TypeIs(TYP_VOID) && "GT_NOP should be TYP_VOID."); - } + case GT_NOP: + case GT_JTRUE: + case GT_BOUNDS_CHECK: + if (!node->TypeIs(TYP_VOID)) + { + printf("The tree is expected to be TYP_VOID:\n"); + m_compiler->gtDispTree(node); + } + break; - if (node->OperIs(GT_JTRUE)) - { - assert(node->TypeIs(TYP_VOID) && "GT_JTRUE should be TYP_VOID."); + default: + break; } if (varTypeIsSmall(node)) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 52ece4ca380c58..6e0f78013f32a0 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4260,15 +4260,15 @@ PhaseStatus Compiler::fgUpdateFlowGraphPhase() } //------------------------------------------------------------- -// fgDedupReturnComparison: Expands BBJ_RETURN CMP into BBJ_COND CMP with two +// fgDedupReturnComparison: Expands BBJ_RETURN into BBJ_COND with two // BBJ_RETURN blocks ("return true" and "return false"). Such transformation -// helps other phases to focus only on BBJ_COND CMP. +// helps other phases to focus only on BBJ_COND (normalization). // // Arguments: -// block - the BBJ_RETURN block to convert into BBJ_COND CMP +// block - the BBJ_RETURN block to convert into BBJ_COND // // Returns: -// true if the block was converted into BBJ_COND CMP +// true if the block was converted into BBJ_COND // bool Compiler::fgDedupReturnComparison(BasicBlock* block) { @@ -4291,18 +4291,7 @@ bool Compiler::fgDedupReturnComparison(BasicBlock* block) return false; } - GenTree* cmp = rootNode->gtGetOp1(); - GenTree* cmpOp1 = cmp->gtGetOp1(); - GenTree* cmpOp2 = cmp->gtGetOp2(); - - // The following check is purely to improve TP and handle some size regressions we fail to handle - // Eventually, we should remove it. - /*bool profitable = (cmpOp1->IsLocal() && cmpOp2->IsCnsIntOrI()) || (cmpOp2->IsLocal() && cmpOp1->IsCnsIntOrI()); - if (!profitable) - { - return false; - }*/ - + GenTree* cmp = rootNode->gtGetOp1(); cmp->gtFlags |= (GTF_RELOP_JMP_USED | GTF_DONT_CSE); rootNode->ChangeOper(GT_JTRUE); rootNode->ChangeType(TYP_VOID); @@ -4423,13 +4412,15 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bool isPh bDest = nullptr; bFalseDest = nullptr; + // Expand BBJ_RETURN into BBJ_COND when doTailDuplication is enabled if (doTailDuplication && block->KindIs(BBJ_RETURN) && fgDedupReturnComparison(block)) { + assert(block->KindIs(BBJ_COND)); change = true; modified = true; bNext = block->Next(); } - + // Remove empty blocks if (block->KindIs(BBJ_ALWAYS)) { bDest = block->GetTarget(); diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 51362c79a2783d..4a6057e122d014 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -1776,87 +1776,6 @@ GenTree* OptBoolsDsc::optIsBoolComp(OptTestInfo* pOptTest) return opr1; } -bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) -{ - assert(block->KindIs(BBJ_COND)); - -#ifdef JIT32_GCENCODER - // JIT32_GCENCODER has a hard limit on the number of epilogues, let's not add more. - return false; -#endif - - // Early out if the current method is not returning a boolean. - if ((info.compRetType != TYP_UBYTE) || (genReturnBB != nullptr)) - { - return false; - } - - BasicBlock* retTrueBb = block->GetTrueEdge()->getDestinationBlock(); - BasicBlock* retFalseBb = block->GetFalseEdge()->getDestinationBlock(); - if (!retTrueBb->KindIs(BBJ_RETURN) || !retFalseBb->KindIs(BBJ_RETURN)) - { - return false; - } - - if ((retTrueBb->GetUniquePred(this) == nullptr) && (retFalseBb->GetUniquePred(this) == nullptr)) - { - // Both return blocks have multiple predecessors - bail out. - // We don't want to introduce a new epilogue. - return false; - } - - // Is block a BBJ_RETURN(1/0) ? (single statement) - auto isReturnBool = [](const BasicBlock* block, bool value) { - if (block->KindIs(BBJ_RETURN) && block->hasSingleStmt() && (block->lastStmt() != nullptr)) - { - GenTree* node = block->lastStmt()->GetRootNode(); - return node->OperIs(GT_RETURN) && node->gtGetOp1()->IsIntegralConst(value ? 1 : 0); - } - return false; - }; - - // Make sure we deal with true/false return blocks (or false/true) - bool retTrueFalse = isReturnBool(retTrueBb, true) && isReturnBool(retFalseBb, false); - bool retFalseTrue = isReturnBool(retTrueBb, false) && isReturnBool(retFalseBb, true); - if (!retTrueFalse && !retFalseTrue) - { - return false; - } - - GenTree* node = block->lastStmt()->GetRootNode(); - assert(node->OperIs(GT_JTRUE)); - - GenTree* cond = node->gtGetOp1(); - if (!cond->OperIsCompare()) - { - return false; - } - - // Reverse the condition if we jump to "return false" on true. - if (retFalseTrue) - { - cond->SetOper(GenTree::ReverseRelop(cond->OperGet())); - } - - assert(cond->TypeIs(TYP_INT)); - assert(BasicBlock::sameEHRegion(block, retTrueBb)); - assert(BasicBlock::sameEHRegion(block, retFalseBb)); - - // Unlink the return blocks, someone will pick them up later. - fgRemoveRefPred(block->GetTrueEdge()); - fgRemoveRefPred(block->GetFalseEdge()); - block->SetKindAndTargetEdge(BBJ_RETURN); - node->ChangeOper(GT_RETURN); - node->ChangeType(TYP_INT); - - fgPgoConsistent = false; - block->bbCodeOffsEnd = max(retTrueBb->bbCodeOffsEnd, retFalseBb->bbCodeOffsEnd); - gtSetStmtInfo(block->lastStmt()); - fgSetStmtSeq(block->lastStmt()); - gtUpdateStmtSideEffects(block->lastStmt()); - return true; -} - //----------------------------------------------------------------------------- // optOptimizeBools: Folds boolean conditionals for GT_JTRUE/GT_RETURN/GT_SWIFT_ERROR_RET nodes // @@ -2002,7 +1921,6 @@ PhaseStatus Compiler::optOptimizeBools() unsigned numReturn = 0; unsigned numPasses = 0; unsigned stress = false; - bool modified = false; do { @@ -2012,10 +1930,6 @@ PhaseStatus Compiler::optOptimizeBools() for (BasicBlock* b1 = fgFirstBB; b1 != nullptr; b1 = retry ? b1 : b1->Next()) { retry = false; - if (b1->KindIs(BBJ_COND)) - { - modified |= fgFoldCondToReturnBlock(b1); - } // We're only interested in conditional jumps here @@ -2110,6 +2024,6 @@ PhaseStatus Compiler::optOptimizeBools() JITDUMP("\noptimized %u BBJ_COND cases, %u BBJ_RETURN cases in %u passes\n", numCond, numReturn, numPasses); - modified |= stress || ((numCond + numReturn) > 0); + const bool modified = stress || ((numCond + numReturn) > 0); return modified ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; } From a85ee6dffaba34a1b907f1178e472abc50bca9a5 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 23 Mar 2025 02:12:58 +0100 Subject: [PATCH 26/37] remove comment --- src/coreclr/jit/fgopt.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 6e0f78013f32a0..c82cc4def87f7e 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -4420,7 +4420,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */, bool isPh modified = true; bNext = block->Next(); } - // Remove empty blocks + if (block->KindIs(BBJ_ALWAYS)) { bDest = block->GetTarget(); From e441ed7b17439cb87518efdaabca23cd4d4be1c5 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 23 Mar 2025 04:02:00 +0100 Subject: [PATCH 27/37] Cleanup --- src/coreclr/jit/compiler.h | 2 + src/coreclr/jit/optimizebools.cpp | 480 +++++++----------------------- 2 files changed, 109 insertions(+), 373 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 48b7fc24a6723b..859e4f9000eb31 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5384,6 +5384,8 @@ class Compiler FoldResult fgFoldConditional(BasicBlock* block); + bool fgFoldCondToReturnBlock(BasicBlock* block); + struct MorphUnreachableInfo { MorphUnreachableInfo(Compiler* comp); diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 4a6057e122d014..062ca190bfc0b6 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -67,20 +67,17 @@ class OptBoolsDsc { m_b1 = b1; m_b2 = b2; - m_b3 = nullptr; m_comp = comp; } private: BasicBlock* m_b1; // The first basic block with the BBJ_COND conditional jump type - BasicBlock* m_b2; // The next basic block of m_b1. Either BBJ_COND or BBJ_RETURN type - BasicBlock* m_b3; // m_b1's target block. Null if m_b2 is not a return block. + BasicBlock* m_b2; // The next basic block of m_b1. BBJ_COND type Compiler* m_comp; // The pointer to the Compiler instance OptTestInfo m_testInfo1; // The first test info OptTestInfo m_testInfo2; // The second test info - GenTree* m_t3; // The root node of the first statement of m_b3 GenTree* m_c1; // The first operand of m_testInfo1.compTree GenTree* m_c2; // The first operand of m_testInfo2.compTree @@ -95,7 +92,6 @@ class OptBoolsDsc bool optOptimizeBoolsCondBlock(); bool optOptimizeCompareChainCondBlock(); bool optOptimizeRangeTests(); - bool optOptimizeBoolsReturnBlock(BasicBlock* b3); #ifdef DEBUG void optOptimizeBoolsGcStress(); #endif @@ -125,16 +121,6 @@ class OptBoolsDsc // B1 : brtrue(t1|t2, BX) // B3 : // -// For example, (x == 0 && y == 0 && z == 0) generates -// B1: GT_JTRUE (BBJ_COND), jump to B4 -// B2: GT_JTRUE (BBJ_COND), jump to B4 -// B3: GT_RETURN/GT_SWIFT_ERROR_RET (BBJ_RETURN) -// B4: GT_RETURN/GT_SWIFT_ERROR_RET (BBJ_RETURN) -// and B1 and B2 are folded into B1: -// B1: GT_JTRUE (BBJ_COND), jump to B4 -// B3: GT_RETURN/GT_SWIFT_ERROR_RET (BBJ_RETURN) -// B4: GT_RETURN/GT_SWIFT_ERROR_RET (BBJ_RETURN) -// // Case 2: if B2->FalseTargetIs(B1->GetTarget()), it transforms // B1 : brtrue(t1, B3) // B2 : brtrue(t2, Bx) @@ -145,12 +131,9 @@ class OptBoolsDsc // bool OptBoolsDsc::optOptimizeBoolsCondBlock() { - assert(m_b1 != nullptr && m_b2 != nullptr && m_b3 == nullptr); + assert(m_b1 != nullptr && m_b2 != nullptr); // Check if m_b1 and m_b2 jump to the same target and get back pointers to m_testInfo1 and t2 tree nodes - - m_t3 = nullptr; - // Check if m_b1 and m_b2 have the same target if (m_b1->TrueTargetIs(m_b2->GetTrueTarget())) @@ -751,7 +734,7 @@ bool FoldRangeTests(Compiler* comp, GenTreeOp* cmp1, bool cmp1IsReversed, GenTre bool OptBoolsDsc::optOptimizeRangeTests() { // 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 != nullptr) && (m_b2 != nullptr)); assert(m_b1->KindIs(BBJ_COND) && m_b2->KindIs(BBJ_COND) && m_b1->FalseTargetIs(m_b2)); if (m_b2->isRunRarely()) @@ -962,8 +945,7 @@ bool OptBoolsDsc::optOptimizeRangeTests() // bool OptBoolsDsc::optOptimizeCompareChainCondBlock() { - assert((m_b1 != nullptr) && (m_b2 != nullptr) && (m_b3 == nullptr)); - m_t3 = nullptr; + assert((m_b1 != nullptr) && (m_b2 != nullptr)); bool foundEndOfOrConditions = false; if (m_b1->FalseTargetIs(m_b2) && m_b2->FalseTargetIs(m_b1->GetTrueTarget())) @@ -1121,15 +1103,9 @@ Statement* OptBoolsDsc::optOptimizeBoolsChkBlkCond() { assert(m_b1 != nullptr && m_b2 != nullptr); - bool optReturnBlock = false; - if (m_b3 != nullptr) - { - optReturnBlock = true; - } - // Find the block conditions of m_b1 and m_b2 - if (m_b2->countOfInEdges() > 1 || (optReturnBlock && m_b3->countOfInEdges() > 1)) + if (m_b2->countOfInEdges() > 1) { return nullptr; } @@ -1150,49 +1126,7 @@ Statement* OptBoolsDsc::optOptimizeBoolsChkBlkCond() } GenTree* testTree2 = s2->GetRootNode(); - - if (!optReturnBlock) - { - assert(testTree2->OperIs(GT_JTRUE)); - } - else - { - if (!testTree2->OperIs(GT_RETURN, GT_SWIFT_ERROR_RET)) - { - return nullptr; - } - - Statement* s3 = m_b3->firstStmt(); - if (s3->GetPrevStmt() != s3) - { - return nullptr; - } - - GenTree* testTree3 = s3->GetRootNode(); - if (!testTree3->OperIs(GT_RETURN, GT_SWIFT_ERROR_RET)) - { - return nullptr; - } - - if (!varTypeIsIntegral(testTree2->TypeGet()) || !varTypeIsIntegral(testTree3->TypeGet())) - { - return nullptr; - } - - // The third block is Return with "CNS_INT int 0/1" - GenTree* const retVal = testTree3->AsOp()->GetReturnValue(); - if (!retVal->OperIs(GT_CNS_INT)) - { - return nullptr; - } - - if (!retVal->TypeIs(TYP_INT)) - { - return nullptr; - } - - m_t3 = testTree3; - } + assert(testTree2->OperIs(GT_JTRUE)); m_testInfo1.testStmt = s1; m_testInfo1.testTree = testTree1; @@ -1263,13 +1197,6 @@ bool OptBoolsDsc::optOptimizeBoolsChkTypeCostCond() 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); @@ -1278,17 +1205,6 @@ void OptBoolsDsc::optOptimizeBoolsUpdateTrees() 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 // @@ -1300,15 +1216,6 @@ void OptBoolsDsc::optOptimizeBoolsUpdateTrees() /* Modify the target of the conditional jump and update bbRefs and bbPreds */ - if (optReturnBlock) - { - assert(m_b1->KindIs(BBJ_COND)); - assert(m_b2->KindIs(BBJ_RETURN)); - assert(m_b1->FalseTargetIs(m_b2)); - assert(m_b3 != nullptr); - m_b1->SetKindAndTargetEdge(BBJ_RETURN); - } - else { // Modify b1, if necessary, so it has the same // true target as b2. @@ -1386,253 +1293,8 @@ void OptBoolsDsc::optOptimizeBoolsUpdateTrees() // If m_b2 was the last block of a try or handler, update the EH table. m_comp->ehUpdateForDeletedBlock(m_b2); - if (optReturnBlock) - { - // Get rid of the third block - m_comp->fgUnlinkBlockForRemoval(m_b3); - m_b3->SetFlags(BBF_REMOVED); - // If m_b3 was the last block of a try or handler, update the EH table. - m_comp->ehUpdateForDeletedBlock(m_b3); - } - // Update IL range of first block - m_b1->bbCodeOffsEnd = optReturnBlock ? m_b3->bbCodeOffsEnd : m_b2->bbCodeOffsEnd; -} - -//----------------------------------------------------------------------------- -// optOptimizeBoolsReturnBlock: Optimize boolean when m_b1 is BBJ_COND and m_b2 and m_b3 are BBJ_RETURN -// -// Arguments: -// b3: Pointer to basic block b3 -// -// Returns: -// true if boolean optimization is done and m_b1, m_b2 and m_b3 are folded into m_b1, else false. -// -// Notes: -// m_b1, m_b2 and m_b3 of OptBoolsDsc are set on entry. -// -// if B1->TargetIs(b3), it transforms -// B1 : brtrue(t1, B3) -// B2 : ret(t2) -// B3 : ret(0) -// to -// B1 : ret((!t1) && t2) -// -// For example, (x==0 && y==0) generates: -// B1: GT_JTRUE (BBJ_COND), jumps to B3 -// B2: GT_RETURN/GT_SWIFT_ERROR (BBJ_RETURN) -// B3: GT_RETURN/GT_SWIFT_ERROR (BBJ_RETURN), -// and it is folded into -// B1: GT_RETURN/GT_SWIFT_ERROR (BBJ_RETURN) -// -bool OptBoolsDsc::optOptimizeBoolsReturnBlock(BasicBlock* b3) -{ - assert(m_b1 != nullptr && m_b2 != nullptr); - - // m_b3 is set for cond/return/return case - m_b3 = b3; - - m_sameTarget = false; - Statement* const s1 = optOptimizeBoolsChkBlkCond(); - if (s1 == nullptr) - { - return false; - } - - // Find the branch conditions of m_b1 and m_b2 - - m_c1 = optIsBoolComp(&m_testInfo1); - if (m_c1 == nullptr) - { - return false; - } - - m_c2 = optIsBoolComp(&m_testInfo2); - if (m_c2 == nullptr) - { - return false; - } - - // Find the type and cost conditions of m_testInfo1 and m_testInfo2 - - if (!optOptimizeBoolsChkTypeCostCond()) - { - return false; - } - - // Get the fold operator (m_foldOp, e.g., GT_OR/GT_AND) and - // the comparison operator (m_cmpOp, e.g., GT_EQ/GT_NE/GT_GE/GT_LT) - - var_types foldType = genActualType(m_c1->TypeGet()); - if (varTypeIsGC(foldType)) - { - foldType = TYP_I_IMPL; - } - m_foldType = foldType; - - m_foldOp = GT_NONE; - m_cmpOp = GT_NONE; - - genTreeOps foldOp; - genTreeOps cmpOp; - - ssize_t it1val = m_testInfo1.compTree->AsOp()->gtOp2->AsIntCon()->gtIconVal; - ssize_t it2val = m_testInfo2.compTree->AsOp()->gtOp2->AsIntCon()->gtIconVal; - ssize_t it3val = m_t3->AsOp()->gtOp1->AsIntCon()->gtIconVal; - - if (m_c1->gtOper == GT_LCL_VAR && m_c2->gtOper == GT_LCL_VAR && - m_c1->AsLclVarCommon()->GetLclNum() == m_c2->AsLclVarCommon()->GetLclNum()) - { - if (((m_testInfo1.compTree->gtOper == GT_LT && m_testInfo2.compTree->gtOper == GT_EQ) || - (m_testInfo1.compTree->gtOper == GT_EQ && m_testInfo2.compTree->gtOper == GT_LT)) && - it3val == 1) - { - // Case: x < 0 || x == 0 - // t1:c1<0 t2:c2==0 t3:c3==1 - // ==> true if c1<=0 - // - // Case: x == 0 || x < 0 - // t1:c1==0 t2:c2<0 t3:c3==1 - // ==> true if c1 <= 0 - cmpOp = GT_LE; - } - else if (((m_testInfo1.compTree->gtOper == GT_GT && m_testInfo2.compTree->gtOper == GT_EQ) || - (m_testInfo1.compTree->gtOper == GT_EQ && m_testInfo2.compTree->gtOper == GT_GT)) && - it3val == 1) - { - // Case: x > 0 || x == 0 - // t1:c1<0 t2:c2==0 t3:c3==1 - // ==> true if c1>=0 - // - // Case: x == 0 || x > 0 - // t1:c1==0 t2:c2>0 t3:c3==1 - // ==> true if c1 >= 0 - cmpOp = GT_GE; - } - else if (((m_testInfo1.compTree->gtOper == GT_LT && m_testInfo2.compTree->gtOper == GT_NE) || - (m_testInfo1.compTree->gtOper == GT_EQ && m_testInfo2.compTree->gtOper == GT_GE)) && - it3val == 0) - { - // Case: x >= 0 && x != 0 - // t1:c1<0 t2:c2==0 t3:c3==0 - // ==> true if c1>0 - // - // Case: x != 0 && x >= 0 - // t1:c1==0 t2:c2>=0 t3:c3==0 - // ==> true if c1>0 - cmpOp = GT_GT; - } - else if (((m_testInfo1.compTree->gtOper == GT_GT && m_testInfo2.compTree->gtOper == GT_NE) || - (m_testInfo1.compTree->gtOper == GT_EQ && m_testInfo2.compTree->gtOper == GT_LE)) && - it3val == 0) - { - // Case: x <= 0 && x != 0 - // t1:c1<0 t2:c2==0 t3:c3==0 - // ==> true if c1<0 - // - // Case: x != 0 && x <= 0 - // t1:c1==0 t2:c2<=0 t3:c3==0 - // ==> true if c1<0 - cmpOp = GT_LT; - } - else - { - return false; - } - - foldOp = GT_NONE; - } - else if ((m_testInfo1.compTree->gtOper == GT_NE && m_testInfo2.compTree->gtOper == GT_EQ) && - (it1val == 0 && it2val == 0 && it3val == 0)) - { - // Case: x == 0 && y == 0 - // t1:c1!=0 t2:c2==0 t3:c3==0 - // ==> true if (c1|c2)==0 - foldOp = GT_OR; - cmpOp = GT_EQ; - } - else if ((m_testInfo1.compTree->gtOper == GT_EQ && m_testInfo2.compTree->gtOper == GT_NE) && - (it1val == 0 && it2val == 0 && it3val == 0)) - { - // Case: x == 1 && y ==1 - // t1:c1!=1 t2:c2==1 t3:c3==0 is reversed from optIsBoolComp() to: t1:c1==0 t2:c2!=0 t3:c3==0 - // ==> true if (c1&c2)!=0 - foldOp = GT_AND; - cmpOp = GT_NE; - } - else if ((m_testInfo1.compTree->gtOper == GT_LT && m_testInfo2.compTree->gtOper == GT_GE) && - (it1val == 0 && it2val == 0 && it3val == 0) && - (!m_testInfo1.GetTestOp()->IsUnsigned() && !m_testInfo2.GetTestOp()->IsUnsigned())) - { - // Case: x >= 0 && y >= 0 - // t1:c1<0 t2:c2>=0 t3:c3==0 - // ==> true if (c1|c2)>=0 - - foldOp = GT_OR; - cmpOp = GT_GE; - } - else if ((m_testInfo1.compTree->gtOper == GT_EQ && m_testInfo2.compTree->gtOper == GT_EQ) && - (it1val == 0 && it2val == 0 && it3val == 1)) - { - // Case: x == 0 || y == 0 - // t1:c1==0 t2:c2==0 t3:c3==1 - // ==> true if (c1&c2)==0 - foldOp = GT_AND; - cmpOp = GT_EQ; - } - else if ((m_testInfo1.compTree->gtOper == GT_NE && m_testInfo2.compTree->gtOper == GT_NE) && - (it1val == 0 && it2val == 0 && it3val == 1)) - { - // Case: x == 1 || y == 1 - // t1:c1==1 t2:c2==1 t3:c3==1 is reversed from optIsBoolComp() to: t1:c1!=0 t2:c2!=0 t3:c3==1 - // ==> true if (c1|c2)!=0 - foldOp = GT_OR; - cmpOp = GT_NE; - } - else if ((m_testInfo1.compTree->gtOper == GT_LT && m_testInfo2.compTree->gtOper == GT_LT) && - (it1val == 0 && it2val == 0 && it3val == 1) && - (!m_testInfo1.GetTestOp()->IsUnsigned() && !m_testInfo2.GetTestOp()->IsUnsigned())) - { - // Case: x < 0 || y < 0 - // t1:c1<0 t2:c2<0 t3:c3==1 - // ==> true if (c1|c2)<0 - - foldOp = GT_OR; - cmpOp = GT_LT; - } - else - { - // Require NOT operation for operand(s). Do Not fold. - return false; - } - - if ((foldOp == GT_AND || (cmpOp == GT_NE && foldOp != GT_OR)) && (!m_testInfo1.isBool || !m_testInfo2.isBool)) - { - // x == 1 && y == 1: Skip cases where x or y is greater than 1, e.g., x=3, y=1 - // x == 0 || y == 0: Skip cases where x and y have opposite bits set, e.g., x=2, y=1 - // x == 1 || y == 1: Skip cases where either x or y is greater than 1, e.g., x=2, y=0 - return false; - } - - m_foldOp = foldOp; - m_cmpOp = cmpOp; - - // Now update the trees - - optOptimizeBoolsUpdateTrees(); - -#ifdef DEBUG - if (m_comp->verbose) - { - printf("Folded %sboolean conditions of " FMT_BB ", " FMT_BB " and " FMT_BB " to :\n", - m_c2->OperIsLeaf() ? "" : "non-leaf ", m_b1->bbNum, m_b2->bbNum, m_b3->bbNum); - m_comp->gtDispStmt(s1); - printf("\n"); - } -#endif - - // Return true to continue the bool optimization for the rest of the BB chain - return true; + m_b1->bbCodeOffsEnd = m_b2->bbCodeOffsEnd; } //----------------------------------------------------------------------------- @@ -1918,7 +1580,6 @@ PhaseStatus Compiler::optOptimizeBools() bool change = false; bool retry = false; unsigned numCond = 0; - unsigned numReturn = 0; unsigned numPasses = 0; unsigned stress = false; @@ -1930,6 +1591,11 @@ PhaseStatus Compiler::optOptimizeBools() for (BasicBlock* b1 = fgFirstBB; b1 != nullptr; b1 = retry ? b1 : b1->Next()) { retry = false; + if (b1->KindIs(BBJ_COND) && fgFoldCondToReturnBlock(b1)) + { + change = true; + numCond++; + } // We're only interested in conditional jumps here @@ -1987,31 +1653,6 @@ PhaseStatus Compiler::optOptimizeBools() } #endif } - else if (b2->KindIs(BBJ_RETURN)) - { - // Set b3 to b1 jump destination - BasicBlock* b3 = b1->GetTrueTarget(); - - // b3 must not be marked as BBF_DONT_REMOVE - - if (b3->HasFlag(BBF_DONT_REMOVE)) - { - continue; - } - - // b3 must be RETURN type - - if (!b3->KindIs(BBJ_RETURN)) - { - continue; - } - - if (optBoolsDsc.optOptimizeBoolsReturnBlock(b3)) - { - change = true; - numReturn++; - } - } else { #ifdef DEBUG @@ -2022,8 +1663,101 @@ PhaseStatus Compiler::optOptimizeBools() } } while (change); - JITDUMP("\noptimized %u BBJ_COND cases, %u BBJ_RETURN cases in %u passes\n", numCond, numReturn, numPasses); + JITDUMP("\noptimized %u BBJ_COND cases in %u passes\n", numCond, numPasses); - const bool modified = stress || ((numCond + numReturn) > 0); + const bool modified = stress || (numCond > 0); return modified ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; } + +//------------------------------------------------------------- +// fgFoldCondToReturnBlock: Folds BBJ_COND into BBJ_RETURN +// This operation is the opposite of what fgDedupReturnComparison does. +// We don't fold such conditionals if both return blocks have multiple predecessors. +// +// Arguments: +// block - the BBJ_COND block to convert into BBJ_RETURN +// +// Returns: +// true if the block was converted into BBJ_RETURN +// +bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) +{ + assert(block->KindIs(BBJ_COND)); + +#ifdef JIT32_GCENCODER + // JIT32_GCENCODER has a hard limit on the number of epilogues. + return false; +#endif + + // Early out if the current method is not returning a boolean. + if ((info.compRetType != TYP_UBYTE) || (genReturnBB != nullptr)) + { + return false; + } + + // Both edges must be BBJ_RETURN + BasicBlock* retTrueBb = block->GetTrueEdge()->getDestinationBlock(); + BasicBlock* retFalseBb = block->GetFalseEdge()->getDestinationBlock(); + if (!retTrueBb->KindIs(BBJ_RETURN) || !retFalseBb->KindIs(BBJ_RETURN)) + { + return false; + } + + GenTree* node = block->lastStmt()->GetRootNode(); + GenTree* cond = node->gtGetOp1(); + if (!cond->OperIsCompare()) + { + return false; + } + + if ((retTrueBb->GetUniquePred(this) == nullptr) && (retFalseBb->GetUniquePred(this) == nullptr)) + { + // Both return blocks have multiple predecessors - bail out. + // We don't want to introduce a new epilogue. + return false; + } + + // Is block a BBJ_RETURN(1/0) ? (single statement) + auto isReturnBool = [](const BasicBlock* block, bool value) { + if (block->KindIs(BBJ_RETURN) && block->hasSingleStmt() && (block->lastStmt() != nullptr)) + { + GenTree* node = block->lastStmt()->GetRootNode(); + return node->OperIs(GT_RETURN) && node->gtGetOp1()->IsIntegralConst(value ? 1 : 0); + } + return false; + }; + + // Make sure we deal with true/false return blocks (or false/true) + bool retTrueFalse = isReturnBool(retTrueBb, true) && isReturnBool(retFalseBb, false); + bool retFalseTrue = isReturnBool(retTrueBb, false) && isReturnBool(retFalseBb, true); + if (!retTrueFalse && !retFalseTrue) + { + return false; + } + + // Reverse the condition if we jump to "return false" on true. + if (retFalseTrue) + { + cond->SetOper(GenTree::ReverseRelop(cond->OperGet())); + } + + assert(cond->TypeIs(TYP_INT)); + assert(BasicBlock::sameEHRegion(block, retTrueBb)); + assert(BasicBlock::sameEHRegion(block, retFalseBb)); + + // Unlink the return blocks, someone will pick them up later. + fgRemoveRefPred(block->GetTrueEdge()); + fgRemoveRefPred(block->GetFalseEdge()); + block->SetKindAndTargetEdge(BBJ_RETURN); + node->ChangeOper(GT_RETURN); + node->ChangeType(TYP_INT); + cond->gtFlags &= ~GTF_RELOP_JMP_USED; + + // It's difficult to restore the original weight of the block + fgPgoConsistent = false; + block->bbCodeOffsEnd = max(retTrueBb->bbCodeOffsEnd, retFalseBb->bbCodeOffsEnd); + gtSetStmtInfo(block->lastStmt()); + fgSetStmtSeq(block->lastStmt()); + gtUpdateStmtSideEffects(block->lastStmt()); + return true; +} From 76cdec5fcf3d69b050d14f88a6eeda90069fb91a Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 23 Mar 2025 14:41:08 +0100 Subject: [PATCH 28/37] Clean up --- src/coreclr/jit/optimizebools.cpp | 47 +++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 062ca190bfc0b6..ae914c427eec08 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -1682,39 +1682,65 @@ PhaseStatus Compiler::optOptimizeBools() // bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) { + bool modified = false; + assert(block->KindIs(BBJ_COND)); #ifdef JIT32_GCENCODER // JIT32_GCENCODER has a hard limit on the number of epilogues. - return false; + return changed; #endif // Early out if the current method is not returning a boolean. if ((info.compRetType != TYP_UBYTE) || (genReturnBB != nullptr)) { - return false; + return modified; } // Both edges must be BBJ_RETURN BasicBlock* retTrueBb = block->GetTrueEdge()->getDestinationBlock(); BasicBlock* retFalseBb = block->GetFalseEdge()->getDestinationBlock(); + + // Although, we might want to fold fallthrough BBJ_ALWAYS blocks first + if (fgCanCompactBlock(retTrueBb)) + { + fgCompactBlock(retTrueBb); + modified = true; + } + + if (fgCanCompactBlock(retFalseBb)) + { + fgCompactBlock(retFalseBb); + modified = true; + } + + if (!block->KindIs(BBJ_COND)) + { + // In rare cases fgCompactBlock() might have changed the current block + return modified; + } + + retTrueBb = block->GetTrueEdge()->getDestinationBlock(); + retFalseBb = block->GetFalseEdge()->getDestinationBlock(); if (!retTrueBb->KindIs(BBJ_RETURN) || !retFalseBb->KindIs(BBJ_RETURN)) { - return false; + return modified; } + // It has to be JTRUE(cond) or JTRUE(comma(cond)), but let's be resilient. + assert(block->lastStmt() != nullptr); GenTree* node = block->lastStmt()->GetRootNode(); GenTree* cond = node->gtGetOp1(); if (!cond->OperIsCompare()) { - return false; + return modified; } if ((retTrueBb->GetUniquePred(this) == nullptr) && (retFalseBb->GetUniquePred(this) == nullptr)) { // Both return blocks have multiple predecessors - bail out. // We don't want to introduce a new epilogue. - return false; + return modified; } // Is block a BBJ_RETURN(1/0) ? (single statement) @@ -1732,7 +1758,7 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) bool retFalseTrue = isReturnBool(retTrueBb, false) && isReturnBool(retFalseBb, true); if (!retTrueFalse && !retFalseTrue) { - return false; + return modified; } // Reverse the condition if we jump to "return false" on true. @@ -1740,7 +1766,7 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) { cond->SetOper(GenTree::ReverseRelop(cond->OperGet())); } - + modified = true; assert(cond->TypeIs(TYP_INT)); assert(BasicBlock::sameEHRegion(block, retTrueBb)); assert(BasicBlock::sameEHRegion(block, retFalseBb)); @@ -1753,11 +1779,14 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) node->ChangeType(TYP_INT); cond->gtFlags &= ~GTF_RELOP_JMP_USED; - // It's difficult to restore the original weight of the block + // It's difficult to restore the original weight of the block, profile repair will handle it. fgPgoConsistent = false; block->bbCodeOffsEnd = max(retTrueBb->bbCodeOffsEnd, retFalseBb->bbCodeOffsEnd); gtSetStmtInfo(block->lastStmt()); fgSetStmtSeq(block->lastStmt()); gtUpdateStmtSideEffects(block->lastStmt()); - return true; + + JITDUMP("fgFoldCondToReturnBlock: folding " FMT_BB " from BBJ_COND into BBJ_RETURN:", block->bbNum); + DISPBLOCK(block) + return modified; } From 8d32cffe61d892878b3fd9df055290c2551c0b52 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 23 Mar 2025 15:10:28 +0100 Subject: [PATCH 29/37] fix build --- 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 ae914c427eec08..f8a00a4f34e81c 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -1688,7 +1688,7 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) #ifdef JIT32_GCENCODER // JIT32_GCENCODER has a hard limit on the number of epilogues. - return changed; + return modified; #endif // Early out if the current method is not returning a boolean. From a04750bad6cc39612c1228736b9e4dcfe4c57e5c Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 23 Mar 2025 16:21:31 +0100 Subject: [PATCH 30/37] fix build --- src/coreclr/jit/optimizebools.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index f8a00a4f34e81c..e5a6c4ba744300 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -1698,8 +1698,8 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) } // Both edges must be BBJ_RETURN - BasicBlock* retTrueBb = block->GetTrueEdge()->getDestinationBlock(); BasicBlock* retFalseBb = block->GetFalseEdge()->getDestinationBlock(); + BasicBlock* retTrueBb = block->GetTrueEdge()->getDestinationBlock(); // Although, we might want to fold fallthrough BBJ_ALWAYS blocks first if (fgCanCompactBlock(retTrueBb)) @@ -1707,16 +1707,16 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) fgCompactBlock(retTrueBb); modified = true; } - - if (fgCanCompactBlock(retFalseBb)) + // By the time we get to the retFalseBb, it might be removed by fgCompactBlock() + // so we need to check if it is still valid. + if (!retFalseBb->HasFlag(BBF_REMOVED) && fgCanCompactBlock(retFalseBb)) { fgCompactBlock(retFalseBb); modified = true; } - + // Same here - bail out if the block is no longer BBJ_COND after compacting. if (!block->KindIs(BBJ_COND)) { - // In rare cases fgCompactBlock() might have changed the current block return modified; } @@ -1724,10 +1724,12 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) retFalseBb = block->GetFalseEdge()->getDestinationBlock(); if (!retTrueBb->KindIs(BBJ_RETURN) || !retFalseBb->KindIs(BBJ_RETURN)) { + // Both edges must be BBJ_RETURN return modified; } - // It has to be JTRUE(cond) or JTRUE(comma(cond)), but let's be resilient. + // The last statement has to be either JTRUE(cond) or JTRUE(comma(cond)), + // but let's be resilient just in case. assert(block->lastStmt() != nullptr); GenTree* node = block->lastStmt()->GetRootNode(); GenTree* cond = node->gtGetOp1(); From 8430840e764e033362f6a8f9c97a678d8fb67b43 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Sun, 23 Mar 2025 19:17:15 +0100 Subject: [PATCH 31/37] Comment out conditional block in optimizebools.cpp --- src/coreclr/jit/optimizebools.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index e5a6c4ba744300..51584ffc0fbf26 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -1591,11 +1591,11 @@ PhaseStatus Compiler::optOptimizeBools() for (BasicBlock* b1 = fgFirstBB; b1 != nullptr; b1 = retry ? b1 : b1->Next()) { retry = false; - if (b1->KindIs(BBJ_COND) && fgFoldCondToReturnBlock(b1)) + /*if (b1->KindIs(BBJ_COND) && fgFoldCondToReturnBlock(b1)) { change = true; numCond++; - } + }*/ // We're only interested in conditional jumps here From 4b9df079aab76d06d58b0d8c10195323e88a7935 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 23 Mar 2025 22:12:40 +0100 Subject: [PATCH 32/37] fix issue with arm32 --- src/coreclr/jit/optimizebools.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 51584ffc0fbf26..472c039736ad39 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -1591,11 +1591,11 @@ PhaseStatus Compiler::optOptimizeBools() for (BasicBlock* b1 = fgFirstBB; b1 != nullptr; b1 = retry ? b1 : b1->Next()) { retry = false; - /*if (b1->KindIs(BBJ_COND) && fgFoldCondToReturnBlock(b1)) + if (b1->KindIs(BBJ_COND) && fgFoldCondToReturnBlock(b1)) { change = true; numCond++; - }*/ + } // We're only interested in conditional jumps here @@ -1722,7 +1722,8 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) retTrueBb = block->GetTrueEdge()->getDestinationBlock(); retFalseBb = block->GetFalseEdge()->getDestinationBlock(); - if (!retTrueBb->KindIs(BBJ_RETURN) || !retFalseBb->KindIs(BBJ_RETURN)) + if (!retTrueBb->KindIs(BBJ_RETURN) || !retFalseBb->KindIs(BBJ_RETURN) || + !BasicBlock::sameEHRegion(block, retTrueBb) || !BasicBlock::sameEHRegion(block, retFalseBb)) { // Both edges must be BBJ_RETURN return modified; @@ -1737,6 +1738,7 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) { return modified; } + assert(cond->TypeIs(TYP_INT)); if ((retTrueBb->GetUniquePred(this) == nullptr) && (retFalseBb->GetUniquePred(this) == nullptr)) { @@ -1766,12 +1768,9 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) // Reverse the condition if we jump to "return false" on true. if (retFalseTrue) { - cond->SetOper(GenTree::ReverseRelop(cond->OperGet())); + gtReverseCond(cond); } modified = true; - assert(cond->TypeIs(TYP_INT)); - assert(BasicBlock::sameEHRegion(block, retTrueBb)); - assert(BasicBlock::sameEHRegion(block, retFalseBb)); // Unlink the return blocks, someone will pick them up later. fgRemoveRefPred(block->GetTrueEdge()); From aea34d2f8f516d6c02363287eb085da7ab15f747 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Mon, 24 Mar 2025 16:57:45 +0100 Subject: [PATCH 33/37] Apply suggestions from code review Co-authored-by: Aman Khalid --- src/coreclr/jit/optimizebools.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 472c039736ad39..1be4f12915a48a 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -1698,8 +1698,8 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) } // Both edges must be BBJ_RETURN - BasicBlock* retFalseBb = block->GetFalseEdge()->getDestinationBlock(); - BasicBlock* retTrueBb = block->GetTrueEdge()->getDestinationBlock(); + BasicBlock* retFalseBb = block->GetFalseTarget(); + BasicBlock* retTrueBb = block->GetTrueTarget(); // Although, we might want to fold fallthrough BBJ_ALWAYS blocks first if (fgCanCompactBlock(retTrueBb)) From 8e5980d4e8907110ada5d347be34cba24c430a1f Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 24 Mar 2025 17:07:17 +0100 Subject: [PATCH 34/37] Address feedback --- src/coreclr/jit/fgdiagnostic.cpp | 2 +- src/coreclr/jit/optimizebools.cpp | 13 ++++++++----- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index 422b6fe6ef1731..3133d8d6a70b34 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -3285,8 +3285,8 @@ void Compiler::fgDebugCheckTypes(GenTree* tree) case GT_BOUNDS_CHECK: if (!node->TypeIs(TYP_VOID)) { - printf("The tree is expected to be TYP_VOID:\n"); m_compiler->gtDispTree(node); + assert("The tree is expected to be of TYP_VOID type"); } break; diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 1be4f12915a48a..0a7a348dccc606 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -1720,8 +1720,8 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) return modified; } - retTrueBb = block->GetTrueEdge()->getDestinationBlock(); - retFalseBb = block->GetFalseEdge()->getDestinationBlock(); + retTrueBb = block->GetTrueTarget(); + retFalseBb = block->GetFalseTarget(); if (!retTrueBb->KindIs(BBJ_RETURN) || !retFalseBb->KindIs(BBJ_RETURN) || !BasicBlock::sameEHRegion(block, retTrueBb) || !BasicBlock::sameEHRegion(block, retFalseBb)) { @@ -1772,7 +1772,12 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) } modified = true; - // Unlink the return blocks, someone will pick them up later. + // Decrease the weight of the return blocks since we no longer have edges to them. + // Although, they still might be reachable from other blocks (at least one of them). + retTrueBb->decreaseBBProfileWeight(block->GetTrueEdge()->getLikelyWeight()); + retFalseBb->decreaseBBProfileWeight(block->GetFalseEdge()->getLikelyWeight()); + + // Unlink the return blocks fgRemoveRefPred(block->GetTrueEdge()); fgRemoveRefPred(block->GetFalseEdge()); block->SetKindAndTargetEdge(BBJ_RETURN); @@ -1780,8 +1785,6 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) node->ChangeType(TYP_INT); cond->gtFlags &= ~GTF_RELOP_JMP_USED; - // It's difficult to restore the original weight of the block, profile repair will handle it. - fgPgoConsistent = false; block->bbCodeOffsEnd = max(retTrueBb->bbCodeOffsEnd, retFalseBb->bbCodeOffsEnd); gtSetStmtInfo(block->lastStmt()); fgSetStmtSeq(block->lastStmt()); From 68a7aea7466dbacb3d6998984901592218bf168a Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 24 Mar 2025 17:44:08 +0100 Subject: [PATCH 35/37] Address feedback --- src/coreclr/jit/optimizebools.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index 0a7a348dccc606..43c1d2a2c80c8b 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -1774,8 +1774,14 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) // Decrease the weight of the return blocks since we no longer have edges to them. // Although, they still might be reachable from other blocks (at least one of them). - retTrueBb->decreaseBBProfileWeight(block->GetTrueEdge()->getLikelyWeight()); - retFalseBb->decreaseBBProfileWeight(block->GetFalseEdge()->getLikelyWeight()); + if (retTrueBb->hasProfileWeight()) + { + retTrueBb->decreaseBBProfileWeight(block->GetTrueEdge()->getLikelyWeight()); + } + if (retFalseBb->hasProfileWeight()) + { + retFalseBb->decreaseBBProfileWeight(block->GetFalseEdge()->getLikelyWeight()); + } // Unlink the return blocks fgRemoveRefPred(block->GetTrueEdge()); From aa74f0890b5713866ee406c2e3899a77250dbdb5 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Mon, 24 Mar 2025 17:47:45 +0100 Subject: [PATCH 36/37] Update src/coreclr/jit/optimizebools.cpp Co-authored-by: Aman Khalid --- 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 43c1d2a2c80c8b..f008645b998b20 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -1773,7 +1773,7 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) modified = true; // Decrease the weight of the return blocks since we no longer have edges to them. - // Although, they still might be reachable from other blocks (at least one of them). + // Although one might still be reachable from other blocks. if (retTrueBb->hasProfileWeight()) { retTrueBb->decreaseBBProfileWeight(block->GetTrueEdge()->getLikelyWeight()); From ce10e5c8665147cf8ff11900e04402eeaff23364 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 24 Mar 2025 17:59:40 +0100 Subject: [PATCH 37/37] Address feedback --- src/coreclr/jit/optimizebools.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/optimizebools.cpp b/src/coreclr/jit/optimizebools.cpp index f008645b998b20..451f35241799e2 100644 --- a/src/coreclr/jit/optimizebools.cpp +++ b/src/coreclr/jit/optimizebools.cpp @@ -1692,7 +1692,7 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) #endif // Early out if the current method is not returning a boolean. - if ((info.compRetType != TYP_UBYTE) || (genReturnBB != nullptr)) + if ((info.compRetType != TYP_UBYTE)) { return modified; } @@ -1723,7 +1723,8 @@ bool Compiler::fgFoldCondToReturnBlock(BasicBlock* block) retTrueBb = block->GetTrueTarget(); retFalseBb = block->GetFalseTarget(); if (!retTrueBb->KindIs(BBJ_RETURN) || !retFalseBb->KindIs(BBJ_RETURN) || - !BasicBlock::sameEHRegion(block, retTrueBb) || !BasicBlock::sameEHRegion(block, retFalseBb)) + !BasicBlock::sameEHRegion(block, retTrueBb) || !BasicBlock::sameEHRegion(block, retFalseBb) || + (retTrueBb == genReturnBB) || (retFalseBb == genReturnBB)) { // Both edges must be BBJ_RETURN return modified;