JIT: don't create degenerate flow in fgReplaceJumpTarget
#99509
Conversation
Detect if the update has created degenerate BBJ_COND flow, and simplify. Also symmetrize the true/false cases. Fixes dotnet#48609. Also fixes some issues seen in enhanced likelhood checking.
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
@amanasifkhalid PTAL |
src/coreclr/jit/fgbasic.cpp
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
Since you're here, could you please remove this comment about fgRemoveRefPred returning nullptr? The new version of it that takes in a flow edge doesn't return anything. Thanks!
src/coreclr/jit/fgbasic.cpp
Outdated
| } | ||
| } | ||
| else | ||
| { | ||
| assert(block->FalseTargetIs(oldTarget)); | ||
| FlowEdge* const oldEdge = block->GetFalseEdge(); | ||
|
|
||
| if (block->TrueEdgeIs(oldEdge)) |
There was a problem hiding this comment.
I'm a bit confused by this check here. If a block's true and false edges are the same, then shouldn't the condition if (block->TrueTargetIs(oldTarget)) above be true, making this code unreachable?
src/coreclr/jit/fgbasic.cpp
Outdated
| } | ||
|
|
||
| if (block->GetFalseEdge() == block->GetTrueEdge()) |
There was a problem hiding this comment.
I think we need to check if block is still a BBJ_COND here before calling GetFalseEdge/GetTrueEdge. If we converted it to a BBJ_ALWAYS above, this will assert.
| } | ||
|
|
||
| if (block->KindIs(BBJ_COND) && (block->GetFalseEdge() == block->GetTrueEdge())) |
There was a problem hiding this comment.
I noticed that we tend to compare the true/false edges with TrueEdgeIs. I'm planning on modifying fgReplaceJumpTarget in an upcoming PR, so I'll fix this then.
|
Diffs don't look too bad. Seems to be mostly layout changes (not surprising) and all that that entails. |
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 likelihood checking.
Diffs