Kill RCX when used by shift

RCX must be explicitly killed. Otherwise, if there's a case of a def/use conflict - as in this test case where the shift amount is defined by a divide that must go in RAX, it won't be explicitly assigned to RCX,.
Also, the handling of conflicts must not use the register assignment of the def on the use  if it conflicts with the use register requirements, and vice versa.

Fix dotnet/coreclr#18884

Commit migrated from https://github.com/dotnet/coreclr/commit/f48801ea7c1412e228a5230ec6cbeabdf5277ec0

diff --git a/src/coreclr/src/jit/lsra.h b/src/coreclr/src/jit/lsra.h
index f88f2d9..eddeee9 100644 (file)
--- a/src/coreclr/src/jit/lsra.h
+++ b/src/coreclr/src/jit/lsra.h
@@ -1031,6 +1031,7 @@ private:
 
     // Helpers for getKillSetForNode().
     regMaskTP getKillSetForStoreInd(GenTreeStoreInd* tree);
+    regMaskTP getKillSetForShiftRotate(GenTreeOp* tree);
     regMaskTP getKillSetForMul(GenTreeOp* tree);
     regMaskTP getKillSetForCall(GenTreeCall* call);
     regMaskTP getKillSetForModDiv(GenTreeOp* tree);

diff --git a/src/coreclr/src/jit/lsrabuild.cpp b/src/coreclr/src/jit/lsrabuild.cpp
index 3c2a266..f42486d 100644 (file)
--- a/src/coreclr/src/jit/lsrabuild.cpp
+++ b/src/coreclr/src/jit/lsrabuild.cpp
@@ -250,8 +250,8 @@ void LinearScan::resolveConflictingDefAndUse(Interval* interval, RefPosition* de
     RegRecord*   useRegRecord     = nullptr;
     regNumber    defReg           = REG_NA;
     regNumber    useReg           = REG_NA;
-    bool         defRegConflict   = false;
-    bool         useRegConflict   = false;
+    bool         defRegConflict   = ((defRegAssignment & useRegAssignment) == RBM_NONE);
+    bool         useRegConflict   = defRegConflict;
 
     // If the useRefPosition is a "delayRegFree", we can't change the registerAssignment
     // on it, or we will fail to ensure that the fixedReg is busy at the time the target
@@ -263,7 +263,7 @@ void LinearScan::resolveConflictingDefAndUse(Interval* interval, RefPosition* de
     {
         INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_FIXED_DELAY_USE));
     }
-    if (defRefPosition->isFixedRegRef)
+    if (defRefPosition->isFixedRegRef && !defRegConflict)
     {
         defReg       = defRefPosition->assignedReg();
         defRegRecord = getRegisterRecord(defReg);
@@ -287,7 +287,7 @@ void LinearScan::resolveConflictingDefAndUse(Interval* interval, RefPosition* de
             }
         }
     }
-    if (useRefPosition->isFixedRegRef)
+    if (useRefPosition->isFixedRegRef && !useRegConflict)
     {
         useReg                               = useRefPosition->assignedReg();
         useRegRecord                         = getRegisterRecord(useReg);
@@ -768,6 +768,28 @@ regMaskTP LinearScan::getKillSetForStoreInd(GenTreeStoreInd* tree)
 }
 
 //------------------------------------------------------------------------
+// getKillSetForShiftRotate: Determine the liveness kill set for a shift or rotate node.
+//
+// Arguments:
+//    shiftNode - the shift or rotate node
+//
+// Return Value:    a register mask of the registers killed
+//
+regMaskTP LinearScan::getKillSetForShiftRotate(GenTreeOp* shiftNode)
+{
+    regMaskTP killMask = RBM_NONE;
+#ifdef _TARGET_XARCH_
+    assert(shiftNode->OperIsShiftOrRotate());
+    GenTree* shiftBy = shiftNode->gtGetOp2();
+    if (!shiftBy->isContained())
+    {
+        killMask = RBM_RCX;
+    }
+#endif // _TARGET_XARCH_
+    return killMask;
+}
+
+//------------------------------------------------------------------------
 // getKillSetForMul: Determine the liveness kill set for a multiply node.
 //
 // Arguments:
@@ -1011,6 +1033,18 @@ regMaskTP LinearScan::getKillSetForNode(GenTree* tree)
     regMaskTP killMask = RBM_NONE;
     switch (tree->OperGet())
     {
+        case GT_LSH:
+        case GT_RSH:
+        case GT_RSZ:
+        case GT_ROL:
+        case GT_ROR:
+#ifdef _TARGET_X86_
+        case GT_LSH_HI:
+        case GT_RSH_LO:
+#endif
+            killMask = getKillSetForShiftRotate(tree->AsOp());
+            break;
+
         case GT_MUL:
         case GT_MULHI:
 #if !defined(_TARGET_64BIT_)

diff --git a/src/coreclr/src/jit/lsraxarch.cpp b/src/coreclr/src/jit/lsraxarch.cpp
index f5c4a03..1cb407b 100644 (file)
--- a/src/coreclr/src/jit/lsraxarch.cpp
+++ b/src/coreclr/src/jit/lsraxarch.cpp
@@ -964,6 +964,7 @@ int LinearScan::BuildShiftRotate(GenTree* tree)
         if (!shiftBy->isContained())
         {
             srcCount += BuildDelayFreeUses(shiftBy, RBM_RCX);
+            buildKillPositionsForNode(tree, currentLoc + 1, RBM_RCX);
         }
         BuildDef(tree, dstCandidates);
     }
