-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Fix JitOptRepeat around shared constants #96130
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsWith SharedConstCse we can convert a handle e.g.:
to an ADD node:
where There are two fixes:
|
@dotnet/jit-contrib @AndyAyersMS @BruceForstall PTAL, fixes an assert seen in SPMI for JitOptRepeat |
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.
I wonder what else we could defer until the last pass.
This is suspect to me. It seems odd and probably undesirable to do some optimizations only in the last pass of OptRepeat. Specifically, it means that the results of shared constant CSE can't be further optimized. I had fixed shared constant CSE + OptRepeat asserts in #94250 (still WIP). |
Do you have a better option in mind? We basically destroy handles at the first iteration converting them into
Ah ok, didn't see that, closing (although, I don't see how it fixes the test case I mentioned) |
With SharedConstCse we can convert a handle e.g.:
to an ADD node (
0x7ffb48d0ee44 + 300
=0x7ffb48d0ef70
):where
0x7ffb48d0ee44
is picked as a shared base constant (and it's not a handle) -- it's ok since ourADD
(andCOMMA
) nodes have a VN that has ICON flag encoded into it. However, we completely erase that info if we do multiple passes of Value Numbering (JitOptRepeat) so ourADD
node will no longer have an ICON flag after 2nd pass of VN.There are two fixes:
optCreateAssertion
doesn't create "Exact type" assertion if it doesn't see the handle flagA test case where it easily reproduces: https://github.com/dotnet/runtime/blob/main/src/tests/JIT/Regression/JitBlue/Runtime_70790/Runtime_70790.cs (
s_intType
is folded to a constant, also, note- 300
there).