[RyuJit/ARM] Fix lsra BuildShiftRotate (#17103)

* add repro

* delete BuildShiftRotate for arm

* fix GT_LSH_HI and GT_RSH_LO

* return the special handling for GT_LSH_HI and GT_RSH_LO

* fix the header

diff --git a/src/jit/lsra.h b/src/jit/lsra.h
index 62ba386..d3d3f99 100644 (file)
--- a/src/jit/lsra.h
+++ b/src/jit/lsra.h
@@ -1654,9 +1654,14 @@ private:
 
     void BuildStoreLoc(GenTree* tree);
     void BuildReturn(GenTree* tree);
+#ifdef _TARGET_XARCH_
     // This method, unlike the others, returns the number of sources, since it may be called when
     // 'tree' is contained.
     int BuildShiftRotate(GenTree* tree);
+#endif // _TARGET_XARCH_
+#ifdef _TARGET_ARM_
+    void BuildShiftLongCarry(GenTree* tree);
+#endif
     void BuildPutArgReg(GenTreeUnOp* node);
     void BuildCall(GenTreeCall* call);
     void BuildCmp(GenTree* tree);

diff --git a/src/jit/lsraarm.cpp b/src/jit/lsraarm.cpp
index df4a435..63e93fa 100644 (file)
--- a/src/jit/lsraarm.cpp
+++ b/src/jit/lsraarm.cpp
@@ -113,6 +113,46 @@ void LinearScan::BuildLclHeap(GenTree* tree)
 }
 
 //------------------------------------------------------------------------
+// BuildShiftLongCarry: Set the node info for GT_LSH_HI or GT_RSH_LO.
+//
+// Arguments:
+//    tree      - The node of interest
+//
+// Note: these operands have uses that interfere with the def and need the special handling.
+//
+void LinearScan::BuildShiftLongCarry(GenTree* tree)
+{
+    assert(tree->OperGet() == GT_LSH_HI || tree->OperGet() == GT_RSH_LO);
+
+    GenTree* source = tree->gtOp.gtOp1;
+    assert((source->OperGet() == GT_LONG) && source->isContained());
+
+    TreeNodeInfo* info = currentNodeInfo;
+    info->srcCount     = 2;
+
+    LocationInfoListNode* sourceLoInfo = getLocationInfo(source->gtOp.gtOp1);
+    LocationInfoListNode* sourceHiInfo = getLocationInfo(source->gtOp.gtOp2);
+    if (tree->OperGet() == GT_LSH_HI)
+    {
+        sourceLoInfo->info.isDelayFree = true;
+    }
+    else
+    {
+        sourceHiInfo->info.isDelayFree = true;
+    }
+    useList.Append(sourceLoInfo);
+    useList.Append(sourceHiInfo);
+    info->hasDelayFreeSrc = true;
+
+    GenTree* shiftBy = tree->gtOp.gtOp2;
+    if (!shiftBy->isContained())
+    {
+        appendLocationInfoToList(shiftBy);
+        info->srcCount += 1;
+    }
+}
+
+//------------------------------------------------------------------------
 // BuildNode: Set the register requirements for RA.
 //
 // Notes:
@@ -335,11 +375,22 @@ void LinearScan::BuildNode(GenTree* tree)
         case GT_AND:
         case GT_OR:
         case GT_XOR:
+        case GT_LSH:
+        case GT_RSH:
+        case GT_RSZ:
+        case GT_ROR:
             assert(info->dstCount == 1);
             info->srcCount = appendBinaryLocationInfoToList(tree->AsOp());
             assert(info->srcCount == (tree->gtOp.gtOp2->isContained() ? 1 : 2));
             break;
 
+        case GT_LSH_HI:
+        case GT_RSH_LO:
+            assert(info->dstCount == 1);
+            BuildShiftLongCarry(tree);
+            assert(info->srcCount == (tree->gtOp.gtOp2->isContained() ? 2 : 3));
+            break;
+
         case GT_RETURNTRAP:
             // this just turns into a compare of its child with an int
             // + a conditional call
@@ -553,15 +604,6 @@ void LinearScan::BuildNode(GenTree* tree)
             appendLocationInfoToList(tree->gtOp.gtOp1);
             break;
 
-        case GT_LSH:
-        case GT_RSH:
-        case GT_RSZ:
-        case GT_ROR:
-        case GT_LSH_HI:
-        case GT_RSH_LO:
-            BuildShiftRotate(tree);
-            break;
-
         case GT_EQ:
         case GT_NE:
         case GT_LT:

