Skip to content

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

Merged
merged 1 commit into from
May 21, 2025

Conversation

amanasifkhalid
Copy link
Member

@amanasifkhalid amanasifkhalid commented May 21, 2025

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 make fgOptimizeBranch 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).

@Copilot Copilot AI review requested due to automatic review settings May 21, 2025 15:49
@github-actions github-actions bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 21, 2025
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

@Copilot Copilot AI left a 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 and trueEdge refer to edges on the original bDest block but could be confused with the redirected edges later. Consider renaming them to destFalseEdge and destTrueEdge 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
//
Copy link
Preview

Copilot AI May 21, 2025

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.

Suggested change
//
//
// 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.

@amanasifkhalid
Copy link
Member Author

@dotnet/jit-contrib PTAL. Small diffs overall, though slightly bigger on x64 due to changes in branch size. Thanks!

@amanasifkhalid amanasifkhalid merged commit 9694901 into dotnet:main May 21, 2025
110 checks passed
@amanasifkhalid amanasifkhalid deleted the fgOptimizeBranch-cleanup branch May 21, 2025 19:29
@github-actions github-actions bot locked and limited conversation to collaborators Jun 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants