-
Notifications
You must be signed in to change notification settings - Fork 5.1k
JIT: Don't reverse copied condition in fgOptimizeBranch #115842
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JIT: Don't reverse copied condition in fgOptimizeBranch #115842
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR removes the unnecessary reversal of branch conditions in fgOptimizeBranch
, streamlining how branch edges and likelihoods are handled now that the pass runs later in the frontend.
- Eliminates
gtReverseCond(condTree)
call and related comment block. - Directly retrieves
bDest
’s false/true edges instead of flipping them. - Updates edge redirection and likelihood assignment for
bJump
. - Adjusts
SetCond
invocation to use the new true edge and the redirected false edge.
Comments suppressed due to low confidence (2)
src/coreclr/jit/fgopt.cpp:2742
- [nitpick] The names
falseEdge
andtrueEdge
refer to edges on the originalbDest
block but could be confused with the redirected edges later. Consider renaming them todestFalseEdge
anddestTrueEdge
for clarity.
FlowEdge* const falseEdge = bDest->GetFalseEdge();
src/coreclr/jit/fgopt.cpp:2750
- Consider adding or updating a unit test to verify that
SetCond
preserves the correct edge likelihoods after optimization, ensuring this change doesn't regress profiling data handling.
bJump->SetCond(newTrueEdge, bJump->GetTargetEdge());
// Reverse the sense of the compare | ||
// | ||
gtReverseCond(condTree); | ||
|
||
// We need to update the following flags of the bJump block if they were set in the bDest block | ||
bJump->CopyFlags(bDest, BBF_COPY_PROPAGATE); | ||
|
||
// Update bbRefs and bbPreds | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a brief comment here explaining that we no longer reverse the condition and are using the original bDest
edges directly, to aid future maintainers in following this logic.
// | |
// | |
// We no longer reverse the condition and instead use the original `bDest` edges directly. | |
// This ensures that the flow graph remains consistent and avoids unnecessary complexity. |
Copilot uses AI. Check for mistakes.
@dotnet/jit-contrib PTAL. Small diffs overall, though slightly bigger on x64 due to changes in branch size. Thanks! |
Now that
fgOptimizeBranch
only runs late in the frontend, removing the condition reversal is low-churn. I'm PRing this as a prerequisite to a larger rewrite I'm making to makefgOptimizeBranch
more useful for block layout's purposes (i.e. if we can predict that block layout will probably misrotate a loop, try to clone the loop condition, ideally without needing to compute/maintain loop annotations).