-
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
Delete CLS_VAR
#68524
Delete CLS_VAR
#68524
Conversation
ApplySelectorsTypeCheck already adds the cast in case it is necessary, and VN does support small-typed operands.
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsTODO: description.
|
6dbc72e
to
f292aff
Compare
@dotnet/jit-contrib |
@@ -8742,7 +8654,17 @@ void Compiler::fgValueNumberTree(GenTree* tree) | |||
} | |||
else if (isVolatile) | |||
{ | |||
// For Volatile indirection, mutate GcHeap/ByrefExposed | |||
// We just mutate GcHeap/ByrefExposed if isVolatile is true, and then do the read as normal. |
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.
The comment is unfortunately not quite correct anymore, we don't actually do "read as normal" on this path; I will adjust it in the future VN changes (it is a very nice comment overall).
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.
Happy to see the IR getting more regular.
The final change in the
CLS_VAR
deletion story. With this, we will now have a regular shape for "simple" static fields on all platforms, simplifying the IR and ensuring unified handling.We have some (overall positive) code diffs:
We also have some TP diffs, positive on all platforms but ARM itself, where we see a 0.1%-0.3% regression (presumably due to the enlarged IR). I think that's ok - the improvements made in #68383 should make the overall diff small.
SPMI in CI.