From fd28c76cf36738195d8f4108d1f43b34f04a33ae Mon Sep 17 00:00:00 2001 From: anthonycanino Date: Thu, 9 Dec 2021 06:55:12 -0800 Subject: [PATCH] Add additional binary operations into the RangeCheck analysis. (#61662) * Add additional binary operations into the RangeCheck analysis. GT_LSH and GT_MUL are now covered in the range analysis check. This allows to catch and eliminate the range check for cases like ``` ReadOnlySpan readOnlySpan => new byte[] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 }; byte byt = 0; for (int i = 0; i < 5; i++) { byt = readOnlySpan[i * 3]; // ... } ``` or ``` bool[] flags = new bool[Size + 1]; for (int i = 2; i <= Size; i++) { for (int k = i * 2; k <= Size; k += i) { flags[k] = false; } } ``` Note that without this change, the previous snippet would not eliminate the range check on `flags[k]`, but the equivalent snippet would ``` for (int i = 2; i <= Size; i++) { for (int k = i + i; k <= Size; k += i) { flags[k] = false; } } ``` as additional was implemented in the range check analysis, but multiply was not. * RangeCheck multiply overflow fix and tests. Tests catch some edge cases with multiplcation overflow IF the overflow detection isn't implemented correctly. * Use CheckedOps::Signed instead of false. * Reduced overflow test array size to more reasonable value. * Update src/coreclr/jit/rangecheck.cpp Co-authored-by: Egor Bogatov * Update src/coreclr/jit/rangecheck.cpp Co-authored-by: Egor Bogatov Co-authored-by: Egor Bogatov --- src/coreclr/jit/rangecheck.cpp | 88 +++++++++++--- src/coreclr/jit/rangecheck.h | 107 +++++++++++++++++ .../opt/AssertionPropagation/ArrBoundElim.cs | 111 ++++++++++++++++++ .../AssertionPropagation/ArrBoundElim.csproj | 13 ++ 4 files changed, 301 insertions(+), 18 deletions(-) create mode 100644 src/tests/JIT/opt/AssertionPropagation/ArrBoundElim.cs create mode 100644 src/tests/JIT/opt/AssertionPropagation/ArrBoundElim.csproj diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index 4b957a720adc3..8590ecdd81c9f 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -336,18 +336,20 @@ void RangeCheck::Widen(BasicBlock* block, GenTree* tree, Range* pRange) bool RangeCheck::IsBinOpMonotonicallyIncreasing(GenTreeOp* binop) { - assert(binop->OperIs(GT_ADD)); + assert(binop->OperIs(GT_ADD, GT_MUL, GT_LSH)); GenTree* op1 = binop->gtGetOp1(); GenTree* op2 = binop->gtGetOp2(); JITDUMP("[RangeCheck::IsBinOpMonotonicallyIncreasing] [%06d], [%06d]\n", Compiler::dspTreeID(op1), Compiler::dspTreeID(op2)); - // Check if we have a var + const. - if (op2->OperGet() == GT_LCL_VAR) + + // Check if we have a var + const or var * const. + if (binop->OperIs(GT_ADD, GT_MUL) && op2->OperGet() == GT_LCL_VAR) { std::swap(op1, op2); } + if (op1->OperGet() != GT_LCL_VAR) { JITDUMP("Not monotonically increasing because op1 is not lclVar.\n"); @@ -356,7 +358,8 @@ bool RangeCheck::IsBinOpMonotonicallyIncreasing(GenTreeOp* binop) switch (op2->OperGet()) { case GT_LCL_VAR: - // When adding two local variables, we also must ensure that any constant is non-negative. + // When adding/multiplying/shifting two local variables, we also must ensure that any constant is + // non-negative. return IsMonotonicallyIncreasing(op1, true) && IsMonotonicallyIncreasing(op2, true); case GT_CNS_INT: @@ -417,7 +420,7 @@ bool RangeCheck::IsMonotonicallyIncreasing(GenTree* expr, bool rejectNegativeCon return (ssaDef != nullptr) && IsMonotonicallyIncreasing(ssaDef->GetAssignment()->gtGetOp2(), rejectNegativeConst); } - else if (expr->OperGet() == GT_ADD) + else if (expr->OperIs(GT_ADD, GT_MUL, GT_LSH)) { return IsBinOpMonotonicallyIncreasing(expr->AsOp()); } @@ -894,11 +897,12 @@ void RangeCheck::MergeAssertion(BasicBlock* block, GenTree* op, Range* pRange DE // Compute the range for a binary operation. Range RangeCheck::ComputeRangeForBinOp(BasicBlock* block, GenTreeOp* binop, bool monIncreasing DEBUGARG(int indent)) { - assert(binop->OperIs(GT_ADD, GT_AND, GT_RSH, GT_LSH, GT_UMOD)); + assert(binop->OperIs(GT_ADD, GT_AND, GT_RSH, GT_LSH, GT_UMOD, GT_MUL)); GenTree* op1 = binop->gtGetOp1(); GenTree* op2 = binop->gtGetOp2(); + // Special cases for binops where op2 is a constant if (binop->OperIs(GT_AND, GT_RSH, GT_LSH, GT_UMOD)) { if (!op2->IsIntCnsFitsInI32()) @@ -929,17 +933,21 @@ Range RangeCheck::ComputeRangeForBinOp(BasicBlock* block, GenTreeOp* binop, bool } } - if (icon < 0) + if (icon >= 0) + { + Range range(Limit(Limit::keConstant, 0), Limit(Limit::keConstant, icon)); + JITDUMP("Limit range to %s\n", range.ToString(m_pCompiler->getAllocatorDebugOnly())); + return range; + } + // Generalized range computation not implemented for these operators + else if (binop->OperIs(GT_AND, GT_UMOD, GT_RSH)) { return Range(Limit::keUnknown); } - Range range(Limit(Limit::keConstant, 0), Limit(Limit::keConstant, icon)); - JITDUMP("Limit range to %s\n", range.ToString(m_pCompiler->getAllocatorDebugOnly())); - return range; } // other operators are expected to be handled above. - assert(binop->OperIs(GT_ADD)); + assert(binop->OperIs(GT_ADD, GT_MUL, GT_LSH)); Range* op1RangeCached = nullptr; Range op1Range = Limit(Limit::keUndef); @@ -985,9 +993,30 @@ Range RangeCheck::ComputeRangeForBinOp(BasicBlock* block, GenTreeOp* binop, bool op2Range = *op2RangeCached; } - Range r = RangeOps::Add(op1Range, op2Range); - JITDUMP("BinOp add ranges %s %s = %s\n", op1Range.ToString(m_pCompiler->getAllocatorDebugOnly()), - op2Range.ToString(m_pCompiler->getAllocatorDebugOnly()), r.ToString(m_pCompiler->getAllocatorDebugOnly())); + Range r = Range(Limit::keUnknown); + if (binop->OperIs(GT_ADD)) + { + r = RangeOps::Add(op1Range, op2Range); + JITDUMP("BinOp add ranges %s %s = %s\n", op1Range.ToString(m_pCompiler->getAllocatorDebugOnly()), + op2Range.ToString(m_pCompiler->getAllocatorDebugOnly()), + r.ToString(m_pCompiler->getAllocatorDebugOnly())); + } + else if (binop->OperIs(GT_MUL)) + { + r = RangeOps::Multiply(op1Range, op2Range); + JITDUMP("BinOp multiply ranges %s %s = %s\n", op1Range.ToString(m_pCompiler->getAllocatorDebugOnly()), + op2Range.ToString(m_pCompiler->getAllocatorDebugOnly()), + r.ToString(m_pCompiler->getAllocatorDebugOnly())); + } + else if (binop->OperIs(GT_LSH)) + { + // help the next step a bit, convert the LSH rhs to a multiply + Range convertedOp2Range = RangeOps::ConvertShiftToMultiply(op2Range); + r = RangeOps::Multiply(op1Range, convertedOp2Range); + JITDUMP("BinOp multiply ranges %s %s = %s\n", op1Range.ToString(m_pCompiler->getAllocatorDebugOnly()), + convertedOp2Range.ToString(m_pCompiler->getAllocatorDebugOnly()), + r.ToString(m_pCompiler->getAllocatorDebugOnly())); + } return r; } @@ -1077,6 +1106,24 @@ bool RangeCheck::AddOverflows(Limit& limit1, Limit& limit2) return IntAddOverflows(max1, max2); } +// Check if the arithmetic overflows. +bool RangeCheck::MultiplyOverflows(Limit& limit1, Limit& limit2) +{ + int max1; + if (!GetLimitMax(limit1, &max1)) + { + return true; + } + + int max2; + if (!GetLimitMax(limit2, &max2)) + { + return true; + } + + return CheckedOps::MulOverflows(max1, max2, CheckedOps::Signed); +} + // Does the bin operation overflow. bool RangeCheck::DoesBinOpOverflow(BasicBlock* block, GenTreeOp* binop) { @@ -1109,10 +1156,15 @@ bool RangeCheck::DoesBinOpOverflow(BasicBlock* block, GenTreeOp* binop) JITDUMP("Checking bin op overflow %s %s\n", op1Range->ToString(m_pCompiler->getAllocatorDebugOnly()), op2Range->ToString(m_pCompiler->getAllocatorDebugOnly())); - if (!AddOverflows(op1Range->UpperLimit(), op2Range->UpperLimit())) + if (binop->OperIs(GT_ADD)) { - return false; + return AddOverflows(op1Range->UpperLimit(), op2Range->UpperLimit()); } + else if (binop->OperIs(GT_MUL)) + { + return MultiplyOverflows(op1Range->UpperLimit(), op2Range->UpperLimit()); + } + return true; } @@ -1180,7 +1232,7 @@ bool RangeCheck::ComputeDoesOverflow(BasicBlock* block, GenTree* expr) overflows = DoesVarDefOverflow(expr->AsLclVarCommon()); } // Check if add overflows. - else if (expr->OperGet() == GT_ADD) + else if (expr->OperGet() == GT_ADD || expr->OperGet() == GT_MUL) { overflows = DoesBinOpOverflow(block, expr->AsOp()); } @@ -1276,7 +1328,7 @@ Range RangeCheck::ComputeRange(BasicBlock* block, GenTree* expr, bool monIncreas MergeAssertion(block, expr, &range DEBUGARG(indent + 1)); } // compute the range for binary operation - else if (expr->OperIs(GT_ADD, GT_AND, GT_RSH, GT_LSH, GT_UMOD)) + else if (expr->OperIs(GT_ADD, GT_AND, GT_RSH, GT_LSH, GT_UMOD, GT_MUL)) { range = ComputeRangeForBinOp(block, expr->AsOp(), monIncreasing DEBUGARG(indent + 1)); } diff --git a/src/coreclr/jit/rangecheck.h b/src/coreclr/jit/rangecheck.h index 751b243740c40..7161aa2d162c4 100644 --- a/src/coreclr/jit/rangecheck.h +++ b/src/coreclr/jit/rangecheck.h @@ -147,6 +147,28 @@ struct Limit return false; } + bool MultiplyConstant(int i) + { + switch (type) + { + case keDependent: + return true; + case keBinOpArray: + case keConstant: + if (CheckedOps::MulOverflows(cns, i, CheckedOps::Signed)) + { + return false; + } + cns *= i; + return true; + case keUndef: + case keUnknown: + // For these values of 'type', conservatively return false + break; + } + + return false; + } bool Equals(Limit& l) { @@ -250,6 +272,21 @@ struct RangeOps } } + // Given a constant limit in "l1", multiply it to l2 and mutate "l2". + static Limit MultiplyConstantLimit(Limit& l1, Limit& l2) + { + assert(l1.IsConstant()); + Limit l = l2; + if (l.MultiplyConstant(l1.GetConstant())) + { + return l; + } + else + { + return Limit(Limit::keUnknown); + } + } + // Given two ranges "r1" and "r2", perform an add operation on the // ranges. static Range Add(Range& r1, Range& r2) @@ -291,6 +328,47 @@ struct RangeOps return result; } + // Given two ranges "r1" and "r2", perform an multiply operation on the + // ranges. + static Range Multiply(Range& r1, Range& r2) + { + Limit& r1lo = r1.LowerLimit(); + Limit& r1hi = r1.UpperLimit(); + Limit& r2lo = r2.LowerLimit(); + Limit& r2hi = r2.UpperLimit(); + + Range result = Limit(Limit::keUnknown); + + // Check lo ranges if they are dependent and not unknown. + if ((r1lo.IsDependent() && !r1lo.IsUnknown()) || (r2lo.IsDependent() && !r2lo.IsUnknown())) + { + result.lLimit = Limit(Limit::keDependent); + } + // Check hi ranges if they are dependent and not unknown. + if ((r1hi.IsDependent() && !r1hi.IsUnknown()) || (r2hi.IsDependent() && !r2hi.IsUnknown())) + { + result.uLimit = Limit(Limit::keDependent); + } + + if (r1lo.IsConstant()) + { + result.lLimit = MultiplyConstantLimit(r1lo, r2lo); + } + if (r2lo.IsConstant()) + { + result.lLimit = MultiplyConstantLimit(r2lo, r1lo); + } + if (r1hi.IsConstant()) + { + result.uLimit = MultiplyConstantLimit(r1hi, r2hi); + } + if (r2hi.IsConstant()) + { + result.uLimit = MultiplyConstantLimit(r2hi, r1hi); + } + return result; + } + // Given two ranges "r1" and "r2", do a Phi merge. If "monIncreasing" is true, // then ignore the dependent variables for the lower bound but not for the upper bound. static Range Merge(Range& r1, Range& r2, bool monIncreasing) @@ -378,6 +456,32 @@ struct RangeOps } return result; } + + // Given a Range C from an op (x << C), convert it to be used as + // (x * C'), where C' is a power of 2. + static Range ConvertShiftToMultiply(Range& r1) + { + Limit& r1lo = r1.LowerLimit(); + Limit& r1hi = r1.UpperLimit(); + + if (!r1lo.IsConstant() || !r1hi.IsConstant()) + { + return Limit(Limit::keUnknown); + } + + // Keep it simple for now, check if 0 <= C < 31 + int r1loConstant = r1lo.GetConstant(); + int r1hiConstant = r1hi.GetConstant(); + if (r1loConstant <= 0 || r1loConstant > 31 || r1hiConstant <= 0 || r1hiConstant > 31) + { + return Limit(Limit::keUnknown); + } + + Range result = Limit(Limit::keConstant); + result.lLimit = Limit(Limit::keConstant, 1 << r1loConstant); + result.uLimit = Limit(Limit::keConstant, 1 << r1hiConstant); + return result; + } }; class RangeCheck @@ -484,6 +588,9 @@ class RangeCheck // Does the addition of the two limits overflow? bool AddOverflows(Limit& limit1, Limit& limit2); + // Does the multiplication of the two limits overflow? + bool MultiplyOverflows(Limit& limit1, Limit& limit2); + // Does the binary operation between the operands overflow? Check recursively. bool DoesBinOpOverflow(BasicBlock* block, GenTreeOp* binop); diff --git a/src/tests/JIT/opt/AssertionPropagation/ArrBoundElim.cs b/src/tests/JIT/opt/AssertionPropagation/ArrBoundElim.cs new file mode 100644 index 0000000000000..8fd88e2e7715d --- /dev/null +++ b/src/tests/JIT/opt/AssertionPropagation/ArrBoundElim.cs @@ -0,0 +1,111 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.CompilerServices; + +class Program +{ + private static int returnCode = 100; + + public static int Main(string[] args) + { + RunTestThrows(Tests.MulOutsideRange); + RunTestThrows(Tests.MulOverflow); + RunTestThrows(Tests.LshOutsideRange); + + RunTestNoThrow(Tests.MulInsideRange); + RunTestNoThrow(Tests.LshInsideRange); + + return returnCode; + } + + private static void RunTestThrows(Action action) + { + try + { + action(); + Console.WriteLine("failed " + action.Method.Name); + returnCode--; + } + catch (Exception) + { + + } + } + + private static void RunTestNoThrow(Action action) + { + try + { + action(); + } + catch (Exception) + { + Console.WriteLine("failed " + action.Method.Name); + returnCode--; + } + } +} + +public static class Tests +{ + private static byte[] smallArr => new byte[10]; + + // RangeCheck analysis should eliminate the bounds check on + // smallArr. + public static void MulInsideRange() + { + for (int i = 0; i < 3; i++) + { + smallArr[i*3] = 17; + } + } + + // RangeCheck analysis should keep the bounds check on + // smallArr. + public static void MulOutsideRange() + { + for (int i = 0; i < 3; i++) + { + smallArr[i*5] = 17; + } + } + + private static byte[] bigArr => new byte[268435460]; + + // RangeCheck analysis should detect that the multiplcation + // overflows and keep all range checks for bigArr. bigArr + // size, and the bounds on the loop were carefully chosen to to + // potentially spoof the RangeCheck analysis to eliminate a bound + // check IF overflow detection on GT_MUL for RangeCheck is implemented + // incorrectly. + public static void MulOverflow() + { + for (int i = 0; i < 39768215; i++) + { + bigArr[i*402653184] = 17; + } + } + + // RangeCheck analysis should eliminate the bounds check on + // smallArr. + public static void LshInsideRange() + { + for (int i = 0; i < 3; i++) + { + smallArr[i<<1] = 17; + } + } + + // RangeCheck analysis should keep the bounds check on + // smallArr. + public static void LshOutsideRange() + { + for (int i = 0; i < 3; i++) + { + smallArr[i<<3] = 17; + } + } + +} diff --git a/src/tests/JIT/opt/AssertionPropagation/ArrBoundElim.csproj b/src/tests/JIT/opt/AssertionPropagation/ArrBoundElim.csproj new file mode 100644 index 0000000000000..e5ad660f3d500 --- /dev/null +++ b/src/tests/JIT/opt/AssertionPropagation/ArrBoundElim.csproj @@ -0,0 +1,13 @@ + + + Exe + + + None + True + + + + + +