small clean-up and bug fix in genIntToIntCast (dotnet/coreclr#10989)

genintToIntCast was cleaned-up, the assert that was wrong for x86 was deleted. The repro was added.

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

diff --git a/src/coreclr/src/jit/codegenxarch.cpp b/src/coreclr/src/jit/codegenxarch.cpp
index a7881de..ad1f027 100644 (file)
--- a/src/coreclr/src/jit/codegenxarch.cpp
+++ b/src/coreclr/src/jit/codegenxarch.cpp
@@ -6344,7 +6344,7 @@ void CodeGen::genSetRegToCond(regNumber dstReg, GenTreePtr tree)
 
 #if !defined(_TARGET_64BIT_)
 //------------------------------------------------------------------------
-// genIntToIntCast: Generate code for long to int casts on x86.
+// genLongToIntCast: Generate code for long to int casts on x86.
 //
 // Arguments:
 //    cast - The GT_CAST node
@@ -6454,14 +6454,15 @@ void CodeGen::genIntToIntCast(GenTreePtr treeNode)
 
     GenTreePtr castOp  = treeNode->gtCast.CastOp();
     var_types  srcType = genActualType(castOp->TypeGet());
+    noway_assert(genTypeSize(srcType) >= 4);
 
-#if !defined(_TARGET_64BIT_)
+#ifdef _TARGET_X86_
     if (varTypeIsLong(srcType))
     {
         genLongToIntCast(treeNode);
         return;
     }
-#endif // !defined(_TARGET_64BIT_)
+#endif // _TARGET_X86_
 
     regNumber targetReg     = treeNode->gtRegNum;
     regNumber sourceReg     = castOp->gtRegNum;
@@ -6477,18 +6478,17 @@ void CodeGen::genIntToIntCast(GenTreePtr treeNode)
     }
 
     bool requiresOverflowCheck = false;
-    bool needAndAfter          = false;
 
     assert(genIsValidIntReg(targetReg));
     assert(genIsValidIntReg(sourceReg));
 
-    instruction ins  = INS_invalid;
-    emitAttr    size = EA_UNKNOWN;
+    instruction ins     = INS_invalid;
+    emitAttr    srcSize = EA_ATTR(genTypeSize(srcType));
+    emitAttr    dstSize = EA_ATTR(genTypeSize(dstType));
 
-    if (genTypeSize(srcType) < genTypeSize(dstType))
+    if (srcSize < dstSize)
     {
         // Widening cast
-
         // Is this an Overflow checking cast?
         // We only need to handle one case, as the other casts can never overflow.
         //   cast from TYP_INT to TYP_ULONG
@@ -6496,14 +6496,11 @@ void CodeGen::genIntToIntCast(GenTreePtr treeNode)
         if (treeNode->gtOverflow() && (srcType == TYP_INT) && (dstType == TYP_ULONG))
         {
             requiresOverflowCheck = true;
-            size                  = EA_ATTR(genTypeSize(srcType));
             ins                   = INS_mov;
         }
         else
         {
-            // we need the source size
-            size = EA_ATTR(genTypeSize(srcType));
-            noway_assert(size < EA_PTRSIZE);
+            noway_assert(srcSize < EA_PTRSIZE);
 
             ins = ins_Move_Extend(srcType, castOp->InReg());
 
@@ -6513,7 +6510,7 @@ void CodeGen::genIntToIntCast(GenTreePtr treeNode)
                 64-bit, and a regular 32-bit mov clears the high 32 bits (like the non-existant movzxd),
                 but for a sign extension from TYP_INT to TYP_LONG, we need to use movsxd opcode.
             */
-            if (!isUnsignedSrc && !isUnsignedDst && (size == EA_4BYTE) && (genTypeSize(dstType) > EA_4BYTE))
+            if (!isUnsignedSrc && !isUnsignedDst)
             {
 #ifdef _TARGET_X86_
                 NYI_X86("Cast to 64 bit for x86/RyuJIT");
@@ -6521,36 +6518,22 @@ void CodeGen::genIntToIntCast(GenTreePtr treeNode)
                 ins = INS_movsxd;
 #endif // !_TARGET_X86_
             }
-
-            /*
-                Special case: for a cast of byte to char we first
-                have to expand the byte (w/ sign extension), then
-                mask off the high bits.
-                Use 'movsx' followed by 'and'
-            */
-            if (!isUnsignedSrc && isUnsignedDst && (genTypeSize(dstType) < EA_4BYTE))
-            {
-                noway_assert(genTypeSize(dstType) == EA_2BYTE && size == EA_1BYTE);
-                needAndAfter = true;
-            }
         }
     }
     else
     {
         // Narrowing cast, or sign-changing cast
-        noway_assert(genTypeSize(srcType) >= genTypeSize(dstType));
+        noway_assert(srcSize >= dstSize);
 
         // Is this an Overflow checking cast?
         if (treeNode->gtOverflow())
         {
             requiresOverflowCheck = true;
-            size                  = EA_ATTR(genTypeSize(srcType));
             ins                   = INS_mov;
         }
         else
         {
-            size = EA_ATTR(genTypeSize(dstType));
-            ins  = ins_Move_Extend(dstType, castOp->InReg());
+            ins = ins_Move_Extend(dstType, castOp->InReg());
         }
     }
 
