From 4d6f13b4ce4fdcadfb186a93ac73591f4ab3466d Mon Sep 17 00:00:00 2001 From: Sergey Andreenko Date: Wed, 21 Mar 2018 12:59:31 -0700 Subject: [PATCH 1/5] add repro --- .../JitBlue/DevDiv_545497/DevDiv_545497.il | 117 ++++++++++++++++++ .../DevDiv_545497/DevDiv_545497.ilproj | 23 ++++ 2 files changed, 140 insertions(+) create mode 100644 tests/src/JIT/Regression/JitBlue/DevDiv_545497/DevDiv_545497.il create mode 100644 tests/src/JIT/Regression/JitBlue/DevDiv_545497/DevDiv_545497.ilproj 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 index 000000000000..fa6bf4997d05 --- /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 index 000000000000..5934cf63ac17 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/DevDiv_545497/DevDiv_545497.ilproj @@ -0,0 +1,23 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + {95DFC527-4DC1-495E-97D7-E94EE1F7140D} + Exe + ..\..\ + + + + + None + True + + + + + + + From c7ba8ca17617a51fadfc24f316f69396aaad4875 Mon Sep 17 00:00:00 2001 From: Sergey Andreenko Date: Wed, 21 Mar 2018 15:18:55 -0700 Subject: [PATCH 2/5] delete BuildShiftRotate for arm --- src/jit/lsra.h | 2 ++ src/jit/lsraarm.cpp | 15 +++++------- src/jit/lsraarm64.cpp | 11 ++++----- src/jit/lsraarmarch.cpp | 53 ----------------------------------------- 4 files changed, 12 insertions(+), 69 deletions(-) diff --git a/src/jit/lsra.h b/src/jit/lsra.h index fe1d71334de9..74534f102bb3 100644 --- a/src/jit/lsra.h +++ b/src/jit/lsra.h @@ -1654,9 +1654,11 @@ class LinearScan : public LinearScanInterface 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_ 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 df4a435589bd..c61e8db6f051 100644 --- a/src/jit/lsraarm.cpp +++ b/src/jit/lsraarm.cpp @@ -335,6 +335,12 @@ 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: + case GT_LSH_HI: + case GT_RSH_LO: assert(info->dstCount == 1); info->srcCount = appendBinaryLocationInfoToList(tree->AsOp()); assert(info->srcCount == (tree->gtOp.gtOp2->isContained() ? 1 : 2)); @@ -553,15 +559,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 6497ac877aef..6c1fc4af29a9 100644 --- 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 72acf829d10d..22c96f919aaa 100644 --- a/src/jit/lsraarmarch.cpp +++ b/src/jit/lsraarmarch.cpp @@ -111,59 +111,6 @@ void LinearScan::BuildIndir(GenTreeIndir* indirTree) #endif // FEATURE_SIMD } -//------------------------------------------------------------------------ -// 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. // From 33ce4f7c8b0baa6eac3fd2aaeb8fd193c3479963 Mon Sep 17 00:00:00 2001 From: Sergey Andreenko Date: Thu, 22 Mar 2018 02:25:32 -0700 Subject: [PATCH 3/5] fix GT_LSH_HI and GT_RSH_LO --- src/jit/lsraarm.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/jit/lsraarm.cpp b/src/jit/lsraarm.cpp index c61e8db6f051..800c2bb7ffe6 100644 --- a/src/jit/lsraarm.cpp +++ b/src/jit/lsraarm.cpp @@ -339,11 +339,15 @@ void LinearScan::BuildNode(GenTree* tree) 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); info->srcCount = appendBinaryLocationInfoToList(tree->AsOp()); - assert(info->srcCount == (tree->gtOp.gtOp2->isContained() ? 1 : 2)); + assert(info->srcCount == (tree->gtOp.gtOp2->isContained() ? 2 : 3)); break; case GT_RETURNTRAP: From 8891ef7cf47cae5d2a4fd19f52ae9bc0fb9d65ef Mon Sep 17 00:00:00 2001 From: Sergey Andreenko Date: Fri, 23 Mar 2018 17:44:46 -0700 Subject: [PATCH 4/5] return the special handling for GT_LSH_HI and GT_RSH_LO --- src/jit/lsra.h | 3 +++ src/jit/lsraarm.cpp | 43 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/src/jit/lsra.h b/src/jit/lsra.h index 74534f102bb3..fc8d8b26dc27 100644 --- a/src/jit/lsra.h +++ b/src/jit/lsra.h @@ -1659,6 +1659,9 @@ class LinearScan : public LinearScanInterface // '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 800c2bb7ffe6..7cb5e5f33e27 100644 --- a/src/jit/lsraarm.cpp +++ b/src/jit/lsraarm.cpp @@ -112,6 +112,46 @@ void LinearScan::BuildLclHeap(GenTree* tree) } } +//------------------------------------------------------------------------ +// BuildShiftLongCarry: Set the node info for GT_LSH_HI or GT_RSH_LO. +// +// Note: these operands have interfering uses and need the special handling. +// +// Arguments: +// tree - The node of interest +// +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. // @@ -343,10 +383,11 @@ void LinearScan::BuildNode(GenTree* tree) 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); - info->srcCount = appendBinaryLocationInfoToList(tree->AsOp()); + BuildShiftLongCarry(tree); assert(info->srcCount == (tree->gtOp.gtOp2->isContained() ? 2 : 3)); break; From 8520820f48e6d2256f6ccdd782d3b4cf2035a655 Mon Sep 17 00:00:00 2001 From: Sergey Andreenko Date: Sat, 24 Mar 2018 01:18:43 -0700 Subject: [PATCH 5/5] fix the header --- src/jit/lsraarm.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/jit/lsraarm.cpp b/src/jit/lsraarm.cpp index 7cb5e5f33e27..63e93fa8ea91 100644 --- a/src/jit/lsraarm.cpp +++ b/src/jit/lsraarm.cpp @@ -115,11 +115,11 @@ void LinearScan::BuildLclHeap(GenTree* tree) //------------------------------------------------------------------------ // BuildShiftLongCarry: Set the node info for GT_LSH_HI or GT_RSH_LO. // -// Note: these operands have interfering uses and need the special handling. -// // 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);