JIT: Set GTF_ORDER_SIDEEFF for some nodes with invisible dependencies (…

…#78698)

We have a few places where we create the pattern "COMMA(some check, some value)". In some of these cases there may not be any visible dependency (e.g. use of a defined value) which makes the dependency invisible to the JIT.

If the value is safe to compute only because of the check (for example, a bounds check + indexing operation), and if the value otherwise has no side effects, then nothing prevented the backend or optimizations from reordering these nodes.

A particular problem we may have is around array indexing and bounds checks. Creating an arbitrary illegal byref is not allowed, but to the JIT this node is just a normal node that is completely free of any side effects. Before this change nothing was preventing us from reordering the bounds checks with the computation of the array element.

Fix #78554

Co-authored-by: Alan Hayward <alan.hayward@arm.com>

src/coreclr/jit/importer.cpp

@@ -10462,7 +10462,15 @@ void Compiler::impImportBlockCode(BasicBlock* block)
                                 GenTree* boxPayloadAddress =
                                     gtNewOperNode(GT_ADD, TYP_BYREF, cloneOperand, boxPayloadOffset);
                                 GenTree* nullcheck = gtNewNullCheck(op1, block);
-                                GenTree* result    = gtNewOperNode(GT_COMMA, TYP_BYREF, nullcheck, boxPayloadAddress);
+                                // Add an ordering dependency between the null
+                                // check and forming the byref; the JIT assumes
+                                // in many places that the only legal null
+                                // byref is literally 0, and since the byref
+                                // leaks out here, we need to ensure it is
+                                // nullchecked.
+                                nullcheck->gtFlags |= GTF_ORDER_SIDEEFF;
+                                boxPayloadAddress->gtFlags |= GTF_ORDER_SIDEEFF;
+                                GenTree* result = gtNewOperNode(GT_COMMA, TYP_BYREF, nullcheck, boxPayloadAddress);
                                 impPushOnStack(result, tiRetVal);
                                 break;
                             }

src/coreclr/jit/importercalls.cpp

