From 0fbd81404d1f211572387498474063bc6f407f0f Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 14 Aug 2024 18:26:51 +0200 Subject: [PATCH] JIT: Disallow separate CSE candidates for struct `GT_COMMA`s (#106387) 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 --- src/coreclr/jit/optcse.cpp | 12 +++++- .../JitBlue/Runtime_106380/Runtime_106380.cs | 40 +++++++++++++++++++ .../Runtime_106380/Runtime_106380.csproj | 8 ++++ 3 files changed, 58 insertions(+), 2 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_106380/Runtime_106380.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_106380/Runtime_106380.csproj diff --git a/src/coreclr/jit/optcse.cpp b/src/coreclr/jit/optcse.cpp index f633a5e3272bb..0d67ffd7a956a 100644 --- a/src/coreclr/jit/optcse.cpp +++ b/src/coreclr/jit/optcse.cpp @@ -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; @@ -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; } diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_106380/Runtime_106380.cs b/src/tests/JIT/Regression/JitBlue/Runtime_106380/Runtime_106380.cs new file mode 100644 index 0000000000000..5853038fc1df6 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_106380/Runtime_106380.cs @@ -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 s_v64_uint_23 = Vector64.AllBitsSet; + Vector64 v64_uint_74 = Vector64.Create((uint)2, 2); + private void Method0() + { + unchecked + { + AdvSimd.MaxPairwise(v64_uint_74 += Vector64.ConditionalSelect(Vector64.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 +*/ diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_106380/Runtime_106380.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_106380/Runtime_106380.csproj new file mode 100644 index 0000000000000..15edd99711a1a --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_106380/Runtime_106380.csproj @@ -0,0 +1,8 @@ + + + True + + + + + \ No newline at end of file