-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Ignore re-typed lcl in optAssertionPropLocal_RelOp #119797
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
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 a bug in the JIT compiler's local assertion propagation where incorrect type handling led to faulty optimizations. The issue occurred when the compiler incorrectly folded a comparison to true based on an assertion from a different type comparison of the same local variable.
- Adds a type check in
optAssertionPropLocal_RelOpto prevent assertion propagation when local variable types don't match - Includes a regression test to verify the fix for the specific scenario involving long-to-int type conversions
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/jit/assertionprop.cpp | Adds type validation to prevent incorrect assertion propagation across different types of the same local variable |
| src/tests/JIT/Regression/JitBlue/Runtime_119654/Runtime_119654.cs | Test case demonstrating the bug scenario with long/int type conversions and conditional logic |
| src/tests/JIT/Regression/JitBlue/Runtime_119654/Runtime_119654.csproj | Project file for the regression test |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
I presume it's similar to #112506 but opposite - there we stop creating assertions for truncated locals and here we stop consuming them for truncated locals |
I think we can move the transformation to lowering for x64 and arm64, and then keep the |
Good point, I fear a change like that might not be safe to backport to .net 10, so let's land this and then I'll implement your suggestion, will see how the diffs look like |
I definitely agree, we shouldn't backport a change like that. I just think that would be the better long term design. |
|
/backport to release/10.0 |
|
Started backporting to release/10.0: https://github.com/dotnet/runtime/actions/runs/17803482710 |
Fixes #119654
For a minimal repro:
We have two BBJ_TRUE blocks
an optimization in morph (
optNarrowTreeto be precise) optimizes theCASTnode into a re-typed local (LONG->INT) and we end up with:and then Local Assertion Prop thinks the second NE can be folded to true given the assertion created by the first JTRUE - but it's not correct since the locals are the same but their types are different (INT vs LONG).
Looks like such re-typed locals are a bug farm, unfortunately, it's a massive size regression if I disable that in
optNarrowTree