From: Carol Eidt Date: Mon, 16 Jul 2018 18:37:40 +0000 (-0700) Subject: Kill RCX when used by shift X-Git-Tag: submit/tizen/20210909.063632~11030^2~4345^2 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=cf30649338af0d414842381245f5d5ae7d4fb7db;p=platform%2Fupstream%2Fdotnet%2Fruntime.git 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 --- 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 --- 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 --- 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 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 index 0000000..ea8e8a7 --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18884/GitHub_18884.csproj @@ -0,0 +1,45 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {95DFC527-4DC1-495E-97D7-E94EE1F7140D} + Exe + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + ..\..\ + + + + + + + False + + + + + True + True + + + + + + + + + + + + + +