Skip to content

Commit

Permalink
JIT: Disallow separate CSE candidates for struct GT_COMMAs (#106387)
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 #106380
  • Loading branch information
jakobbotsch authored Aug 14, 2024
1 parent fe0bad4 commit 0fbd814
Show file tree
Hide file tree
Showing 3 changed files with 58 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
40 changes: 40 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_106380/Runtime_106380.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

// Found by Antigen
// Reduced from 24.98 KB to 779 B.
using System.Runtime.Intrinsics;
using System.Runtime.Intrinsics.Arm;
using Xunit;

public class Runtime_106380
{
static Vector64<uint> s_v64_uint_23 = Vector64<uint>.AllBitsSet;
Vector64<uint> v64_uint_74 = Vector64.Create((uint)2, 2);
private void Method0()
{
unchecked
{
AdvSimd.MaxPairwise(v64_uint_74 += Vector64.ConditionalSelect(Vector64<uint>.Zero, s_v64_uint_23, s_v64_uint_23) & v64_uint_74, v64_uint_74 += (s_v64_uint_23 ^ v64_uint_74)& (s_v64_uint_23 = v64_uint_74));
return;
}
}
[Fact]
public static void TestEntryPoint()
{
if (AdvSimd.IsSupported)
{
new Runtime_106380().Method0();
}
}
}

/*
Environment:
set DOTNET_TieredCompilation=0
Assert failure(PID 13404 [0x0000345c], Thread: 5136 [0x1410]): Assertion failed 'type != TYP_VOID' in 'TestClass:Method0():this' during 'Optimize Valnum CSEs' (IL size 111; hash 0x46e9aa75; FullOpts)
File: C:\wk\runtime\src\coreclr\jit\gentree.cpp:8388
Image: C:\aman\Core_Root\corerun.exe
*/
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>

0 comments on commit 0fbd814

Please sign in to comment.