JIT: Avoid reordering operands in fgMorphModToSubMulDiv (#71615)

* JIT: Avoid reordering operands in fgMorphModToSubMulDiv

fgMorphModToSubMulDiv tries to check if it is ok to "clone" locals
instead of spilling them by checking for address exposure. This was
necessary because GTF_GLOB_REF is not up-to-date in preorder during
morph, which is when this runs. However, address exposure is not valid
for implicit byrefs at this point, so the check is not enough.

The logic is trying to figure out which of the operands need to be
spilled and which of them can be cloned. However, the logic was also
wrong in the face of potential embedded assignments. Thus, rework it to
be a bit more conservative but correct.

As part of this, remove fgIsSafeToClone and make fgMakeMultiUse less
conservative by avoiding checking for address exposure. This was
probably an attempt at some limited interference checks, but from what I
could see other uses do not need this.

Fix #65118

* Add a test

* Revert unnecessary change

* Switch to gtCloneExpr

Author: jakobbotsch

src/coreclr/jit/compiler.h

@@ -5574,7 +5574,6 @@ class Compiler
     //                  Create a new temporary variable to hold the result of *ppTree,
     //                  and transform the graph accordingly.
     GenTree* fgInsertCommaFormTemp(GenTree** ppTree, CORINFO_CLASS_HANDLE structType = nullptr);
-    bool fgIsSafeToClone(GenTree* tree);
     TempInfo fgMakeTemp(GenTree* rhs, CORINFO_CLASS_HANDLE structType = nullptr);
     GenTree* fgMakeMultiUse(GenTree** ppTree, CORINFO_CLASS_HANDLE structType = nullptr);
 

src/coreclr/jit/morph.cpp

@@ -1785,39 +1785,6 @@ void CallArgs::SetNeedsTemp(CallArg* arg)
     m_needsTemps   = true;
 }
 
-//------------------------------------------------------------------------------
-// fgIsSafeToClone: If the node is an unaliased local or constant,
-//                  then it is safe to clone.
-//
-// Arguments:
-//    tree - The node to check if it is safe to clone.
-//
-// Return Value:
-//    True if the tree is cloneable. False if the tree is not cloneable.
-//
-// Notes:
-//    This is conservative as this will return False if the local's address
-//    is exposed.
-//
-bool Compiler::fgIsSafeToClone(GenTree* tree)
-{
-    if (tree->IsInvariant())
-    {
-        return true;
-    }
-    else if (tree->IsLocal())
-    {
-        // Can't rely on GTF_GLOB_REF here.
-        //
-        if (!lvaGetDesc(tree->AsLclVarCommon())->IsAddressExposed())
-        {
-            return true;
-        }
-    }
-
-    return false;
-}
-
 //------------------------------------------------------------------------------
 // fgMakeTemp: Make a temp variable with a right-hand side expression as the assignment.
 //
@@ -1865,18 +1832,18 @@ TempInfo Compiler::fgMakeTemp(GenTree* rhs, CORINFO_CLASS_HANDLE structType /*=
 //    A fresh GT_LCL_VAR node referencing the temp which has not been used
 //
 // Notes:
-//    Caller must ensure that if the node is an unaliased local, the second use this
-//    creates will be evaluated before the local can be reassigned.
-//
-//    Can be safely called in morph preorder, before GTF_GLOB_REF is reliable.
+//    This function will clone invariant nodes and locals, so this function
+//    should only be used in situations where no interference between the
+//    original use and new use is possible. Otherwise, fgInsertCommaFormTemp
+//    should be used directly.
 //
 GenTree* Compiler::fgMakeMultiUse(GenTree** pOp, CORINFO_CLASS_HANDLE structType /*= nullptr*/)
 {
     GenTree* const tree = *pOp;
 
-    if (fgIsSafeToClone(tree))
+    if (tree->IsInvariant() || tree->OperIsLocal())
     {
-        return gtClone(tree);
+        return gtCloneExpr(tree);
     }
 
     return fgInsertCommaFormTemp(pOp, structType);
@@ -13816,20 +13783,43 @@ GenTree* Compiler::fgMorphModToSubMulDiv(GenTreeOp* tree)
 
     GenTreeOp* const div = tree;
 
+    assert(!div->IsReverseOp());
+
     GenTree* dividend = div->gtGetOp1();
     GenTree* divisor  = div->gtGetOp2();
 
-    TempInfo tempInfos[2]{};
+    TempInfo tempInfos[2];
     int      tempInfoCount = 0;
 
-    if (!fgIsSafeToClone(dividend))
+    // This transform runs in pre-morph so we cannot rely on GTF_GLOB_REF.
+    // Furthermore, this logic is somewhat complicated since the divisor and
+    // dividend are arbitrary nodes. For instance, if we spill the divisor and
+    // the dividend is a local, we need to spill the dividend too unless the
+    // divisor could not cause it to be reassigned.
+    // This could be slightly better via GTF_CALL and GTF_ASG checks on the
+    // divisor but the diffs of this were minor and the extra complexity seemed
+    // not worth it.
+    bool spillDividend;
+    bool spillDivisor;
+    if (divisor->IsInvariant() || divisor->OperIsLocal())
+    {
+        spillDivisor  = false;
+        spillDividend = !dividend->IsInvariant() && !dividend->OperIsLocal();
+    }
+    else
+    {
+        spillDivisor  = true;
+        spillDividend = !dividend->IsInvariant();
+    }
+
+    if (spillDividend)
     {
         tempInfos[tempInfoCount] = fgMakeTemp(dividend);
         dividend                 = tempInfos[tempInfoCount].load;
         tempInfoCount++;
     }
 
-    if (!fgIsSafeToClone(divisor))
+    if (spillDivisor)
     {
         tempInfos[tempInfoCount] = fgMakeTemp(divisor);
         divisor                  = tempInfos[tempInfoCount].load;

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

@@ -0,0 +1,58 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+// Generated by Fuzzlyn v1.5 on 2022-07-03 14:40:52
+// Run on Arm64 MacOS
+// Seed: 5960657379714964908
+// Reduced from 328.5 KiB to 0.8 KiB in 00:02:08
+// Debug: Outputs 0
+// Release: Outputs 1
+using System.Runtime.CompilerServices;
+
+public struct S0
+{
+    public long F0;
+    public long F1;
+    public S0(long f1): this()
+    {
+        F1 = f1;
+    }
+}
+
+public struct S1
+{
+    public uint F1;
+    public ushort F2;
+    public S0 F4;
+    public S1(ulong f5): this()
+    {
+    }
+
+    public long M82(ref short[] arg0)
+    {
+        Runtime_71600.s_13 = this.F1++;
+        S1 var1 = new S1(0);
+        return Runtime_71600.s_39[0].F1;
+    }
+}
+
+public class Runtime_71600
+{
+    public static uint s_13;
+    public static short[] s_28;
+    public static S0[] s_39;
+    public static int Main()
+    {
+        S0[] vr2 = new S0[]{new S0(-1)};
+        s_39 = vr2;
+        S1 vr3 = default(S1);
+        return M80(vr3) == 0 ? 100 : -1;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    public static int M80(S1 arg0)
+    {
+        arg0.F2 += (ushort)(arg0.F1 % (uint)arg0.M82(ref s_28));
+        return arg0.F2;
+    }
+}

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

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