-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 the zero-offset field sequence map #71455
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsIn #68712 I promised that we'd delete the zero-offset field sequences. This change delivers on that promise. There are still two scenarios we have where the root field sequence will have a zero offset:
Due to the fact the former cases are rare, this change represents them as The latter cases are not handled in this change to minimize diffs. It is likely that, if/when I get to supporting them, an explicit annotation node will be used (similar to We have some diffs due to the usual consequences of more precise VNs (they are no longer "polluted" by zero-offset sequences.
|
ca99fdd
to
45aa921
Compare
@dotnet/jit-contrib |
Leftover from GT_INDEX.
For statics at a zero offset, preserve the "CNS_INT 0" node instead.
This avoids regressing cases where we now have new CSE opportunities for statics with static init helper calls.
Replace with using "nullptr" directly. Note that "nullptr" can mean both an absense of a field sequence and an "implicit" sequence of struct fields.
Now that we don't need the map to be a set, we can apply some optimizations.
We don't expect empty sequences: they are either coming from the "IsFieldAddr" code path, which disallows these, or from the boxed static code path, where we should always have them.
45aa921
to
ff1089a
Compare
ff1089a
to
bdbaf25
Compare
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.
Love it! I will trigger some stress jobs.
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
Azure Pipelines successfully started running 2 pipeline(s). |
Most of the stress failures are #71748, but the arm32 libraries-jitstress failures look related to this or one of the recent changes: Assert failure(PID 61 [0x0000003d], Thread: 74 [0x004a]): Assertion failed 'OperIs(GT_LCL_VAR, GT_LCL_VAR_ADDR, GT_STORE_LCL_VAR)' in 'System.Buffers.Binary.Tests.BinaryReaderUnitTests:SpanWriteAndReadBigEndianHeterogeneousStruct():this' during 'Lowering nodeinfo' (IL size 1323; hash 0x710360ec; FullOpts)
File: /__w/1/s/src/coreclr/jit/gtstructs.h Line: 65
Image: /root/helix/work/correlation/dotnet |
Taking a look. |
In #68712 I promised that we'd delete the zero-offset field sequences. This change (finally, one could say) delivers on that promise.
There are still two scenarios where the root field sequence will have a zero offset:
Due to the fact the former cases are rare, this change represents them as
ADD(baseAddr, 0 <field seq>)
, which requires minimal extra handling (just one special case in morph).The latter cases are not handled in this change to minimize diffs. It is likely that, if/when I get to supporting them, an explicit annotation node will be used (similar to
ARR_ADDR
), however, for now, there is no need for it.We have some diffs due to the usual consequences of more precise VNs (they are no longer "polluted" by zero-offset sequences.