Forbid fgMorphCommutative during optValnumCSE_phase

src/coreclr/jit/morph.cpp

@@ -10962,11 +10962,26 @@ GenTree* Compiler::fgMorphCommutative(GenTreeOp* tree)
     genTreeOps oper = tree->OperGet();
 
     if (!op1->OperIs(oper) || !tree->gtGetOp2()->IsCnsIntOrI() || !op1->gtGetOp2()->IsCnsIntOrI() ||
-        op1->gtGetOp1()->IsCnsIntOrI() || gtIsActiveCSE_Candidate(op1))
+        op1->gtGetOp1()->IsCnsIntOrI())
     {
         return nullptr;
     }
 
+    if (!fgGlobalMorph && (op1 != tree->gtGetOp1()))
+    {
+        // Since 'tree->gtGetOp1()' can have complex structure (e.g. COMMA(..(COMMA(..,op1)))
+        // don't run the optimization for such trees outside of global morph.
+        // Otherwise, there is a chance of violating VNs invariants and/or modifying a tree
+        // that is an active CSE candidate.
+        return nullptr;
+    }
+
+    if (gtIsActiveCSE_Candidate(tree) || gtIsActiveCSE_Candidate(op1))
+    {
+        // The optimization removes 'tree' from IR and changes the value of 'op1'.
+        return nullptr;
+    }
+
     if (tree->OperMayOverflow() && (tree->gtOverflow() || op1->gtOverflow()))
     {
         return nullptr;
@@ -10980,26 +10995,41 @@ GenTree* Compiler::fgMorphCommutative(GenTreeOp* tree)
         return nullptr;
     }
 
-    GenTree* foldedCns = gtFoldExprConst(gtNewOperNode(oper, cns1->TypeGet(), cns1, cns2));
-    if (!foldedCns->IsCnsIntOrI())
+    if (gtIsActiveCSE_Candidate(cns1) || gtIsActiveCSE_Candidate(cns2))
+    {
+        // The optimization removes 'cns2' from IR and changes the value of 'cns1'.
+        return nullptr;
+    }
+
+    GenTree* folded = gtFoldExprConst(gtNewOperNode(oper, cns1->TypeGet(), cns1, cns2));
+
+    if (!folded->IsCnsIntOrI())
     {
         // Give up if we can't fold "C1 op C2"
         return nullptr;
     }
 
-    cns1->gtIconVal = foldedCns->AsIntCon()->IconValue();
-    if ((oper == GT_ADD) && foldedCns->IsCnsIntOrI())
+    auto foldedCns = folded->AsIntCon();
+
+    cns1->SetIconValue(foldedCns->IconValue());
+    cns1->SetVNsFromNode(foldedCns);
+
+    if (oper == GT_ADD)
     {
-        cns1->AsIntCon()->gtFieldSeq =
-            GetFieldSeqStore()->Append(cns1->AsIntCon()->gtFieldSeq, cns2->AsIntCon()->gtFieldSeq);
+        // Note that gtFoldExprConst doesn't maintain fieldSeq when folding constant
+        // trees of TYP_LONG.
+        cns1->gtFieldSeq = GetFieldSeqStore()->Append(cns1->gtFieldSeq, cns2->gtFieldSeq);
     }
 
-    GenTreeOp* newTree = tree->gtGetOp1()->AsOp();
+    op1 = tree->gtGetOp1();
+    op1->SetVNsFromNode(tree);
+
     DEBUG_DESTROY_NODE(tree);
     DEBUG_DESTROY_NODE(cns2);
     DEBUG_DESTROY_NODE(foldedCns);
-    INDEBUG(newTree->gtOp2->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);
-    return newTree;
+    INDEBUG(cns1->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);
+
+    return op1;
 }
 
 /*****************************************************************************

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

@@ -0,0 +1,51 @@
+// 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;
+
+namespace Runtime_56935
+{
+    public class Program
+    {
+        static int clsFld;
+
+        public static int Main()
+        {
+            int zeroVal = 0;
+
+            // The rhs of the following statement on Arm64 will be transformed into the following tree
+            //
+            // N012 ( 27, 14) [000009] ---X--------  *  ADD       int    $42
+            // N010 ( 25, 11) [000029] ---X--------  +--*  ADD       int    $40
+            // N008 ( 23,  8) [000032] ---X--------  |  +--*  NEG       int    $41
+            // N007 ( 22,  7) [000008] ---X--------  |  |  \--*  DIV       int    $42
+            // N003 (  1,  2) [000004] ------------  |  |     +--*  CNS_INT   int    1 $42
+            // N006 (  1,  2) [000024] ------------  |  |     \--*  COMMA     int    $42
+            // N004 (  0,  0) [000022] ------------  |  |        +--*  NOP       void   $100
+            // N005 (  1,  2) [000023] ------------  |  |        \--*  CNS_INT   int    1 $42
+            // N009 (  1,  2) [000028] ------------  |  \--*  CNS_INT   int    1 $42
+            // N011 (  1,  2) [000003] ------------  \--*  CNS_INT   int    1 $42
+            //
+            // Then, during optValnumCSE() the tree is transformed even further by fgMorphCommutative()
+            //
+            // N014 ( 25, 11) [000029] ---X--------  *  ADD       int    $40
+            // N012 ( 23,  8) [000032] ---X--------  +--*  NEG       int    $41
+            // N011 ( 22,  7) [000008] ---X--------  |  \--*  DIV       int    $42
+            // N007 (  1,  2) [000004] ------------  |     +--*  CNS_INT   int    1 $42
+            // N010 (  1,  2) [000024] ------------  |     \--*  COMMA     int    $42
+            // N008 (  0,  0) [000022] ------------  |        +--*  NOP       void   $100
+            // N009 (  1,  2) [000023] ------------  |        \--*  CNS_INT   int    1 $42
+            // N013 (  1,  2) [000028] ------------  \--*  CNS_INT   int    2 $42
+            //
+            // The issue is that VN for [000028] has not been updated ($42 corresponds to CnsInt(1)).
+            // As a result, during optVNAssertionPropCurStmt() the whole tree is **incorrecly** folded to
+            //
+            // After constant propagation on [000029]:
+            // N007 (  1,  2) [000040] ------------  *  CNS_INT   int    0 $40
+
+            clsFld = 1 + (1 % (zeroVal + 1));
+            return (clsFld == 1) ? 100 : 0;
+        }
+    }
+}

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

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