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: Disallow separate CSE candidates for struct GT_COMMAs #106387

Merged
merged 3 commits into from
Aug 14, 2024

Conversation

jakobbotsch
Copy link
Member

CSE normally creates candidates based on the normal VN of trees.
However, there is an exception for GT_COMMA nodes, where the
GT_COMMA itself creates a candidate with its op1 exceptions, while the
op2 then creates the usual "normal VN" candidate.

This can be problematic for TYP_STRUCT typed trees. In the JIT we do
not have a first class representation for TYP_STRUCT temporaries,
meaning that these always are restricted in what their immediate user
is. gtNewTempStore thus needs special logic to keep this invariant
satisfied; one of those special cases is that for GT_COMMA, instead of
creating the store with the comma as the source, we sink the store into
the op2 recursively so that we end up with the store immediately next
to the node that produces the struct value. This is ok since we are
storing to a new local anyway, so there can't be interference with the
op1's of the commas we skipped into.

This, however, causes problems for CSE with the GT_COMMA special case
above. If we end up CSE'ing the outer comma we will create a definition
that is sunk into op2 of the comma. If that op2 is itself as COMMA
that was a CSE candidate, then once we get to that candidate it no
longer has an IR shape that produces a value, and we thus cannot create
a CSE definition. The fix is to avoid creating separate CSE candidates
for struct-typed commas.

Fix #106380

#91586 is a better long term solution. I think to make that happen we should allow struct temporaries in HIR/LIR and have lowering figure out how to insert proper locals when it sees interference for struct LIR edges. That should be much better since there are many varTypeIsStruct(x) trees that actually do have first class handling in the backend, for example multi-reg and SIMD nodes.

CSE normally creates candidates based on the normal VN of trees.
However, there is an exception for `GT_COMMA` nodes, where the
`GT_COMMA` itself creates a candidate with its op1 exceptions, while the
op2 then creates the usual "normal VN" candidate.

This can be problematic for `TYP_STRUCT` typed trees. In the JIT we do
not have a first class representation for `TYP_STRUCT` temporaries,
meaning that these always are restricted in what their immediate user
is. `gtNewTempStore` thus needs special logic to keep this invariant
satisfied; one of those special cases is that for `GT_COMMA`, instead of
creating the store with the comma as the source, we sink the store into
the `op2` recursively so that we end up with the store immediately next
to the node that produces the struct value. This is ok since we are
storing to a new local anyway, so there can't be interference with the
op1's of the commas we skipped into.

This, however, causes problems for CSE with the `GT_COMMA` special case
above. If we end up CSE'ing the outer comma we will create a definition
that is sunk into `op2` of the comma. If that `op2` is itself as `COMMA`
that was a CSE candidate, then once we get to that candidate it no
longer has an IR shape that produces a value, and we thus cannot create
a CSE definition. The fix is to avoid creating separate CSE candidates
for struct-typed commas.

Fix dotnet#106380
@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 Aug 14, 2024
@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @AndyAyersMS

Minor diffs

@jakobbotsch jakobbotsch added this to the 9.0.0 milestone Aug 14, 2024
@jakobbotsch jakobbotsch merged commit 0fbd814 into dotnet:main Aug 14, 2024
110 of 114 checks passed
@jakobbotsch jakobbotsch deleted the fix-106380 branch August 14, 2024 16:26
@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2024
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.

JIT: Assertion failed 'type != TYP_VOID' during 'Optimize Valnum CSEs'
2 participants