-
Notifications
You must be signed in to change notification settings - Fork 5k
JIT: Fix likelihoods in range test bool opts #116867
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: Fix likelihoods in range test bool opts #116867
Conversation
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 fixes the likelihood computation in range tests for bool optimizations when the second comparison is reversed.
- Moves likelihood assignments outside the if/else branches to ensure a consistent calculation of the branch probabilities.
- Removes redundant likelihood assignments that led to incorrect flipping of probabilities.
Comments suppressed due to low confidence (2)
src/coreclr/jit/optimizebools.cpp:828
- Ensure that moving the likelihood assignments outside the if/else block correctly reflects the intended behavior for both cmp2IsReversed cases. Verify that the new structure accurately flips the likelihoods as expected.
newEdge->setLikelihood(inRangeLikelihood);
src/coreclr/jit/optimizebools.cpp:841
- Check that the removal of the redundant likelihood assignments in the reversed branch does not inadvertently affect the logic ensuring the branch likelihood is correctly flipped.
assert(m_b1->TrueTargetIs(notInRangeBb));
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
@dotnet/jit-contrib PTAL. Diffs.
|
When optimizing compound conditionals into one check and branch, we'd expect the likelihood of the optimized branch to be the product of the conditionals' likelihoods. We currently get this wrong in range test opts if the second comparison is flipped, yielding behavior like this:
Based on what the profile initially looked like, the likelihoods out of
BB12
should be flipped.