-
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
Mark promoted SIMD locals used by HWIs as DNER #64855
Mark promoted SIMD locals used by HWIs as DNER #64855
Conversation
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsAs the added comment states, we must do this to get dependent promotion, as otherwise the compiler does not support independent promotion of structs not fully eliminated from the IR, save some special cases (such as multi-reg structs). We will only need to do this very rarely, as otherwise we mark SIMD locals used by SIMDs/HWIs specially when creating those nodes so as not to promote them. This issue was revealed by forward substituion, where we had: SIMD a = OBJ(ADDR(b)) // b is SIMD too
HWI(a) And forward-substituted the promoted "b" into "HWI". Notably, by the time we are forward-substituting, "we cannot really do anything about it", as struct promotion has already run, and by the time we get to morph, we too cannot do much save some gymnastics with conjuring up a tree of inserts from individual fields. No diffs are expected. Side note: this is a conservative solution, but is always correct and does not put us in a worse position CQ-wise than before. It is possible/probable that we can "do better" at marking SIMD locals as "no promotion", or abandoning promotion for them altogether, but that is out of scope for this assert fix.
|
5c3a78b
to
afde19d
Compare
As the added comment states, we must do this to get dependent promotion, as otherwise the compiler does not support independent promotion of structs not fully eliminated from the IR, save some special cases (such as multi-reg structs). We will only need to do this very rarely, as otherwise we mark SIMD locals used by SIMDs/HWIs specially when creating those nodes so as not to promote them. This issue was revealed by forward substituion, where we had: SIMD a = OBJ(ADDR(b)) // b is SIMD too HWI(a) And forward-substituted the promoted "b" into "HWI". Notably, by the time we are forward-substituting, "we cannot really do anything about it", as struct promotion has already run, and by the time we get to morph, we too cannot do much save some gymnastics with conjuring up a tree of inserts from individual fields.
afde19d
to
8f7178c
Compare
What is 'DNER'? |
It's a shorthand for |
Will assume the OSX x64 timeout is not related (I've seen it on quite a few other PRs as well recently). @dotnet/jit-contrib, @tannergooding - fixing one of the asserts that are currently showing up in SPMI in CI (and locally). |
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.
Thanks for the fix.
As the added comment states, we must do this to get dependent promotion, as otherwise the compiler does not support independent promotion of structs not fully eliminated from the IR, save some special cases (such as multi-reg structs).
We will only need to do this very rarely, as otherwise we mark SIMD locals used by SIMDs/HWIs specially when creating those nodes so as not to promote them.
This issue was revealed by forward substituion, where we had:
And forward-substituted the promoted "b" into "HWI". Notably, by the time we are forward-substituting, "we cannot really do anything about it", as struct promotion has already run, and by the time we get to morph, we too cannot do much save some gymnastics with conjuring up a tree of inserts from individual fields.
No diffs.
Side note: this is a conservative solution, but is always correct and does not put us in a worse position CQ-wise. It is possible/probable that we can "do better" at marking SIMD locals as "no promotion", or abandoning promotion for them altogether, but that is out of scope for this assert fix.