Fix Assertion failed 'ins == INS_add' for madd/msub (#61171)

Co-authored-by: SingleAccretion <[email protected]>
dotnet · Nov 4, 2021 · 1902063 · 1902063
1 parent cebe877
commit 1902063
Show file tree

Hide file tree

Showing 8 changed files with 100 additions and 38 deletions.
diff --git a/src/coreclr/jit/codegen.h b/src/coreclr/jit/codegen.h
@@ -1248,6 +1248,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
     void genCodeForJumpTrue(GenTreeOp* jtrue);
 #ifdef TARGET_ARM64
     void genCodeForJumpCompare(GenTreeOp* tree);
+    void genCodeForMadd(GenTreeOp* tree);
     void genCodeForBfiz(GenTreeOp* tree);
 #endif // TARGET_ARM64
 

diff --git a/src/coreclr/jit/codegenarm64.cpp b/src/coreclr/jit/codegenarm64.cpp
@@ -9564,6 +9564,51 @@ void CodeGen::instGen_MemoryBarrier(BarrierKind barrierKind)
     }
 }
 
+//-----------------------------------------------------------------------------------
+// genCodeForMadd: Emit a madd/msub (Multiply-Add) instruction
+//
+// Arguments:
+//     tree - GT_MADD tree where op1 or op2 is GT_ADD
+//
+void CodeGen::genCodeForMadd(GenTreeOp* tree)
+{
+    assert(tree->OperIs(GT_MADD) && varTypeIsIntegral(tree) && !(tree->gtFlags & GTF_SET_FLAGS));
+    genConsumeOperands(tree);
+
+    GenTree* a;
+    GenTree* b;
+    GenTree* c;
+    if (tree->gtGetOp1()->OperIs(GT_MUL) && tree->gtGetOp1()->isContained())
+    {
+        a = tree->gtGetOp1()->gtGetOp1();
+        b = tree->gtGetOp1()->gtGetOp2();
+        c = tree->gtGetOp2();
+    }
+    else
+    {
+        assert(tree->gtGetOp2()->OperIs(GT_MUL) && tree->gtGetOp2()->isContained());
+        a = tree->gtGetOp2()->gtGetOp1();
+        b = tree->gtGetOp2()->gtGetOp2();
+        c = tree->gtGetOp1();
+    }
+
+    bool useMsub = false;
+    if (a->OperIs(GT_NEG) && a->isContained())
+    {
+        a       = a->gtGetOp1();
+        useMsub = true;
+    }
+    if (b->OperIs(GT_NEG) && b->isContained())
+    {
+        b       = b->gtGetOp1();
+        useMsub = !useMsub; // it's either "a * -b" or "-a * -b" which is the same as "a * b"
+    }
+
+    GetEmitter()->emitIns_R_R_R_R(useMsub ? INS_msub : INS_madd, emitActualTypeSize(tree), tree->GetRegNum(),
+                                  a->GetRegNum(), b->GetRegNum(), c->GetRegNum());
+    genProduceReg(tree);
+}
+
 //------------------------------------------------------------------------
 // genCodeForBfiz: Generates the code sequence for a GenTree node that
 // represents a bitfield insert in zero with sign/zero extension.
@@ -9587,6 +9632,7 @@ void CodeGen::genCodeForBfiz(GenTreeOp* tree)
     const bool isUnsigned = cast->IsUnsigned() || varTypeIsUnsigned(cast->CastToType());
     GetEmitter()->emitIns_R_R_I_I(isUnsigned ? INS_ubfiz : INS_sbfiz, size, tree->GetRegNum(), castOp->GetRegNum(),
                                   (int)shiftByImm, (int)srcBits);
+
     genProduceReg(tree);
 }
 

diff --git a/src/coreclr/jit/codegenarmarch.cpp b/src/coreclr/jit/codegenarmarch.cpp
@@ -300,6 +300,9 @@ void CodeGen::genCodeForTreeNode(GenTree* treeNode)
             break;
 
 #ifdef TARGET_ARM64
+        case GT_MADD:
+            genCodeForMadd(treeNode->AsOp());
+            break;
 
         case GT_INC_SATURATE:
             genCodeForIncSaturate(treeNode);

