Fix 64 bit shift inconsistencies (on 32 bit targets)

Recent shift changes made the JIT_LLsh helper mask the shift count to 6 bits. The other 2 helpers (JIT_LRsh and JIT_LRsz) so now we get inconsistencies such as `(x >> 64) != (x << 64)`.

The ECMA spec says that "the return value is unspecified if shiftAmount is greater than or equal to the width of value" so the JIT has no obligation to implement a particular behavior. But it seems preferable to have all shift instructions behave similarly, it avoids complications and reduces risks.

This also changes `ValueNumStore::EvalOpIntegral` to mask the shift count for 64 bit shifts so it matches `gtFoldExprConst`. Otherwise the produced value depends on the C/C++ compiler's behavior.

diff --git a/src/jit/decomposelongs.cpp b/src/jit/decomposelongs.cpp
index 89092b0..7caae47 100644 (file)
--- a/src/jit/decomposelongs.cpp
+++ b/src/jit/decomposelongs.cpp
@@ -1087,7 +1087,9 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
     // If we are shifting by a constant int, we do not want to use a helper, instead, we decompose.
     if (shiftByOper == GT_CNS_INT)
     {
-        unsigned int count = shiftByOp->gtIntCon.gtIconVal;
+        // Reduce count modulo 64 to match behavior found in the shift helpers,
+        // Compiler::gtFoldExpr and ValueNumStore::EvalOpIntegral.
+        unsigned int count = shiftByOp->gtIntCon.gtIconVal & 0x3F;
         Range().Remove(shiftByOp);
 
         if (count == 0)
@@ -1112,24 +1114,6 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
         {
             case GT_LSH:
             {
-                // Reduce count modulo 64 to match behavior found in
-                // the LLSH helper and in gtFoldExpr.
-                count &= 0x3F;
-
-                // Retest for zero shift
-                if (count == 0)
-                {
-                    GenTree* next = shift->gtNext;
-                    // Remove shift and don't do anything else.
-                    if (shift->IsUnusedValue())
-                    {
-                        gtLong->SetUnusedValue();
-                    }
-                    Range().Remove(shift);
-                    use.ReplaceWith(m_compiler, gtLong);
-                    return next;
-                }
-
                 if (count < 32)
                 {
                     // For shifts of < 32 bits, we transform the code to:
@@ -1258,6 +1242,8 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
                 }
                 else
                 {
+                    assert(count >= 32 && count < 64);
+
                     // Since we're right shifting at least 32 bits, we can remove the lo part of the shifted value iff
                     // it has no side effects.
                     //
@@ -1272,45 +1258,19 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
                         loOp1->SetUnusedValue();
                     }
 
-                    assert(count >= 32);
-                    if (count < 64)
+                    if (count == 32)
                     {
-                        if (count == 32)
-                        {
-                            // Move hiOp1 into loResult.
-                            loResult = hiOp1;
-                        }
-                        else
-                        {
-                            assert(count > 32 && count < 64);
-
-                            // Move hiOp1 into loResult, do a GT_RSZ with count - 32.
-                            GenTree* shiftBy = m_compiler->gtNewIconNode(count - 32, TYP_INT);
-                            loResult         = m_compiler->gtNewOperNode(oper, TYP_INT, hiOp1, shiftBy);
-                            Range().InsertBefore(shift, shiftBy, loResult);
-                        }
+                        // Move hiOp1 into loResult.
+                        loResult = hiOp1;
                     }
                     else
                     {
-                        assert(count >= 64);
-
-                        // Since we're right shifting at least 64 bits, we can remove the hi part of the shifted value
-                        // iff it has no side effects.
-                        //
-                        // TODO-CQ: we could go perform this removal transitively (i.e. iteratively remove everything
-                        // that feeds the hi operand while there are no side effects)
-                        if ((hiOp1->gtFlags & GTF_ALL_EFFECT) == 0)
-                        {
-                            Range().Remove(hiOp1, true);
-                        }
-                        else
-                        {
-                            hiOp1->SetUnusedValue();
-                        }
-
-                        // Zero out lo
-                        loResult = m_compiler->gtNewZeroConNode(TYP_INT);
-                        Range().InsertBefore(shift, loResult);
+                        assert(count > 32 && count < 64);
+
+                        // Move hiOp1 into loResult, do a GT_RSZ with count - 32.
+                        GenTree* shiftBy = m_compiler->gtNewIconNode(count - 32, TYP_INT);
+                        loResult         = m_compiler->gtNewOperNode(oper, TYP_INT, hiOp1, shiftBy);
+                        Range().InsertBefore(shift, shiftBy, loResult);
                     }
 
                     // Zero out hi
@@ -1354,7 +1314,7 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
                 }
                 else
                 {
-                    assert(count >= 32);
+                    assert(count >= 32 && count < 64);
 
                     // Since we're right shifting at least 32 bits, we can remove the lo part of the shifted value iff
                     // it has no side effects.
@@ -1370,47 +1330,28 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
                         loOp1->SetUnusedValue();
                     }
 
-                    if (count < 64)
+                    if (count == 32)
                     {
-                        if (count == 32)
-                        {
-                            // Move hiOp1 into loResult.
-                            loResult = hiOp1;
-                            Range().InsertBefore(shift, loResult);
-                        }
-                        else
-                        {
-                            assert(count > 32 && count < 64);
-
-                            // Move hiOp1 into loResult, do a GT_RSH with count - 32.
-                            GenTree* shiftBy = m_compiler->gtNewIconNode(count - 32, TYP_INT);
-                            loResult         = m_compiler->gtNewOperNode(oper, TYP_INT, hiOp1, shiftBy);
-                            Range().InsertBefore(shift, hiOp1, shiftBy, loResult);
-                        }
-
-                        // Propagate sign bit in hiResult
-                        GenTree* shiftBy = m_compiler->gtNewIconNode(31, TYP_INT);
-                        hiResult         = m_compiler->gtNewOperNode(GT_RSH, TYP_INT, hiCopy, shiftBy);
-                        Range().InsertBefore(shift, shiftBy, hiCopy, hiResult);
-
-                        m_compiler->lvaIncRefCnts(hiCopy);
+                        // Move hiOp1 into loResult.
+                        loResult = hiOp1;
+                        Range().InsertBefore(shift, loResult);
                     }
                     else
                     {
-                        assert(count >= 64);
+                        assert(count > 32 && count < 64);
 
-                        // Propagate sign bit in loResult
-                        GenTree* loShiftBy = m_compiler->gtNewIconNode(31, TYP_INT);
-                        loResult           = m_compiler->gtNewOperNode(GT_RSH, TYP_INT, hiCopy, loShiftBy);
-                        Range().InsertBefore(shift, hiCopy, loShiftBy, loResult);
+                        // Move hiOp1 into loResult, do a GT_RSH with count - 32.
+                        GenTree* shiftBy = m_compiler->gtNewIconNode(count - 32, TYP_INT);
+                        loResult         = m_compiler->gtNewOperNode(oper, TYP_INT, hiOp1, shiftBy);
+                        Range().InsertBefore(shift, hiOp1, shiftBy, loResult);
+                    }
 
-                        // Propagate sign bit in hiResult
-                        GenTree* shiftBy = m_compiler->gtNewIconNode(31, TYP_INT);
-                        hiResult         = m_compiler->gtNewOperNode(GT_RSH, TYP_INT, hiOp1, shiftBy);
-                        Range().InsertBefore(shift, shiftBy, hiOp1, hiResult);
+                    // Propagate sign bit in hiResult
+                    GenTree* shiftBy = m_compiler->gtNewIconNode(31, TYP_INT);
+                    hiResult         = m_compiler->gtNewOperNode(GT_RSH, TYP_INT, hiCopy, shiftBy);
+                    Range().InsertBefore(shift, shiftBy, hiCopy, hiResult);
 
-                        m_compiler->lvaIncRefCnts(hiCopy);
-                    }
+                    m_compiler->lvaIncRefCnts(hiCopy);
                 }
 
                 insertAfter = hiResult;

