[JIT] - Fixed sub-optimal optimization for a % 1 to Zero

src/coreclr/jit/compiler.h

@@ -5797,6 +5797,7 @@ class Compiler
     GenTree* fgOptimizeBitwiseXor(GenTreeOp* xorOp);
     GenTree* fgPropagateCommaThrow(GenTree* parent, GenTreeOp* commaThrow, GenTreeFlags precedingSideEffects);
     GenTree* fgMorphRetInd(GenTreeUnOp* tree);
+    GenTree* fgMorphModToZero(GenTreeOp* tree);
     GenTree* fgMorphModToSubMulDiv(GenTreeOp* tree);
     GenTree* fgMorphUModToAndSub(GenTreeOp* tree);
     GenTree* fgMorphSmpOpOptional(GenTreeOp* tree, bool* optAssertionPropDone);

src/coreclr/jit/morph.cpp

@@ -9869,20 +9869,13 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA
 
         ASSIGN_HELPER_FOR_MOD:
 
-            // For "val % 1", return 0 if op1 doesn't have any side effects
-            // and we are not in the CSE phase, we cannot discard 'tree'
-            // because it may contain CSE expressions that we haven't yet examined.
-            //
-            if (((op1->gtFlags & GTF_SIDE_EFFECT) == 0) && !optValnumCSE_phase)
+            if (!optValnumCSE_phase)
             {
-                if (op2->IsIntegralConst(1))
+                if (tree->OperIs(GT_MOD, GT_UMOD) && (op2->IsIntegralConst(1) || op2->IsIntegralConst(-1)))
                 {
-                    GenTree* zeroNode = gtNewZeroConNode(typ);
-#ifdef DEBUG
-                    zeroNode->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED;
-#endif
-                    DEBUG_DESTROY_NODE(tree);
-                    return zeroNode;
+                    // Transformation: a % 1 = 0
+                    // Transformation: a % -1 = 0
+                    return fgMorphModToZero(tree->AsOp());
                 }
             }
 
@@ -9909,20 +9902,17 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA
             }
 #endif
 #endif // !TARGET_64BIT
-
             if (!optValnumCSE_phase)
             {
                 if (tree->OperIs(GT_UMOD) && op2->IsIntegralConstUnsignedPow2())
                 {
                     // Transformation: a % b = a & (b - 1);
-                    tree = fgMorphUModToAndSub(tree->AsOp());
-                    op1  = tree->AsOp()->gtOp1;
-                    op2  = tree->AsOp()->gtOp2;
+                    return fgMorphUModToAndSub(tree->AsOp());
                 }
 #ifdef TARGET_ARM64
                 // ARM64 architecture manual suggests this transformation
                 // for the mod operator.
-                else
+                else if (tree->OperIs(GT_MOD, GT_UMOD))
 #else
                 // XARCH only applies this transformation if we know
                 // that magic division will be used - which is determined
@@ -9933,9 +9923,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA
 #endif
                 {
                     // Transformation: a % b = a - (a / b) * b;
-                    tree = fgMorphModToSubMulDiv(tree->AsOp());
-                    op1  = tree->AsOp()->gtOp1;
-                    op2  = tree->AsOp()->gtOp2;
+                    return fgMorphModToSubMulDiv(tree->AsOp());
                 }
             }
             break;
@@ -13160,6 +13148,57 @@ GenTree* Compiler::fgMorphMultiOp(GenTreeMultiOp* multiOp)
 }
 #endif // defined(FEATURE_SIMD) || defined(FEATURE_HW_INTRINSICS)
 
+//------------------------------------------------------------------------
+// fgMorphModToZero: Transform 'a % 1' or 'a % -1' into the equivalent '0'.
+//
+// Arguments:
+//    tree - The GT_MOD/GT_UMOD tree to morph
+//
+// Returns:
+//    The morphed tree, will be a GT_COMMA or a zero constant node.
+//
+GenTree* Compiler::fgMorphModToZero(GenTreeOp* tree)
+{
+    JITDUMP("\nMorphing MOD/UMOD [%06u] to Zero\n", dspTreeID(tree));
+
+    assert(tree->OperIs(GT_MOD, GT_UMOD));
+    assert(tree->gtOp2->IsIntegralConst(1) || tree->gtOp2->IsIntegralConst(-1));
+
+    GenTree* op1 = tree->gtGetOp1();
+    GenTree* op2 = tree->gtGetOp2();
+
+    const var_types type = tree->TypeGet();
+
+    GenTree* zero = gtNewZeroConNode(type);
+
+    GenTree* op1SideEffects = nullptr;
+    gtExtractSideEffList(op1, &op1SideEffects, GTF_ALL_EFFECT);
+    if (op1SideEffects != nullptr)
+    {
+        GenTree* comma = gtNewOperNode(GT_COMMA, type, op1SideEffects, zero);
+
+#ifdef DEBUG
+        // op1 may already have been morphed, so unset this bit.
+        op1SideEffects->gtDebugFlags &= ~GTF_DEBUG_NODE_MORPHED;
+#endif // DEBUG
+
+        DEBUG_DESTROY_NODE(tree->gtOp2);
+        DEBUG_DESTROY_NODE(tree);
+
+        return fgMorphTree(comma);
+    }
+    else
+    {
+        INDEBUG(zero->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);
+
+        DEBUG_DESTROY_NODE(tree->gtOp2);
+        DEBUG_DESTROY_NODE(tree->gtOp1);
+        DEBUG_DESTROY_NODE(tree);
+
+        return zero;
+    }
+}
+
 //------------------------------------------------------------------------
 // fgMorphModToSubMulDiv: Transform a % b into the equivalent a - (a / b) * b
 // (see ECMA III 3.55 and III.3.56).