@@ -972,6 +973,7 @@ int LinearScan::BuildShiftRotate(GenTree* tree)
         if (!shiftBy->isContained())
         {
             srcCount += BuildOperandUses(shiftBy, RBM_RCX);
+            buildKillPositionsForNode(tree, currentLoc + 1, RBM_RCX);
         }
     }
     return srcCount;

diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18884/GitHub_18884.cs b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18884/GitHub_18884.cs
new file mode 100644 (file)
index 0000000..1cd9085
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18884/GitHub_18884.cs
@@ -0,0 +1,82 @@
+// 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.
+
+// This bug had to do with the handling (reserving and killing) of RCX
+// for variable shift operations on X64.
+
+using System;
+using System.Collections.Specialized;
+using System.Runtime.CompilerServices;
+
+static class GitHub_18884
+{
+    static ushort s_3;
+    static long s_5;
+    static int returnVal = 100;
+
+    public static int Main()
+    {
+        s_3 = 0; // avoid runtime checks in M15
+        ReproWindows(0, 0, 1, 0);
+        ReproUx(0, 0, 1, 0);
+        Set_Mask_AllTest();
+        return returnVal;
+    }
+
+    static void ReproWindows(byte arg0, long arg1, ushort arg2, ulong arg3)
+    {
+        s_5 >>= 50 / arg2;  // the value shifted by here
+        if (arg0 != 0)
+        {
+            s_3 = s_3;
+        }
+
+        // Is in arg0 here
+        if (arg0 != 0)
+        {
+            Console.WriteLine("FAIL: ReproWindows");
+            returnVal = -1;
+        }
+    }
+    static void ReproUx(ulong arg0, long arg1, ushort arg2, byte arg3)
+    {
+        s_5 >>= 50 / arg2;  // the value shifted by here
+        if (arg3 != 0)
+        {
+            s_3 = s_3;
+        }
+
+        // Is in arg3 here
+        if (arg3 != 0)
+        {
+            Console.WriteLine("FAIL: ReproUx");
+            returnVal = -1;
+        }
+    }
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    public static void CheckValue(int value, int expectedValue)
+    {
+        if (value != expectedValue)
+        {
+            returnVal = -1;
+            Console.WriteLine("FAIL: Set_Mask_AllTest");
+        }
+    }
+
+    // While fixing the above failures, this test (from corefx) failed.
+    public static void Set_Mask_AllTest()
+    {
+        BitVector32 flip = new BitVector32();
+        int mask = 0;
+        for (int bit = 1; bit < 32 + 1; bit++)
+        {
+            mask = BitVector32.CreateMask(mask);
+            BitVector32 single = new BitVector32();
+            single[mask] = true;
+
+            // The bug was exposed by passing the result of a shift in RCX on x64/ux.
+            CheckValue(1 << (bit - 1), single.Data);
+        }
+    }
+}

diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18884/GitHub_18884.csproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18884/GitHub_18884.csproj
new file mode 100644 (file)
index 0000000..ea8e8a7
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18884/GitHub_18884.csproj
@@ -0,0 +1,45 @@
+<?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>
+    <SchemaVersion>2.0</SchemaVersion>
+    <ProjectGuid>{95DFC527-4DC1-495E-97D7-E94EE1F7140D}</ProjectGuid>
+    <OutputType>Exe</OutputType>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+  </PropertyGroup>
+  <!-- Default configurations to help VS understand the configurations -->
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' "></PropertyGroup>
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' "></PropertyGroup>
+  <ItemGroup>
+    <CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
+      <Visible>False</Visible>
+    </CodeAnalysisDependentAssemblyPaths>
+  </ItemGroup>
+  <PropertyGroup>
+    <DebugType></DebugType>
+    <Optimize>True</Optimize>
+    <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+  <PropertyGroup>
+    <CLRTestBatchPreCommands><![CDATA[
+$(CLRTestBatchPreCommands)
+set COMPlus_TailcallStress=1
+]]></CLRTestBatchPreCommands>
+  <BashCLRTestPreCommands><![CDATA[
+$(BashCLRTestPreCommands)
+export COMPlus_TailcallStress=1
+]]></BashCLRTestPreCommands>
+  </PropertyGroup>
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "></PropertyGroup>
+</Project>