Skip to content
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

JIT: Update GTF_GLOB_REF after last-use copy omission #81430

Merged
merged 2 commits into from
Feb 1, 2023

Conversation

jakobbotsch
Copy link
Member

Morph is responsible for marking accesses of address-exposed locals with GTF_GLOB_REF, however the last-use copy elision can newly address expose a preexisting local. This means previous uses that morph already saw have not had GTF_GLOB_REF added. This updates the JIT to at least add GTF_GLOB_REF on locals within the same statement as the call. This is not a complete fix as nothing prevents downstream phases from reordering the call with uses from earlier statements, but this at least fixes the one issue we know of right now (which is itself stress-only).

Note that the previous uses are not actually 'global' uses unless they actually end up getting reordered with the call.

A more complete fix will likely entail moving the identifying of last-use candidates earlier and address exposing them at that point. However, that first requires ABI determination to be moved earlier, so this depends on #76926, which I expect to get to within the near future. So this is sort of an interim fix.

Fix #81049

Morph is responsible for marking accesses of address-exposed locals with
GTF_GLOB_REF, however the last-use copy elision can newly address expose
a preexisting local. This means previous uses that morph already saw
have not had GTF_GLOB_REF added. This updates the JIT to at least add
GTF_GLOB_REF on locals within the same statement as the call. This is
not a complete fix as nothing prevents downstream phases from reordering
the call with uses from earlier statements, but this at least fixes the
one issue we know of right now (which is itself stress-only).

A more complete fix will likely entail moving the identifying of
last-use candidates earlier and address exposing them at that point.
However, that first requires ABI determination to be moved earlier, so
this depends on dotnet#76926, which I expect to get to within the near future.

Fix dotnet#81049
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 31, 2023
@ghost ghost assigned jakobbotsch Jan 31, 2023
@ghost
Copy link

ghost commented Jan 31, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

Morph is responsible for marking accesses of address-exposed locals with GTF_GLOB_REF, however the last-use copy elision can newly address expose a preexisting local. This means previous uses that morph already saw have not had GTF_GLOB_REF added. This updates the JIT to at least add GTF_GLOB_REF on locals within the same statement as the call. This is not a complete fix as nothing prevents downstream phases from reordering the call with uses from earlier statements, but this at least fixes the one issue we know of right now (which is itself stress-only).

Note that the previous uses are not actually 'global' uses unless they actually end up getting reordered with the call.

A more complete fix will likely entail moving the identifying of last-use candidates earlier and address exposing them at that point. However, that first requires ABI determination to be moved earlier, so this depends on #76926, which I expect to get to within the near future. So this is sort of an interim fix.

Fix #81049

Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

The current statement might not be a valid tree during morphing of a
leaf node, so remark the global uses after we are done morphing instead.
@jakobbotsch jakobbotsch marked this pull request as ready for review February 1, 2023 16:08
@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @AndyAyersMS

I'm not so happy with this fix, but I think as an interim fix until I get to #76926 it should be ok.

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a reasonable interim fix.

@jakobbotsch jakobbotsch merged commit 58176f5 into dotnet:main Feb 1, 2023
@jakobbotsch jakobbotsch deleted the fix-81049 branch February 1, 2023 21:34
@jakobbotsch
Copy link
Member Author

No diffs

@ghost ghost locked as resolved and limited conversation to collaborators Mar 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Random jit stress failures in JIT\Directed\arglist\vararg
2 participants