Skip to content

Commit

Permalink
JIT: Disallow separate CSE candidates for struct GT_COMMAs
Browse files Browse the repository at this point in the history
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
  • Loading branch information
jakobbotsch committed Aug 14, 2024
1 parent 5cd69e4 commit 359d77b
Showing 1 changed file with 10 additions and 2 deletions.
12 changes: 10 additions & 2 deletions src/coreclr/jit/optcse.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -536,7 +536,15 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt)
// than the one from its op2. For this case we want to create two different
// CSE candidates. This allows us to CSE the GT_COMMA separately from its value.
//
if (tree->OperGet() == GT_COMMA)
// Even this exception has an exception: for struct typed GT_COMMAs we
// cannot allow the comma and op2 to be separate candidates as, if we
// decide to CSE both the comma and its op2, then creating the store with
// the comma will sink it into the op2, potentially breaking the op2 CSE
// definition if it itself is another comma. This restriction is related to
// the fact that we do not have af first class representation for struct
// temporaries in our IR.
//
if (tree->OperIs(GT_COMMA) && !varTypeIsStruct(tree))
{
// op2 is the value produced by a GT_COMMA
GenTree* op2 = tree->AsOp()->gtOp2;
Expand Down Expand Up @@ -588,7 +596,7 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt)
key = vnLibNorm;
}
}
else // Not a GT_COMMA or a GT_CNS_INT
else // Not a primitive GT_COMMA or a GT_CNS_INT
{
key = vnLibNorm;
}
Expand Down

0 comments on commit 359d77b

Please sign in to comment.