diff --git a/src/jit/valuenum.cpp b/src/jit/valuenum.cpp
index 9ca852e..7cdbfdc 100644 (file)
--- a/src/jit/valuenum.cpp
+++ b/src/jit/valuenum.cpp
@@ -408,13 +408,27 @@ T ValueNumStore::EvalOpIntegral(VNFunc vnf, T v0, T v1, ValueNum* pExcSet)
         case GT_AND:
             return v0 & v1;
         case GT_LSH:
-            return v0 << v1;
+            if (sizeof(T) == 8)
+            {
+                return v0 << (v1 & 0x3F);
+            }
+            else
+            {
+                return v0 << v1;
+            }
         case GT_RSH:
-            return v0 >> v1;
+            if (sizeof(T) == 8)
+            {
+                return v0 >> (v1 & 0x3F);
+            }
+            else
+            {
+                return v0 >> v1;
+            }
         case GT_RSZ:
             if (sizeof(T) == 8)
             {
-                return UINT64(v0) >> v1;
+                return UINT64(v0) >> (v1 & 0x3F);
             }
             else
             {

diff --git a/src/vm/i386/jithelp.asm b/src/vm/i386/jithelp.asm
index a4bbe1c..27b8668 100644 (file)
--- a/src/vm/i386/jithelp.asm
+++ b/src/vm/i386/jithelp.asm
@@ -520,6 +520,8 @@ JIT_LLsh ENDP
         ALIGN 16
 PUBLIC JIT_LRsh
 JIT_LRsh PROC
+; Reduce shift amount mod 64
+        and     ecx, 63
 ; Handle shifts of between bits 0 and 31
         cmp     ecx, 32
         jae     short LRshMORE32
@@ -554,6 +556,8 @@ JIT_LRsh ENDP
         ALIGN 16
 PUBLIC JIT_LRsz
 JIT_LRsz PROC
+; Reduce shift amount mod 64
+        and     ecx, 63
 ; Handle shifts of between bits 0 and 31
         cmp     ecx, 32
         jae     short LRszMORE32

diff --git a/src/vm/jithelpers.cpp b/src/vm/jithelpers.cpp
index 9f887e6..e3d93d2 100644 (file)
--- a/src/vm/jithelpers.cpp
+++ b/src/vm/jithelpers.cpp
@@ -464,7 +464,7 @@ HCIMPLEND
 HCIMPL2_VV(INT64, JIT_LRsh, INT64 num, int shift)
 {
     FCALL_CONTRACT;
-    return num >> shift;
+    return num >> (shift & 0x3F);
 }
 HCIMPLEND
 
@@ -472,7 +472,7 @@ HCIMPLEND
 HCIMPL2_VV(UINT64, JIT_LRsz, UINT64 num, int shift)
 {
     FCALL_CONTRACT;
-    return num >> shift;
+    return num >> (shift & 0x3F);
 }
 HCIMPLEND
 #endif // !BIT64 && !_TARGET_X86_

diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_15949/GitHub_15949.il b/tests/src/JIT/Regression/JitBlue/GitHub_15949/GitHub_15949.il
new file mode 100644 (file)
index 0000000..34c07b1
--- /dev/null
+++ b/tests/src/JIT/Regression/JitBlue/GitHub_15949/GitHub_15949.il
@@ -0,0 +1,235 @@
+// 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.
+
+.assembly extern mscorlib { auto }
+.assembly extern System.Console { auto }
+.assembly GitHub_15949 { }
+
+// Ensure that shifting a 64 bit value by 64 bits produces the same value. The ECMA spec doesn't specify
+// what should happen in this case but it would be preferable to have the same behavior for all shift
+// instructions (use only the lowest 6 bits of the shift count).
+
+// This test is intended for 32 bit targets where RyuJIT has 4 different code paths that are of interest:
+// helper call, decomposition, constant folding via gtFoldExpr and constant folding via VN's EvalOpIntegral.
+// But it also works on the current 64 bit targets (ARM64 & x64) because they too have the same behavior.
+
+.class private auto ansi Program extends [mscorlib]System.Object
+{
+    .method static int32 Main() cil managed
+    {
+        .entrypoint
+        .maxstack 8
+        .locals(int64)
+
+        ldc.i8 0x1020304050607080
+        ldc.i4 64
+        ldc.i4 1
+        call int64 Program::Test_Shl_Helper(int64, int32, bool)
+        stloc.0
+
+        ldc.i8 0x1020304050607080
+        ldc.i4 64
+        ldc.i4 1
+        call int64 Program::Test_Shr_Helper(int64, int32, bool)
+        ldloc.0
+        bne.un FAIL
+
+        ldc.i8 0x1020304050607080
+        ldc.i4 64
+        ldc.i4 1
+        call int64 Program::Test_ShrUn_Helper(int64, int32, bool)
+        ldloc.0
+        bne.un FAIL
+
+        ldc.i8 0x1020304050607080
+        ldc.i4 64
+        ldc.i4 1
+        call int64 Program::Test_Shl_Decompose(int64, int32, bool)
+        ldloc.0
+        bne.un FAIL
+
+        ldc.i8 0x1020304050607080
+        ldc.i4 64
+        ldc.i4 1
+        call int64 Program::Test_Shr_Decompose(int64, int32, bool)
+        ldloc.0
+        bne.un FAIL
+
+        ldc.i8 0x1020304050607080
+        ldc.i4 64
+        ldc.i4 1
+        call int64 Program::Test_ShrUn_Decompose(int64, int32, bool)
+        ldloc.0
+        bne.un FAIL
+
+        ldc.i8 0x1020304050607080
+        ldc.i4 64
+        ldc.i4 1
+        call int64 Program::Test_Shl_gtFoldExpr(int64, int32, bool)
+        ldloc.0
+        bne.un FAIL
+
+        ldc.i8 0x1020304050607080
+        ldc.i4 64
+        ldc.i4 1
+        call int64 Program::Test_Shr_gtFoldExpr(int64, int32, bool)
+        ldloc.0
+        bne.un FAIL
+
+        ldc.i8 0x1020304050607080
+        ldc.i4 64
+        ldc.i4 1
+        call int64 Program::Test_ShrUn_gtFoldExpr(int64, int32, bool)
+        ldloc.0
+        bne.un FAIL
+
+        ldc.i8 0x1020304050607080
+        ldc.i4 64
+        ldc.i4 1
+        call int64 Program::Test_Shl_EvalOpIntegral(int64, int32, bool)
+        ldloc.0
+        bne.un FAIL
+
+        ldc.i8 0x1020304050607080
+        ldc.i4 64
+        ldc.i4 1
+        call int64 Program::Test_Shr_EvalOpIntegral(int64, int32, bool)
+        ldloc.0
+        bne.un FAIL
+
+        ldc.i8 0x1020304050607080
+        ldc.i4 64
+        ldc.i4 1
+        call int64 Program::Test_ShrUn_EvalOpIntegral(int64, int32, bool)
+        ldloc.0
+        bne.un FAIL
+
+        ldc.i4 100
+        ret
+
+    FAIL:
+        ldc.i4 1
+        ret
+    }
+
+    .method static int64 Test_Shl_Helper(int64, int32, bool) cil managed noinlining
+    {
+        .maxstack 2
+        ldarg.0
+        ldarg.1
+        shl
+        ret
+    }
+
+    .method static int64 Test_Shr_Helper(int64, int32, bool) cil managed noinlining
+    {
+        .maxstack 2
+        ldarg.0
+        ldarg.1
+        shr
+        ret
+    }
+
+    .method static int64 Test_ShrUn_Helper(int64, int32, bool) cil managed noinlining
+    {
+        .maxstack 2
+        ldarg.0
+        ldarg.1
+        shr.un
+        ret
+    }
+
+    .method static int64 Test_Shl_Decompose(int64, int32, bool) cil managed noinlining
+    {
+        .maxstack 2
+        ldarg.0
+        ldc.i4 64
+        shl
+        ret
+    }
+
+    .method static int64 Test_Shr_Decompose(int64, int32, bool) cil managed noinlining
+    {
+        .maxstack 2
+        ldarg.0
+        ldc.i4 64
+        shr
+        ret
+    }
+
+    .method static int64 Test_ShrUn_Decompose(int64, int32, bool) cil managed noinlining
+    {
+        .maxstack 2
+        ldarg.0
+        ldc.i4 64
+        shr.un
+        ret
+    }
+
+    .method static int64 Test_Shl_gtFoldExpr(int64, int32, bool) cil managed noinlining
+    {
+        .maxstack 2
+        ldc.i8 0x1020304050607080
+        ldc.i4 64
+        shl
+        ret
+    }
+
+    .method static int64 Test_Shr_gtFoldExpr(int64, int32, bool) cil managed noinlining
+    {
+        .maxstack 2
+        ldc.i8 0x1020304050607080
+        ldc.i4 64
+        shr
+        ret
+    }
+
+    .method static int64 Test_ShrUn_gtFoldExpr(int64, int32, bool) cil managed noinlining
+    {
+        .maxstack 2
+        ldc.i8 0x1020304050607080
+        ldc.i4 64
+        shr.un
+        ret
+    }
+
+    .method static int64 Test_Shl_EvalOpIntegral(int64, int32, bool) cil managed noinlining
+    {
+        .maxstack 2
+        ldc.i8 0x1020304050607080
+        ldarg.2
+        brtrue L1
+        ldc.i4 64
+        br L2
+    L1: ldc.i4 64
+    L2: shl
+        ret
+    }
+
+    .method static int64 Test_Shr_EvalOpIntegral(int64, int32, bool) cil managed noinlining
+    {
+        .maxstack 2
+        ldc.i8 0x1020304050607080
+        ldarg.2
+        brtrue L1
+        ldc.i4 64
+        br L2
+    L1: ldc.i4 64
+    L2: shr
+        ret
+    }
+
+    .method static int64 Test_ShrUn_EvalOpIntegral(int64, int32, bool) cil managed noinlining
+    {
+        .maxstack 2
+        ldc.i8 0x1020304050607080
+        ldarg.2
+        brtrue L1
+        ldc.i4 64
+        br L2
+    L1: ldc.i4 64
+    L2: shr.un
+        ret
+    }
+}

diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_15949/GitHub_15949.ilproj b/tests/src/JIT/Regression/JitBlue/GitHub_15949/GitHub_15949.ilproj
new file mode 100644 (file)
index 0000000..65b4dcf
--- /dev/null
+++ b/tests/src/JIT/Regression/JitBlue/GitHub_15949/GitHub_15949.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="GitHub_15949.il" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "></PropertyGroup>
+</Project>
\ No newline at end of file