From 313654e332fc4981cb145ddbf7af8c80483546dd Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sun, 10 Mar 2024 17:56:27 -0700 Subject: [PATCH 1/2] JIT: don't create degenerate flow in `fgReplaceJumpTarget` Detect if the update has created degenerate BBJ_COND flow, and simplify. Also symmetrize the true/false cases. Fixes #48609. Also fixes some issues seen in enhanced likelhood checking. --- src/coreclr/jit/fgbasic.cpp | 43 ++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 0eb618f177722..de030d3f769a2 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -662,6 +662,8 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas if (block->FalseEdgeIs(oldEdge)) { + // Branch was degenerate, simplify it first + // // fgRemoveRefPred returns nullptr for BBJ_COND blocks with two flow edges to target fgRemoveConditionalJump(block); assert(block->KindIs(BBJ_ALWAYS)); @@ -671,10 +673,7 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas // fgRemoveRefPred should have removed the flow edge fgRemoveRefPred(oldEdge); assert(oldEdge->getDupCount() == 0); - - // TODO-NoFallThrough: Proliferate weight from oldEdge - // (as a quirk, we avoid doing so for the true target to reduce diffs for now) - FlowEdge* const newEdge = fgAddRefPred(newTarget, block); + FlowEdge* const newEdge = fgAddRefPred(newTarget, block, oldEdge); if (block->KindIs(BBJ_ALWAYS)) { @@ -684,11 +683,6 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas { assert(block->KindIs(BBJ_COND)); block->SetTrueEdge(newEdge); - - if (oldEdge->hasLikelihood()) - { - newEdge->setLikelihood(oldEdge->getLikelihood()); - } } } else @@ -696,12 +690,41 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas assert(block->FalseTargetIs(oldTarget)); FlowEdge* const oldEdge = block->GetFalseEdge(); + if (block->TrueEdgeIs(oldEdge)) + { + // Branch was degenerate + // + // fgRemoveRefPred returns nullptr for BBJ_COND blocks with two flow edges to target + fgRemoveConditionalJump(block); + assert(block->KindIs(BBJ_ALWAYS)); + assert(block->TargetIs(oldTarget)); + } + // fgRemoveRefPred should have removed the flow edge fgRemoveRefPred(oldEdge); assert(oldEdge->getDupCount() == 0); FlowEdge* const newEdge = fgAddRefPred(newTarget, block, oldEdge); - block->SetFalseEdge(newEdge); + + if (block->KindIs(BBJ_ALWAYS)) + { + block->SetTargetEdge(newEdge); + } + else + { + assert(block->KindIs(BBJ_COND)); + block->SetFalseEdge(newEdge); + } } + + if (block->GetFalseEdge() == block->GetTrueEdge()) + { + // Block became degenerate, simplify + // + fgRemoveConditionalJump(block); + assert(block->KindIs(BBJ_ALWAYS)); + assert(block->TargetIs(newTarget)); + } + break; case BBJ_SWITCH: From a59c6f206bf72429c3d7f876028aba3d258e3601 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sun, 10 Mar 2024 19:41:56 -0700 Subject: [PATCH 2/2] review feedback --- src/coreclr/jit/fgbasic.cpp | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index de030d3f769a2..8d36598052c90 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -664,7 +664,6 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas { // Branch was degenerate, simplify it first // - // fgRemoveRefPred returns nullptr for BBJ_COND blocks with two flow edges to target fgRemoveConditionalJump(block); assert(block->KindIs(BBJ_ALWAYS)); assert(block->TargetIs(oldTarget)); @@ -690,15 +689,8 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas assert(block->FalseTargetIs(oldTarget)); FlowEdge* const oldEdge = block->GetFalseEdge(); - if (block->TrueEdgeIs(oldEdge)) - { - // Branch was degenerate - // - // fgRemoveRefPred returns nullptr for BBJ_COND blocks with two flow edges to target - fgRemoveConditionalJump(block); - assert(block->KindIs(BBJ_ALWAYS)); - assert(block->TargetIs(oldTarget)); - } + // Already degenerate cases should have taken the true path above. + assert(!block->TrueEdgeIs(oldEdge)); // fgRemoveRefPred should have removed the flow edge fgRemoveRefPred(oldEdge); @@ -716,7 +708,7 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas } } - if (block->GetFalseEdge() == block->GetTrueEdge()) + if (block->KindIs(BBJ_COND) && (block->GetFalseEdge() == block->GetTrueEdge())) { // Block became degenerate, simplify //