JIT: Extract all side effects of the index in optRemoveRangeCheck (#9…

…2210)

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

Co-authored-by: Jakob Botsch Nielsen <jakob.botsch.nielsen@gmail.com>

src/coreclr/jit/optimizer.cpp

@@ -8875,9 +8875,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)
     {

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

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<float> 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)
+    {
+    }
+}

src/tests/JIT/Regression/JitBlue/Runtime_91862/Runtime_91862.csproj

@@ -0,0 +1,8 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>