@@ -6632,7 +6615,7 @@ void CodeGen::genIntToIntCast(GenTreePtr treeNode)
         if (signCheckOnly)
         {
             // We only need to check for a negative value in sourceReg
-            inst_RV_IV(INS_cmp, sourceReg, 0, size);
+            inst_RV_IV(INS_cmp, sourceReg, 0, srcSize);
             genJumpToThrowHlpBlk(EJ_jl, SCK_OVERFLOW);
         }
         else
@@ -6645,13 +6628,13 @@ void CodeGen::genIntToIntCast(GenTreePtr treeNode)
                 {
                     regNumber tmpReg = treeNode->GetSingleTempReg();
                     inst_RV_RV(INS_mov, tmpReg, sourceReg, TYP_LONG); // Move the 64-bit value to a writeable temp reg
-                    inst_RV_SH(INS_SHIFT_RIGHT_LOGICAL, size, tmpReg, 32); // Shift right by 32 bits
-                    genJumpToThrowHlpBlk(EJ_jne, SCK_OVERFLOW);            // Throw if result shift is non-zero
+                    inst_RV_SH(INS_SHIFT_RIGHT_LOGICAL, srcSize, tmpReg, 32); // Shift right by 32 bits
+                    genJumpToThrowHlpBlk(EJ_jne, SCK_OVERFLOW);               // Throw if result shift is non-zero
                 }
                 else
                 {
                     noway_assert(typeMask != 0);
-                    inst_RV_IV(INS_TEST, sourceReg, typeMask, size);
+                    inst_RV_IV(INS_TEST, sourceReg, typeMask, srcSize);
                     genJumpToThrowHlpBlk(EJ_jne, SCK_OVERFLOW);
                 }
             }
@@ -6665,12 +6648,12 @@ void CodeGen::genIntToIntCast(GenTreePtr treeNode)
 
                 noway_assert((typeMin != 0) && (typeMax != 0));
 
-                inst_RV_IV(INS_cmp, sourceReg, typeMax, size);
+                inst_RV_IV(INS_cmp, sourceReg, typeMax, srcSize);
                 genJumpToThrowHlpBlk(EJ_jg, SCK_OVERFLOW);
 
                 // Compare with the MIN
 
-                inst_RV_IV(INS_cmp, sourceReg, typeMin, size);
+                inst_RV_IV(INS_cmp, sourceReg, typeMin, srcSize);
                 genJumpToThrowHlpBlk(EJ_jl, SCK_OVERFLOW);
             }
         }
@@ -6680,15 +6663,13 @@ void CodeGen::genIntToIntCast(GenTreePtr treeNode)
             // On amd64, we can hit this path for a same-register
             // 4-byte to 8-byte widening conversion, and need to
             // emit the instruction to set the high bits correctly.