diff --git a/src/jit/lsraarm64.cpp b/src/jit/lsraarm64.cpp
index 6497ac8..6c1fc4a 100644 (file)
--- a/src/jit/lsraarm64.cpp
+++ b/src/jit/lsraarm64.cpp
@@ -219,6 +219,10 @@ void LinearScan::BuildNode(GenTree* tree)
         case GT_AND:
         case GT_OR:
         case GT_XOR:
+        case GT_LSH:
+        case GT_RSH:
+        case GT_RSZ:
+        case GT_ROR:
             info->srcCount = appendBinaryLocationInfoToList(tree->AsOp());
             assert(info->dstCount == 1);
             break;
@@ -341,13 +345,6 @@ void LinearScan::BuildNode(GenTree* tree)
             assert(info->dstCount == 1);
             break;
 
-        case GT_LSH:
-        case GT_RSH:
-        case GT_RSZ:
-        case GT_ROR:
-            BuildShiftRotate(tree);
-            break;
-
         case GT_EQ:
         case GT_NE:
         case GT_LT:

diff --git a/src/jit/lsraarmarch.cpp b/src/jit/lsraarmarch.cpp
index 72acf82..22c96f9 100644 (file)
--- a/src/jit/lsraarmarch.cpp
+++ b/src/jit/lsraarmarch.cpp
@@ -112,59 +112,6 @@ void LinearScan::BuildIndir(GenTreeIndir* indirTree)
 }
 
 //------------------------------------------------------------------------
-// BuildShiftRotate: Set the NodeInfo for a shift or rotate.
-//
-// Arguments:
-//    tree      - The node of interest
-//
-// Return Value:
-//    None.
-//
-int LinearScan::BuildShiftRotate(GenTree* tree)
-{
-    TreeNodeInfo* info    = currentNodeInfo;
-    GenTree*      source  = tree->gtOp.gtOp1;
-    GenTree*      shiftBy = tree->gtOp.gtOp2;
-    assert(info->dstCount == 1);
-    if (!shiftBy->isContained())
-    {
-        appendLocationInfoToList(shiftBy);
-        info->srcCount = 1;
-    }
-
-#ifdef _TARGET_ARM_
-
-    // The first operand of a GT_LSH_HI and GT_RSH_LO oper is a GT_LONG so that
-    // we can have a three operand form. Increment the srcCount.
-    if (tree->OperGet() == GT_LSH_HI || tree->OperGet() == GT_RSH_LO)
-    {
-        assert((source->OperGet() == GT_LONG) && source->isContained());
-        info->srcCount += 2;
-
-        LocationInfoListNode* sourceLoInfo = getLocationInfo(source->gtOp.gtOp1);
-        useList.Append(sourceLoInfo);
-        LocationInfoListNode* sourceHiInfo = getLocationInfo(source->gtOp.gtOp2);
-        useList.Append(sourceHiInfo);
-        if (tree->OperGet() == GT_LSH_HI)
-        {
-            sourceLoInfo->info.isDelayFree = true;
-        }
-        else
-        {
-            sourceHiInfo->info.isDelayFree = true;
-        }
-        info->hasDelayFreeSrc = true;
-    }
-    else
-#endif // _TARGET_ARM_
-    {
-        appendLocationInfoToList(source);
-        info->srcCount++;
-    }
-    return info->srcCount;
-}
-
-//------------------------------------------------------------------------
 // BuildCall: Set the NodeInfo for a call.
 //
 // Arguments:

