From bbe202b1de189812866acfa288ab1ac21a5fecdb Mon Sep 17 00:00:00 2001 From: Sergey Andreenko Date: Tue, 18 Apr 2017 10:44:01 -0700 Subject: [PATCH] small clean-up and bug fix in genIntToIntCast (#10989) genintToIntCast was cleaned-up, the assert that was wrong for x86 was deleted. The repro was added. --- src/jit/codegenxarch.cpp | 87 ++++++++-------------- .../JitBlue/DevDiv_406160/DevDiv_406160.il | 83 +++++++++++++++++++++ .../JitBlue/DevDiv_406160/DevDiv_406160.ilproj | 37 +++++++++ 3 files changed, 150 insertions(+), 57 deletions(-) create mode 100644 tests/src/JIT/Regression/JitBlue/DevDiv_406160/DevDiv_406160.il create mode 100644 tests/src/JIT/Regression/JitBlue/DevDiv_406160/DevDiv_406160.ilproj diff --git a/src/jit/codegenxarch.cpp b/src/jit/codegenxarch.cpp index a7881de..ad1f027 100644 --- a/src/jit/codegenxarch.cpp +++ b/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/tests/src/JIT/Regression/JitBlue/DevDiv_406160/DevDiv_406160.il b/tests/src/JIT/Regression/JitBlue/DevDiv_406160/DevDiv_406160.il new file mode 100644 index 0000000..a7eaea4 --- /dev/null +++ b/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/tests/src/JIT/Regression/JitBlue/DevDiv_406160/DevDiv_406160.ilproj b/tests/src/JIT/Regression/JitBlue/DevDiv_406160/DevDiv_406160.ilproj new file mode 100644 index 0000000..310db80 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/DevDiv_406160/DevDiv_406160.ilproj @@ -0,0 +1,37 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {95DFC527-4DC1-495E-97D7-E94EE1F7140D} + Exe + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + ..\..\ + + + + + + + + + False + + + + None + True + + + + + + + + + + + -- 2.7.4