-            || (EA_ATTR(genTypeSize(dstType)) == EA_8BYTE && EA_ATTR(genTypeSize(srcType)) == EA_4BYTE)
+            || (dstSize == EA_8BYTE && srcSize == EA_4BYTE)
 #endif // _TARGET_AMD64_
                 )
-            inst_RV_RV(ins, targetReg, sourceReg, srcType, size);
+            inst_RV_RV(ins, targetReg, sourceReg, srcType, srcSize);
     }
     else // non-overflow checking cast
     {
-        noway_assert(size < EA_PTRSIZE || srcType == dstType);
-
         // We may have code transformations that result in casts where srcType is the same as dstType.
         // e.g. Bug 824281, in which a comma is split by the rationalizer, leaving an assignment of a
         // long constant to a long lclVar.
@@ -6697,7 +6678,7 @@ void CodeGen::genIntToIntCast(GenTreePtr treeNode)
             ins = INS_mov;
         }
         /* Is the value sitting in a non-byte-addressable register? */
-        else if (castOp->InReg() && (size == EA_1BYTE) && !isByteReg(sourceReg))
+        else if (castOp->InReg() && (dstSize == EA_1BYTE) && !isByteReg(sourceReg))
         {
             if (isUnsignedDst)
             {
@@ -6713,21 +6694,21 @@ void CodeGen::genIntToIntCast(GenTreePtr treeNode)
             /* Generate "mov targetReg, castOp->gtReg */
             if (targetReg != sourceReg)
             {
-                inst_RV_RV(INS_mov, targetReg, sourceReg, srcType);
+                inst_RV_RV(INS_mov, targetReg, sourceReg, srcType, srcSize);
             }
         }
 
         if (ins == INS_AND)
         {
-            noway_assert((needAndAfter == false) && isUnsignedDst);
+            noway_assert(isUnsignedDst);
 
             /* Generate "and reg, MASK */
             unsigned fillPattern;
-            if (size == EA_1BYTE)
+            if (dstSize == EA_1BYTE)
             {
                 fillPattern = 0xff;
             }
-            else if (size == EA_2BYTE)
+            else if (dstSize == EA_2BYTE)
             {
                 fillPattern = 0xffff;
             }
@@ -6741,37 +6722,29 @@ void CodeGen::genIntToIntCast(GenTreePtr treeNode)
 #ifdef _TARGET_AMD64_
         else if (ins == INS_movsxd)
         {
-            noway_assert(!needAndAfter);
-            inst_RV_RV(ins, targetReg, sourceReg, srcType, size);
+            inst_RV_RV(ins, targetReg, sourceReg, srcType, srcSize);
         }
 #endif // _TARGET_AMD64_
         else if (ins == INS_mov)
         {
-            noway_assert(!needAndAfter);
             if (targetReg != sourceReg
 #ifdef _TARGET_AMD64_
                 // On amd64, 'mov' is the opcode used to zero-extend from
                 // 4 bytes to 8 bytes.
-                || (EA_ATTR(genTypeSize(dstType)) == EA_8BYTE && EA_ATTR(genTypeSize(srcType)) == EA_4BYTE)
+                || (dstSize == EA_8BYTE && srcSize == EA_4BYTE)
 #endif // _TARGET_AMD64_
                     )
             {
-                inst_RV_RV(ins, targetReg, sourceReg, srcType, size);
+                inst_RV_RV(ins, targetReg, sourceReg, srcType, srcSize);
             }
         }
         else
         {
             noway_assert(ins == INS_movsx || ins == INS_movzx);
+            noway_assert(srcSize >= dstSize);
 
             /* Generate "mov targetReg, castOp->gtReg */
-            inst_RV_RV(ins, targetReg, sourceReg, srcType, size);
-
-            /* Mask off high bits for cast from byte to char */
-            if (needAndAfter)
-            {
-                noway_assert(genTypeSize(dstType) == 2 && ins == INS_movsx);
-                inst_RV_IV(INS_AND, targetReg, 0xFFFF, EA_4BYTE);
-            }
+            inst_RV_RV(ins, targetReg, sourceReg, srcType, dstSize);
         }
     }
 

diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/DevDiv_406160/DevDiv_406160.il b/src/coreclr/tests/src/JIT/Regression/JitBlue/DevDiv_406160/DevDiv_406160.il
new file mode 100644 (file)
index 0000000..a7eaea4
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/DevDiv_406160/DevDiv_406160.il
@@ -0,0 +1,83 @@
+// 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.
+
+
+// Metadata version: v4.0.30319
+.assembly extern System.Runtime
+{
+  .publickeytoken = (B0 3F 5F 7F 11 D5 0A 3A )                         // .?_....:
+  .ver 4:2:0:0
+}
+.assembly extern System.Console
+{
+  .publickeytoken = (B0 3F 5F 7F 11 D5 0A 3A )                         // .?_....:
+  .ver 4:1:0:0
+}
+.assembly DevDiv_406160
+{
+}
+
+.class private auto ansi beforefieldinit Bug.Program
+       extends [System.Runtime]System.Object
+{
+  .method static char ILGEN_METHOD(int16, unsigned int16, native unsigned int)
+   {
+      .maxstack  65535
+      .locals init (bool, int64, native unsigned int)
+    IL_0000: ldarg.s 0x00
+    IL_0002: ldc.i4.1
+    IL_0015: clt
+    IL_001b: starg.s 0x00
+    IL_001d: ldloc 0x0001
+    IL_0067: ldc.i8 0xc3ec93cfd869ae83
+    IL_0072: ldc.r8 float64(0xb47a62a75e195a1c)
+    IL_007c: conv.ovf.u8
+    IL_007d: ldc.i4.1
+    IL_0081: conv.ovf.i8.un
+    IL_0088: div.un
+    IL_0089: add.ovf.un
+    IL_008c: ldloc 0x0001
+    IL_009a: ldc.i8 0x97a27f9613c909c1
+    IL_00a3: dup
+    IL_00a4: clt
+    IL_00a6: shr.un
+    IL_00a7: xor
+    IL_00a8: ldarg.s 0x00
+    IL_00aa: conv.ovf.u8.un
+    IL_00ab: and
+    IL_00ac: ldloc.s 0x01
+    IL_00ae: and
+    IL_00af: conv.ovf.i2.un
+    IL_00b0: xor
+    IL_00cd: conv.i4
+    IL_00ce: ret
+  }
+
+  .method public hidebysig static int32  Main() cil managed
+  {
+    .entrypoint
+    // Code size       22 (0x16)
+    .maxstack  8
+    IL_0001:  ldc.i4.0
+    IL_0002:  ldc.i4.0
+     IL_0003:  ldc.i4.0
+    IL_0004:  call       char   Bug.Program::ILGEN_METHOD(int16, unsigned int16, native unsigned int)
+       IL_0005:  pop
+    IL_0009:  ldstr      "Pass"
+    IL_000e:  call       void [System.Console]System.Console::WriteLine(string)
+    IL_0013:  ldc.i4.s   100
+    IL_0015:  ret
+  } // end of method Program::Main
+
+  .method public hidebysig specialname rtspecialname 
+          instance void  .ctor() cil managed
+  {
+    // Code size       7 (0x7)
+    .maxstack  8
+    IL_0000:  ldarg.0
+    IL_0001:  call       instance void [System.Runtime]System.Object::.ctor()
+    IL_0006:  ret
+  } // end of method Program::.ctor
+
+} // end of class Bug.Program
\ No newline at end of file

diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/DevDiv_406160/DevDiv_406160.ilproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/DevDiv_406160/DevDiv_406160.ilproj
new file mode 100644 (file)
index 0000000..310db80
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/DevDiv_406160/DevDiv_406160.ilproj
@@ -0,0 +1,37 @@
+<?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>None</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="DevDiv_406160.il" />
+  </ItemGroup>
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' ">
+  </PropertyGroup> 
+</Project>