@@ -13282,13 +13321,9 @@ GenTree* Compiler::fgMorphModToSubMulDiv(GenTreeOp* tree)
         result = gtNewOperNode(GT_COMMA, type, tempInfos[i].asg, result);
     }
 
-#ifdef DEBUG
-    result->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED;
-#endif
-
     div->CheckDivideByConstOptimized(this);
 
-    return result;
+    return fgMorphTree(result);
 }
 
 //------------------------------------------------------------------------
@@ -13318,12 +13353,10 @@ GenTree* Compiler::fgMorphUModToAndSub(GenTreeOp* tree)
     const size_t   cnsValue = (static_cast<size_t>(tree->gtOp2->AsIntConCommon()->IntegralValue())) - 1;
     GenTree* const newTree  = gtNewOperNode(GT_AND, type, tree->gtOp1, gtNewIconNode(cnsValue, type));
 
-    INDEBUG(newTree->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);
-
     DEBUG_DESTROY_NODE(tree->gtOp2);
     DEBUG_DESTROY_NODE(tree);
 
-    return newTree;
+    return fgMorphTree(newTree);
 }
 
 //------------------------------------------------------------------------------

src/tests/JIT/opt/Remainder/IntRemainder.cs

@@ -0,0 +1,74 @@
+// 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 CodeGenTests
+{
+    static class IntRemainder
+    {
+        static int _fieldValue = 123;
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        static int Int32_RemainderByOne()
+        {
+            // X64-FULL-LINE:      call CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
+            // X64-FULL-LINE-NEXT: xor [[REG0:[a-z]+]], [[REG0]]
+
+            // ARM64-FULL-LINE:      bl CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
+            // ARM64-FULL-LINE-NEXT: mov [[REG0:[a-z0-9]+]], wzr
+
+            return _fieldValue % 1;
+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        static int Int32_RemainderByOneWithValue(int value)
+        {
+            // X64-FULL-LINE: xor [[REG0:[a-z]+]], [[REG0]]
+
+            // ARM64-FULL-LINE: mov [[REG0:[a-z0-9]+]], wzr
+
+            return value % 1;
+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        static int Int32_RemainderByNegativeOne()
+        {
+            // X64-FULL-LINE:      call CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
+            // X64-FULL-LINE-NEXT: xor [[REG0:[a-z]+]], [[REG0]]
+
+            // ARM64-FULL-LINE:      bl CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
+            // ARM64-FULL-LINE-NEXT: mov [[REG0:[a-z0-9]+]], wzr
+
+            return _fieldValue % -1;
+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        static int Int32_RemainderByNegativeOneWithValue(int value)
+        {
+            // X64-FULL-LINE: xor [[REG0:[a-z]+]], [[REG0]]
+
+            // ARM64-FULL-LINE: mov [[REG0:[a-z0-9]+]], wzr
+
+            return value % -1;
+        }
+
+        static int Main()
+        {
+            if (Int32_RemainderByOne() != 0)
+                return 0;
+
+            if (Int32_RemainderByOneWithValue(-123) != 0)
+                return 0;
+
+            if (Int32_RemainderByNegativeOne() != 0)
+                return 0;
+
+            if (Int32_RemainderByNegativeOneWithValue(-123) != 0)
+                return 0;
+
+            return 100;
+        }
+    }
+}

src/tests/JIT/opt/Remainder/IntRemainder.csproj

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

src/tests/JIT/opt/Remainder/Regressions/Regression1/Regression1.cs

@@ -0,0 +1,19 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+public class Program
+{
+    public static ulong[,] s_1;
+    public static int Main()
+    {
+        // This should not assert.
+        try
+        {
+            ushort vr10 = default(ushort);
+            bool vr11 = 0 < ((s_1[0, 0] * (uint)(0 / vr10)) % 1);
+        }
+        catch {}
+
+        return 100;
+    }
+}

src/tests/JIT/opt/Remainder/Regressions/Regression1/Regression1.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>