@@ -2728,6 +2728,11 @@ GenTree* Compiler::impIntrinsic(GenTree*                newobjThis,
                 // Prepare result
                 var_types resultType = JITtype2varType(sig->retType);
                 assert(resultType == result->TypeGet());
+                // Add an ordering dependency between the bounds check and
+                // forming the byref to prevent these from being reordered. The
+                // JIT is not allowed to create arbitrary illegal byrefs.
+                boundsCheck->gtFlags |= GTF_ORDER_SIDEEFF;
+                result->gtFlags |= GTF_ORDER_SIDEEFF;
                 retNode = gtNewOperNode(GT_COMMA, resultType, boundsCheck, result);
 
                 break;

src/coreclr/jit/morph.cpp

@@ -4608,6 +4608,13 @@ GenTree* Compiler::fgMorphIndexAddr(GenTreeIndexAddr* indexAddr)
     // Prepend the bounds check and the assignment trees that were created (if any).
     if (boundsCheck != nullptr)
     {
+        // This is changing a value dependency (INDEX_ADDR node) into a flow
+        // dependency, so make sure this dependency remains visible. Also, the
+        // JIT is not allowed to create arbitrary byrefs, so we must make sure
+        // the address is not reordered with the bounds check.
+        boundsCheck->gtFlags |= GTF_ORDER_SIDEEFF;
+        addr->gtFlags |= GTF_ORDER_SIDEEFF;
+
         tree = gtNewOperNode(GT_COMMA, tree->TypeGet(), boundsCheck, tree);
         fgSetRngChkTarget(boundsCheck);
     }
@@ -5166,6 +5173,8 @@ GenTree* Compiler::fgMorphExpandInstanceField(GenTree* tree, MorphAddrContext* m
         GenTree* lclVar  = gtNewLclvNode(lclNum, objRefType);
         GenTree* nullchk = gtNewNullCheck(lclVar, compCurBB);
 
+        nullchk->gtFlags |= GTF_ORDER_SIDEEFF;
+
         if (asg != nullptr)
         {
             // Create the "comma" node.
@@ -5177,6 +5186,10 @@ GenTree* Compiler::fgMorphExpandInstanceField(GenTree* tree, MorphAddrContext* m
         }
 
         addr = gtNewLclvNode(lclNum, objRefType); // Use "tmpLcl" to create "addr" node.
+
+        // Ensure the creation of the byref does not get reordered with the
+        // null check, as that could otherwise create an illegal byref.
+        addr->gtFlags |= GTF_ORDER_SIDEEFF;
     }
     else
     {
@@ -10479,7 +10492,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA
             // could result in an invalid value number for the newly generated GT_IND node.
             if ((op1->OperGet() == GT_COMMA) && fgGlobalMorph)
             {
-                // Perform the transform IND(COMMA(x, ..., z)) == COMMA(x, ..., IND(z)).
+                // Perform the transform IND(COMMA(x, ..., z)) -> COMMA(x, ..., IND(z)).
                 // TBD: this transformation is currently necessary for correctness -- it might
                 // be good to analyze the failures that result if we don't do this, and fix them
                 // in other ways.  Ideally, this should be optional.
@@ -10512,9 +10525,12 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA
                 // TODO-1stClassStructs: we often create a struct IND without a handle, fix it.
                 op1 = gtNewIndir(typ, addr);
 
-                // Determine flags on the indir.
+                // GTF_GLOB_EFFECT flags can be recomputed from the child
+                // nodes. GTF_ORDER_SIDEEFF may be set already and indicate no
+                // reordering is allowed with sibling nodes, so we cannot
+                // recompute that.
                 //
-                op1->gtFlags |= treeFlags & ~GTF_ALL_EFFECT;
+                op1->gtFlags |= treeFlags & ~GTF_GLOB_EFFECT;
                 op1->gtFlags |= (addr->gtFlags & GTF_ALL_EFFECT);
 
                 // if this was a non-faulting indir, clear GTF_EXCEPT,

src/coreclr/jit/optimizer.cpp

@@ -4794,6 +4794,17 @@ bool Compiler::optIfConvert(BasicBlock* block)
         return false;
     }
 
+    // Evaluating op1/op2 unconditionally effectively has the same effect as
+    // reordering them with the condition (for example, the condition could be
+    // an explicit bounds check and the operand could read an array element).
+    // Disallow this except for some common cases that we know are always side
+    // effect free.
+    if (((cond->gtFlags & GTF_ORDER_SIDEEFF) != 0) && !asgNode->gtGetOp2()->IsInvariant() &&
+        !asgNode->gtGetOp2()->OperIsLocal())
+    {
+        return false;
+    }
+
 #ifdef DEBUG
     if (verbose)
     {

src/tests/JIT/Regression/JitBlue/Runtime_78554/Runtime_78554.cs

@@ -0,0 +1,29 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.Runtime.CompilerServices;
+
+public class Runtime_78554
+{
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static void Consume(uint op)
+    {
+    }
+
+    [MethodImplAttribute(MethodImplOptions.NoInlining)]
+    static void ArrayIndexConsume(uint[] a, uint i)
+    {
+        if (i < a.Length)
+        {
+           i = a[i];
+        }
+        Consume(i);
+    }
+
+    public static int Main()
+    {
+        var arr = new uint[] { 1, 42, 3000 };
+        ArrayIndexConsume(arr, 0xffffffff);
+        return 100;
+    }
+}

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

@@ -0,0 +1,13 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+
+    <CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="0" />
+    <CLRTestEnvironmentVariable Include="DOTNET_JITMinOpts" Value="0" />
+    <CLRTestEnvironmentVariable Include="DOTNET_JitStressModeNames" Value="STRESS_IF_CONVERSION_COST" />
+  </ItemGroup>
+</Project>