From: Eugene Rozenfeld Date: Fri, 7 Sep 2018 17:48:06 +0000 (-0700) Subject: Fix for bug 12398: Lowering is inconsistent in checking safety of RegOptional. (... X-Git-Tag: accepted/tizen/unified/20190422.045933~1263 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=36449cf94237e7cddb7e43a9fe6f873e7f5ebf77;p=platform%2Fupstream%2Fcoreclr.git Fix for bug 12398: Lowering is inconsistent in checking safety of RegOptional. (#19740) This fixes an inconsistency in lowering where it fails to make an operand contained because IsSafeToContainMem() returns false yet it marks it regOptional, which may cause a problem if the operand will be loaded at the point of use. I also fixed a case where an operand was marked RegOptional even though there is a type size mismatch. There are 7 places that are affected, I added repro cases for 4 of them. I wasn't able to construct repros for the 3 places that deal with floating point operands but decided to fix those places anyway. Fixes #12398. --- diff --git a/src/jit/lower.cpp b/src/jit/lower.cpp index fb5fab2..c6648b8 100644 --- a/src/jit/lower.cpp +++ b/src/jit/lower.cpp @@ -4874,7 +4874,6 @@ GenTree* Lowering::LowerConstIntDivOrMod(GenTree* node) newDivMod = comp->gtNewOperNode(GT_SUB, type, comp->gtNewLclvNode(dividendLclNum, type), comp->gtNewOperNode(GT_AND, type, adjustedDividend, divisor)); - ContainCheckBinary(newDivMod->AsOp()); } // Remove the divisor and dividend nodes from the linear order, diff --git a/src/jit/lower.h b/src/jit/lower.h index c78e26b..c57e732 100644 --- a/src/jit/lower.h +++ b/src/jit/lower.h @@ -229,15 +229,18 @@ private: // operands. // // Arguments: - // tree - Gentree of a binary operation. + // tree - Gentree of a binary operation. + // isSafeToMarkOp1 True if it's safe to mark op1 as register optional + // isSafeToMarkOp2 True if it's safe to mark op2 as register optional // // Returns - // None. + // The caller is expected to get isSafeToMarkOp1 and isSafeToMarkOp2 + // by calling IsSafeToContainMem. // // Note: On xarch at most only one of the operands will be marked as // reg optional, even when both operands could be considered register // optional. - void SetRegOptionalForBinOp(GenTree* tree) + void SetRegOptionalForBinOp(GenTree* tree, bool isSafeToMarkOp1, bool isSafeToMarkOp2) { assert(GenTree::OperIsBinary(tree->OperGet())); @@ -246,8 +249,9 @@ private: const unsigned operatorSize = genTypeSize(tree->TypeGet()); - const bool op1Legal = tree->OperIsCommutative() && (operatorSize == genTypeSize(op1->TypeGet())); - const bool op2Legal = operatorSize == genTypeSize(op2->TypeGet()); + const bool op1Legal = + isSafeToMarkOp1 && tree->OperIsCommutative() && (operatorSize == genTypeSize(op1->TypeGet())); + const bool op2Legal = isSafeToMarkOp2 && (operatorSize == genTypeSize(op2->TypeGet())); GenTree* regOptionalOperand = nullptr; if (op1Legal) diff --git a/src/jit/lowerxarch.cpp b/src/jit/lowerxarch.cpp index c748da0..de22b59 100644 --- a/src/jit/lowerxarch.cpp +++ b/src/jit/lowerxarch.cpp @@ -1553,25 +1553,54 @@ void Lowering::ContainCheckMul(GenTreeOp* node) GenTree* op1 = node->gtOp.gtOp1; GenTree* op2 = node->gtOp.gtOp2; + bool isSafeToContainOp1 = true; + bool isSafeToContainOp2 = true; + // Case of float/double mul. if (varTypeIsFloating(node->TypeGet())) { assert(node->OperGet() == GT_MUL); - if (IsContainableMemoryOp(op2) || op2->IsCnsNonZeroFltOrDbl()) + if (op2->IsCnsNonZeroFltOrDbl()) { MakeSrcContained(node, op2); } - else if (op1->IsCnsNonZeroFltOrDbl() || (IsContainableMemoryOp(op1) && IsSafeToContainMem(node, op1))) + else if (IsContainableMemoryOp(op2)) + { + isSafeToContainOp2 = IsSafeToContainMem(node, op2); + if (isSafeToContainOp2) + { + MakeSrcContained(node, op2); + } + } + + if (!op2->isContained()) { // Since GT_MUL is commutative, we will try to re-order operands if it is safe to // generate more efficient code sequence for the case of GT_MUL(op1=memOp, op2=non-memOp) - MakeSrcContained(node, op1); + if (op1->IsCnsNonZeroFltOrDbl()) + { + MakeSrcContained(node, op1); + } + else if (IsContainableMemoryOp(op1)) + { + isSafeToContainOp1 = IsSafeToContainMem(node, op1); + if (isSafeToContainOp1) + { + MakeSrcContained(node, op1); + } + } } - else + + if (!op1->isContained() && !op2->isContained()) { // If there are no containable operands, we can make an operand reg optional. - SetRegOptionalForBinOp(node); + // IsSafeToContainMem is expensive so we call it at most once for each operand + // in this method. If we already called IsSafeToContainMem, it must have returned false; + // otherwise, the corresponding operand (op1 or op2) would be contained. + isSafeToContainOp1 = isSafeToContainOp1 && IsSafeToContainMem(node, op1); + isSafeToContainOp2 = isSafeToContainOp2 && IsSafeToContainMem(node, op2); + SetRegOptionalForBinOp(node, isSafeToContainOp1, isSafeToContainOp2); } return; } @@ -1639,21 +1668,42 @@ void Lowering::ContainCheckMul(GenTreeOp* node) // if (memOp == nullptr) { - if (IsContainableMemoryOp(op2) && (op2->TypeGet() == node->TypeGet()) && IsSafeToContainMem(node, op2)) + if ((op2->TypeGet() == node->TypeGet()) && IsContainableMemoryOp(op2)) { - memOp = op2; + isSafeToContainOp2 = IsSafeToContainMem(node, op2); + if (isSafeToContainOp2) + { + memOp = op2; + } } - else if (IsContainableMemoryOp(op1) && (op1->TypeGet() == node->TypeGet()) && IsSafeToContainMem(node, op1)) + + if ((memOp == nullptr) && (op1->TypeGet() == node->TypeGet()) && IsContainableMemoryOp(op1)) { - memOp = op1; + isSafeToContainOp1 = IsSafeToContainMem(node, op1); + if (isSafeToContainOp1) + { + memOp = op1; + } } } else { - if ((memOp->TypeGet() != node->TypeGet()) || !IsSafeToContainMem(node, memOp)) + if ((memOp->TypeGet() != node->TypeGet())) { memOp = nullptr; } + else if (!IsSafeToContainMem(node, memOp)) + { + if (memOp == op1) + { + isSafeToContainOp1 = false; + } + else + { + isSafeToContainOp2 = false; + } + memOp = nullptr; + } } // To generate an LEA we need to force memOp into a register // so don't allow memOp to be 'contained' @@ -1664,23 +1714,34 @@ void Lowering::ContainCheckMul(GenTreeOp* node) { MakeSrcContained(node, memOp); } - else if (imm != nullptr) - { - // Has a contained immediate operand. - // Only 'other' operand can be marked as reg optional. - assert(other != nullptr); - other->SetRegOptional(); - } - else if (hasImpliedFirstOperand) - { - // Only op2 can be marke as reg optional. - op2->SetRegOptional(); - } else { - // If there are no containable operands, we can make either of op1 or op2 - // as reg optional. - SetRegOptionalForBinOp(node); + // IsSafeToContainMem is expensive so we call it at most once for each operand + // in this method. If we already called IsSafeToContainMem, it must have returned false; + // otherwise, memOp would be set to the corresponding operand (op1 or op2). + if (imm != nullptr) + { + // Has a contained immediate operand. + // Only 'other' operand can be marked as reg optional. + assert(other != nullptr); + + isSafeToContainOp1 = ((other == op1) && isSafeToContainOp1 && IsSafeToContainMem(node, op1)); + isSafeToContainOp2 = ((other == op2) && isSafeToContainOp2 && IsSafeToContainMem(node, op2)); + } + else if (hasImpliedFirstOperand) + { + // Only op2 can be marked as reg optional. + isSafeToContainOp1 = false; + isSafeToContainOp2 = isSafeToContainOp2 && IsSafeToContainMem(node, op2); + } + else + { + // If there are no containable operands, we can make either of op1 or op2 + // as reg optional. + isSafeToContainOp1 = isSafeToContainOp1 && IsSafeToContainMem(node, op1); + isSafeToContainOp2 = isSafeToContainOp2 && IsSafeToContainMem(node, op2); + } + SetRegOptionalForBinOp(node, isSafeToContainOp1, isSafeToContainOp2); } } } @@ -1853,18 +1914,27 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp) } assert(otherOp != nullptr); + bool isSafeToContainOtherOp = true; if (otherOp->IsCnsNonZeroFltOrDbl()) { MakeSrcContained(cmp, otherOp); } - else if (IsContainableMemoryOp(otherOp) && ((otherOp == op2) || IsSafeToContainMem(cmp, otherOp))) + else if (IsContainableMemoryOp(otherOp)) { - MakeSrcContained(cmp, otherOp); + isSafeToContainOtherOp = IsSafeToContainMem(cmp, otherOp); + if (isSafeToContainOtherOp) + { + MakeSrcContained(cmp, otherOp); + } } - else + + if (!otherOp->isContained() && isSafeToContainOtherOp && IsSafeToContainMem(cmp, otherOp)) { // SSE2 allows only otherOp to be a memory-op. Since otherOp is not // contained, we can mark it reg-optional. + // IsSafeToContainMem is expensive so we call it at most once for otherOp. + // If we already called IsSafeToContainMem, it must have returned false; + // otherwise, otherOp would be contained. otherOp->SetRegOptional(); } @@ -1895,24 +1965,44 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp) // Note that TEST does not have a r,rm encoding like CMP has but we can still // contain the second operand because the emitter maps both r,rm and rm,r to // the same instruction code. This avoids the need to special case TEST here. + + bool isSafeToContainOp1 = true; + bool isSafeToContainOp2 = true; + if (IsContainableMemoryOp(op2)) { - MakeSrcContained(cmp, op2); - } - else if (IsContainableMemoryOp(op1) && IsSafeToContainMem(cmp, op1)) - { - MakeSrcContained(cmp, op1); + isSafeToContainOp2 = IsSafeToContainMem(cmp, op2); + if (isSafeToContainOp2) + { + MakeSrcContained(cmp, op2); + } } - else if (op1->IsCnsIntOrI()) + + if (!op2->isContained() && IsContainableMemoryOp(op1)) { - op2->SetRegOptional(); + isSafeToContainOp1 = IsSafeToContainMem(cmp, op1); + if (isSafeToContainOp1) + { + MakeSrcContained(cmp, op1); + } } - else + + if (!op1->isContained() && !op2->isContained()) { // One of op1 or op2 could be marked as reg optional // to indicate that codegen can still generate code // if one of them is on stack. - PreferredRegOptionalOperand(cmp)->SetRegOptional(); + GenTree* regOptionalCandidate = op1->IsCnsIntOrI() ? op2 : PreferredRegOptionalOperand(cmp); + + // IsSafeToContainMem is expensive so we call it at most once for each operand + // in this method. If we already called IsSafeToContainMem, it must have returned false; + // otherwise, the corresponding operand (op1 or op2) would be contained. + bool setRegOptional = (regOptionalCandidate == op1) ? isSafeToContainOp1 && IsSafeToContainMem(cmp, op1) + : isSafeToContainOp2 && IsSafeToContainMem(cmp, op2); + if (setRegOptional) + { + regOptionalCandidate->SetRegOptional(); + } } } } @@ -2056,11 +2146,6 @@ void Lowering::ContainCheckBinary(GenTreeOp* node) return; } - // We're not marking a constant hanging on the left of an add - // as containable so we assign it to a register having CQ impact. - // TODO-XArch-CQ: Detect this case and support both generating a single instruction - // for GT_ADD(Constant, SomeTree) - GenTree* op1 = node->gtOp1; GenTree* op2 = node->gtOp2; @@ -2068,9 +2153,11 @@ void Lowering::ContainCheckBinary(GenTreeOp* node) // In case of memory-op, we can encode it directly provided its type matches with 'tree' type. // This is because during codegen, type of 'tree' is used to determine emit Type size. If the types // do not match, they get normalized (i.e. sign/zero extended) on load into a register. - bool directlyEncodable = false; - bool binOpInRMW = false; - GenTree* operand = nullptr; + bool directlyEncodable = false; + bool binOpInRMW = false; + GenTree* operand = nullptr; + bool isSafeToContainOp1 = true; + bool isSafeToContainOp2 = true; if (IsContainableImmed(node, op2)) { @@ -2083,22 +2170,34 @@ void Lowering::ContainCheckBinary(GenTreeOp* node) if (!binOpInRMW) { const unsigned operatorSize = genTypeSize(node->TypeGet()); - if (IsContainableMemoryOp(op2) && (genTypeSize(op2->TypeGet()) == operatorSize)) + if ((genTypeSize(op2->TypeGet()) == operatorSize) && IsContainableMemoryOp(op2)) { - directlyEncodable = true; - operand = op2; + isSafeToContainOp2 = IsSafeToContainMem(node, op2); + if (isSafeToContainOp2) + { + directlyEncodable = true; + operand = op2; + } } - else if (node->OperIsCommutative()) + + if ((operand == nullptr) && node->OperIsCommutative()) { - if (IsContainableImmed(node, op1) || - (IsContainableMemoryOp(op1) && (genTypeSize(op1->TypeGet()) == operatorSize) && - IsSafeToContainMem(node, op1))) + // If it is safe, we can reverse the order of operands of commutative operations for efficient + // codegen + if (IsContainableImmed(node, op1)) { - // If it is safe, we can reverse the order of operands of commutative operations for efficient - // codegen directlyEncodable = true; operand = op1; } + else if ((genTypeSize(op1->TypeGet()) == operatorSize) && IsContainableMemoryOp(op1)) + { + isSafeToContainOp1 = IsSafeToContainMem(node, op1); + if (isSafeToContainOp1) + { + directlyEncodable = true; + operand = op1; + } + } } } } @@ -2113,7 +2212,14 @@ void Lowering::ContainCheckBinary(GenTreeOp* node) // If this binary op neither has contained operands, nor is a // Read-Modify-Write (RMW) operation, we can mark its operands // as reg optional. - SetRegOptionalForBinOp(node); + + // IsSafeToContainMem is expensive so we call it at most once for each operand + // in this method. If we already called IsSafeToContainMem, it must have returned false; + // otherwise, directlyEncodable would be true. + isSafeToContainOp1 = isSafeToContainOp1 && IsSafeToContainMem(node, op1); + isSafeToContainOp2 = isSafeToContainOp2 && IsSafeToContainMem(node, op2); + + SetRegOptionalForBinOp(node, isSafeToContainOp1, isSafeToContainOp2); } } @@ -3050,12 +3156,23 @@ void Lowering::ContainCheckFloatBinary(GenTreeOp* node) // everything is made explicit by adding casts. assert(op1->TypeGet() == op2->TypeGet()); - if (IsContainableMemoryOp(op2) || op2->IsCnsNonZeroFltOrDbl()) + bool isSafeToContainOp1 = true; + bool isSafeToContainOp2 = true; + + if (op2->IsCnsNonZeroFltOrDbl()) { MakeSrcContained(node, op2); } - else if (node->OperIsCommutative() && - (op1->IsCnsNonZeroFltOrDbl() || (IsContainableMemoryOp(op1) && IsSafeToContainMem(node, op1)))) + else if (IsContainableMemoryOp(op2)) + { + isSafeToContainOp2 = IsSafeToContainMem(node, op2); + if (isSafeToContainOp2) + { + MakeSrcContained(node, op2); + } + } + + if (!op2->isContained() && node->OperIsCommutative()) { // Though we have GT_ADD(op1=memOp, op2=non-memOp, we try to reorder the operands // as long as it is safe so that the following efficient code sequence is generated: @@ -3065,12 +3182,30 @@ void Lowering::ContainCheckFloatBinary(GenTreeOp* node) // Instead of // movss op1Reg, [memOp]; addss/sd targetReg, Op2Reg (if op1Reg == targetReg) OR // movss op1Reg, [memOp]; movaps targetReg, op1Reg, addss/sd targetReg, Op2Reg - MakeSrcContained(node, op1); + + if (op1->IsCnsNonZeroFltOrDbl()) + { + MakeSrcContained(node, op1); + } + else if (IsContainableMemoryOp(op1)) + { + isSafeToContainOp1 = IsSafeToContainMem(node, op1); + if (isSafeToContainOp1) + { + MakeSrcContained(node, op1); + } + } } - else + + if (!op1->isContained() && !op2->isContained()) { // If there are no containable operands, we can make an operand reg optional. - SetRegOptionalForBinOp(node); + // IsSafeToContainMem is expensive so we call it at most once for each operand + // in this method. If we already called IsSafeToContainMem, it must have returned false; + // otherwise, the corresponding operand (op1 or op2) would be contained. + isSafeToContainOp1 = isSafeToContainOp1 && IsSafeToContainMem(node, op1); + isSafeToContainOp2 = isSafeToContainOp2 && IsSafeToContainMem(node, op2); + SetRegOptionalForBinOp(node, isSafeToContainOp1, isSafeToContainOp2); } } diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_12398/Github_12398.cs b/tests/src/JIT/Regression/JitBlue/GitHub_12398/Github_12398.cs new file mode 100644 index 0000000..69057f3 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_12398/Github_12398.cs @@ -0,0 +1,84 @@ +// 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. +// +// GitHub12398: Lowering is inconsistent in checking safety of RegOptional. + +using System; +using System.Runtime.CompilerServices; + +struct S0 +{ + public sbyte F1; + public sbyte F2; +} + +class GitHub_12398 +{ + static int Main() + { + int result = 100; + if (TestBinary() != 0) { + Console.WriteLine("Failed TestBinary"); + result = -1; + } + + if (TestCompare()) { + Console.WriteLine("Failed TestCompare"); + result = -1; + } + + if (TestMul() != 1) { + Console.WriteLine("Failed TestMul"); + result = -1; + } + + if (TestMulTypeSize() != 0) { + Console.WriteLine ("Failed TestMulTypeSize"); + result = -1; + } + + if (result == 100) { + Console.WriteLine("PASSED"); + } + else { + Console.WriteLine("FAILED"); + } + + return result; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static long TestBinary() + { + long l = 0; + long result = l ^ System.Threading.Interlocked.Exchange(ref l, 1); + return result; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static bool TestCompare() + { + long l = 0; + bool result = l > System.Threading.Interlocked.Exchange(ref l, 1); + return result; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static long TestMul() + { + long l = 1; + long result = l * System.Threading.Interlocked.Exchange(ref l, 0); + return result; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static int TestMulTypeSize() + { + S0 s = new S0(); + s.F2--; + int i = System.Threading.Volatile.Read(ref s.F1) * 100; + return i; + } +} + diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_12398/Github_12398.csproj b/tests/src/JIT/Regression/JitBlue/GitHub_12398/Github_12398.csproj new file mode 100644 index 0000000..67d0b29 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_12398/Github_12398.csproj @@ -0,0 +1,34 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {ADEEA3D1-B67B-456E-8F2B-6DCCACC2D34C} + Exe + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + ..\..\ + + + + + + + False + + + + + True + + + + + + + + + +