diff --git a/src/coreclr/jit/emitarm64.cpp b/src/coreclr/jit/emitarm64.cpp
@@ -13597,44 +13597,6 @@ regNumber emitter::emitInsTernary(instruction ins, emitAttr attr, GenTree* dst,
         // src2 can only be a reg
         assert(!src2->isContained());
     }
-    else if ((src1->OperIs(GT_MUL) && src1->isContained()) || (src2->OperIs(GT_MUL) && src2->isContained()))
-    {
-        assert(ins == INS_add);
-
-        GenTree* mul;
-        GenTree* c;
-        if (src1->OperIs(GT_MUL))
-        {
-            mul = src1;
-            c   = src2;
-        }
-        else
-        {
-            mul = src2;
-            c   = src1;
-        }
-
-        GenTree* a = mul->gtGetOp1();
-        GenTree* b = mul->gtGetOp2();
-
-        assert(varTypeIsIntegral(mul) && !mul->gtOverflow());
-
-        bool msub = false;
-        if (a->OperIs(GT_NEG) && a->isContained())
-        {
-            a    = a->gtGetOp1();
-            msub = true;
-        }
-        if (b->OperIs(GT_NEG) && b->isContained())
-        {
-            b    = b->gtGetOp1();
-            msub = !msub; // it's either "a * -b" or "-a * -b" which is the same as "a * b"
-        }
-
-        emitIns_R_R_R_R(msub ? INS_msub : INS_madd, attr, dst->GetRegNum(), a->GetRegNum(), b->GetRegNum(),
-                        c->GetRegNum());
-        return dst->GetRegNum();
-    }
     else // not floating point
     {
         // src2 can be immed or reg

diff --git a/src/coreclr/jit/gtlist.h b/src/coreclr/jit/gtlist.h
@@ -280,6 +280,11 @@ GTNODE(PHI_ARG          , GenTreePhiArg      ,0,(GTK_LEAF|GTK_LOCAL))     // phi
 //  Nodes used by Lower to generate a closer CPU representation of other nodes
 //-----------------------------------------------------------------------------
 
+#ifdef TARGET_ARM64
+GTNODE(MADD             , GenTreeOp          ,0, GTK_BINOP)                // Generates the Multiply-Add instruction (madd/msub)
+                                                                           // In future, we might consider enabling it for both armarch and xarch
+                                                                           // for floating-point MADD "unsafe" math
+#endif
 GTNODE(JMPTABLE         , GenTree            ,0, (GTK_LEAF|GTK_NOCONTAIN)) // Generates the jump table for switches
 GTNODE(SWITCH_TABLE     , GenTreeOp          ,0, (GTK_BINOP|GTK_NOVALUE))  // Jump Table based switch construct
 #ifdef TARGET_ARM64

diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp
@@ -1605,6 +1605,7 @@ void Lowering::ContainCheckBinary(GenTreeOp* node)
             }
             // If both 'a' and 'b' are GT_NEG - MADD will be emitted.
 
+            node->ChangeOper(GT_MADD);
             MakeSrcContained(node, mul);
         }
     }

diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_61037/Runtime_61037.cs b/src/tests/JIT/Regression/JitBlue/Runtime_61037/Runtime_61037.cs
@@ -0,0 +1,34 @@
+// 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 2021-11-03 12:55:21
+// Run on Arm64 Windows
+// Seed: 951014135056301943
+// Reduced from 152.5 KiB to 0.3 KiB in 00:01:22
+// Hits JIT assert in Release:
+// Assertion failed 'ins == INS_add' in 'Program:Main(Fuzzlyn.ExecutionServer.IRuntime)' during 'Generate code' (IL size 28)
+// 
+//     File: D:\a\_work\3\s\src\coreclr\jit\emitarm64.cpp Line: 13602
+// 
+public class C0
+{
+}
+
+public class Runtime_61037
+{
+    public static int Main()
+    {
+        if (0 == (27452 + (-2147483647 * M1())))
+        {
+            var vr3 = new C0();
+        }
+
+        return 100;
+    }
+
+    public static long M1()
+    {
+        var vr1 = new C0[,] { { new C0() } };
+        return 0;
+    }
+}
diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_61037/Runtime_61037.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_61037/Runtime_61037.csproj
@@ -0,0 +1,10 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <DebugType>None</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>