diff --git a/tests/src/JIT/Regression/JitBlue/DevDiv_545497/DevDiv_545497.il b/tests/src/JIT/Regression/JitBlue/DevDiv_545497/DevDiv_545497.il
new file mode 100644 (file)
index 0000000..fa6bf49
--- /dev/null
+++ b/tests/src/JIT/Regression/JitBlue/DevDiv_545497/DevDiv_545497.il
@@ -0,0 +1,117 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+// The bug that this test captures was a case where arm LSRA built wrong uses order for
+// the IL_0090 shift operand.
+
+.assembly extern System.Runtime { auto }
+.assembly DevDiv_524309 {}
+
+.class private auto ansi beforefieldinit DevDiv_545497
+       extends [System.Runtime]System.Object
+{
+  .method private hidebysig static uint8 
+          ILGEN_METHOD(float32 a,
+                       uint32 b,
+                       uint64 c) cil managed
+  {
+       .maxstack  194
+       .locals init (float64, bool, unsigned int64)
+       IL_0000: ldloc.s 0x01
+       IL_0002: conv.r.un
+       IL_0028: conv.ovf.u8.un
+       IL_002c: ldloc.s 0x00
+       IL_002e: conv.ovf.i8.un
+       IL_002f: stloc 0x0002
+       IL_0033: ldarg 0x0002
+       IL_003f: ldloc 0x0001
+       IL_0043: ldloc.s 0x01
+       IL_0045: shl
+       IL_0047: shr.un
+       IL_0049: ldarg.s 0x01
+       IL_005b: conv.i1
+       IL_005d: conv.ovf.u
+       IL_005e: ldloc 0x0001
+       IL_0063: ldc.i4 0
+       IL_0071: ldloc 0x0002
+       IL_007a: conv.ovf.i1.un
+       IL_007b: stloc 0x0001
+       IL_007f: conv.i1
+       IL_0081: rem.un
+       IL_0082: ldarg 0x0002
+       IL_0087: dup
+       IL_0088: add.ovf.un
+       IL_008a: ldloc.s 0x01
+       IL_008c: ldloc 0x0001
+       IL_0090: shr           // This shift moves loc0x01 >> loc0x01 and exposed the original issue.
+       IL_0091: conv.ovf.i8
+       IL_0092: mul
+       IL_0093: ldloc.s 0x01
+       IL_0095: ldarg.s 0x01
+       IL_0097: div.un
+       IL_0098: starg 0x0001
+       IL_009c: ldloc 0x0002
+       IL_00b0: clt
+       IL_00b2: conv.ovf.i1.un
+       IL_00b3: sub.ovf.un
+       IL_00b7: sub.ovf
+       IL_00b9: shr.un
+       IL_00ba: ldloc.s 0x01
+       IL_00bc: conv.ovf.u8.un
+       IL_00bd: and
+       IL_00bf: clt.un
+       IL_00c1: ret
+  } // end of method DevDiv_545497::ILGEN_METHOD
+
+  .method private hidebysig static int32 
+          Main() cil managed
+  {
+    .entrypoint
+    // Code size       31 (0x1f)
+    .maxstack  3
+    .locals init (int32 V_0)
+    IL_0000:  nop
+    .try
+    {
+      IL_0001:  nop
+      IL_0002:  ldc.r4     0.0
+      IL_0007:  ldc.i4.0
+      IL_0008:  ldc.i4.0
+      IL_0009:  conv.i8
+      IL_000a:  call       uint8 DevDiv_545497::ILGEN_METHOD(float32,
+                                                             uint32,
+                                                             uint64)
+      IL_000f:  pop
+      IL_0010:  nop
+      IL_0011:  leave.s    IL_0018
+
+    }  // end .try
+    catch [System.Runtime]System.Object 
+    {
+      IL_0013:  pop
+      IL_0014:  nop
+      IL_0015:  nop
+      IL_0016:  leave.s    IL_0018
+
+    }  // end handler
+    IL_0018:  ldc.i4.s   100
+    IL_001a:  stloc.0
+    IL_001b:  br.s       IL_001d
+
+    IL_001d:  ldloc.0
+    IL_001e:  ret
+  } // end of method DevDiv_545497::Main
+
+  .method public hidebysig specialname rtspecialname 
+          instance void  .ctor() cil managed
+  {
+    // Code size       8 (0x8)
+    .maxstack  8
+    IL_0000:  ldarg.0
+    IL_0001:  call       instance void [System.Runtime]System.Object::.ctor()
+    IL_0006:  nop
+    IL_0007:  ret
+  } // end of method DevDiv_545497::.ctor
+
+} // end of class DevDiv_545497

diff --git a/tests/src/JIT/Regression/JitBlue/DevDiv_545497/DevDiv_545497.ilproj b/tests/src/JIT/Regression/JitBlue/DevDiv_545497/DevDiv_545497.ilproj
new file mode 100644 (file)
index 0000000..5934cf6
--- /dev/null
+++ b/tests/src/JIT/Regression/JitBlue/DevDiv_545497/DevDiv_545497.ilproj
@@ -0,0 +1,23 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <PropertyGroup>
+    <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
+    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
+    <AssemblyName>$(MSBuildProjectName)</AssemblyName>
+    <ProjectGuid>{95DFC527-4DC1-495E-97D7-E94EE1F7140D}</ProjectGuid>
+    <OutputType>Exe</OutputType>
+    <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+  </PropertyGroup>
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' "></PropertyGroup>
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' "></PropertyGroup>
+  <PropertyGroup>
+    <DebugType>None</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).il" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "></PropertyGroup>
+</Project>