From d05415704826f0bd084d2c27f99e4dbe74f62464 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 18 Sep 2023 11:14:49 +0200 Subject: [PATCH] JIT: Extract all side effects of the index in optRemoveRangeCheck (#92116) optRemoveRangeCheck extracts only GTF_ASG from the bounds check. If the BOUNDS_CHECK is complex, that results in silently dropping side effects on the floor (see the example case). The ideal fix is that we should always extract all side effects from the index and length operands, however this has large regressions because the length typically has an ARR_LENGTH that we then extract. This PR instead has a surgical fix for the problem case that can be backported to .NET 8. It extracts all side effects from the index, but keeps extracting only GTF_ASG from the length to get around the issue mentioned above. Fix #91862 --- src/coreclr/jit/optimizer.cpp | 10 +++- .../JitBlue/Runtime_91576/Runtime_91576.cs | 2 +- .../JitBlue/Runtime_91855/Runtime_91855.cs | 2 +- .../JitBlue/Runtime_91862/Runtime_91862.cs | 46 +++++++++++++++++++ .../Runtime_91862/Runtime_91862.csproj | 8 ++++ 5 files changed, 64 insertions(+), 4 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_91862/Runtime_91862.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_91862/Runtime_91862.csproj diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index d5240152c26118..a583db4b3562c4 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -8888,9 +8888,15 @@ GenTree* Compiler::optRemoveRangeCheck(GenTreeBoundsChk* check, GenTree* comma, } #endif - // Extract side effects + // TODO-Bug: We really should be extracting all side effects from the + // length and index here, but the length typically involves a GT_ARR_LENGTH + // that we would preserve. Usually, as part of proving that the range check + // passes, we have also proven that the ARR_LENGTH is non-faulting. We need + // a good way to communicate to this function that it is ok to ignore side + // effects of the ARR_LENGTH. GenTree* sideEffList = nullptr; - gtExtractSideEffList(check, &sideEffList, GTF_ASG); + gtExtractSideEffList(check->GetArrayLength(), &sideEffList, GTF_ASG); + gtExtractSideEffList(check->GetIndex(), &sideEffList); if (sideEffList != nullptr) { diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_91576/Runtime_91576.cs b/src/tests/JIT/Regression/JitBlue/Runtime_91576/Runtime_91576.cs index 4df185d99ab241..23ae4b41c76a28 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_91576/Runtime_91576.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_91576/Runtime_91576.cs @@ -1,5 +1,5 @@ // Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license.aa +// The .NET Foundation licenses this file to you under the MIT license. // Generated by Fuzzlyn v1.6 on 2023-09-03 15:59:01 // Run on X64 Windows diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_91855/Runtime_91855.cs b/src/tests/JIT/Regression/JitBlue/Runtime_91855/Runtime_91855.cs index 6990d188b488c1..922cec127abc19 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_91855/Runtime_91855.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_91855/Runtime_91855.cs @@ -1,5 +1,5 @@ // Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license.aa +// The .NET Foundation licenses this file to you under the MIT license. using System; using System.Runtime.CompilerServices; diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_91862/Runtime_91862.cs b/src/tests/JIT/Regression/JitBlue/Runtime_91862/Runtime_91862.cs new file mode 100644 index 00000000000000..541e2658f2d6a3 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_91862/Runtime_91862.cs @@ -0,0 +1,46 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.CompilerServices; +using System.Runtime.Intrinsics; +using Xunit; + +public class Runtime_91862 +{ + [Fact] + public static int TestEntryPoint() + { + return Foo(default); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int Foo(Vector128 v) + { + int result = 101; + // This tree results in a BOUNDS_CHECK for Bar(...) & 3 + float x = Vector128.GetElement(v, Bar(ref result) & 3); + + if (result != 100) + { + Console.WriteLine("FAIL"); + } + + // After inlining x is DCE'd, which will extract side effects of its assignment above. + // That results IR amenable to forward sub, and we end up with a BOUNDS_CHECK + // with a complex index expression that we can still prove is within bounds. + Baz(x); + return result; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int Bar(ref int result) + { + result = 100; + return 0; + } + + private static void Baz(float x) + { + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_91862/Runtime_91862.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_91862/Runtime_91862.csproj new file mode 100644 index 00000000000000..de6d5e08882e86 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_91862/Runtime_91862.csproj @@ -0,0 +1,8 @@ + + + True + + + + +