Skip to content

Commit

Permalink
Add additional binary operations into the RangeCheck analysis. (#61662)
Browse files Browse the repository at this point in the history 
* 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<byte> 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 <egorbo@gmail.com>

* Update src/coreclr/jit/rangecheck.cpp

Co-authored-by: Egor Bogatov <egorbo@gmail.com>

Co-authored-by: Egor Bogatov <egorbo@gmail.com>
  • Loading branch information
@anthonycanino @EgorBo
anthonycanino and EgorBo authored Dec 9, 2021
1 parent 72e0da8 commit fd28c76
Show file tree
Hide file tree
Showing 4 changed files with 301 additions and 18 deletions.
88 changes: 70 additions & 18 deletions src/coreclr/jit/rangecheck.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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:
Expand Down Expand Up @@ -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());
}
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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());
}
Expand Down Expand Up @@ -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));
}
Expand Down
107 changes: 107 additions & 0 deletions src/coreclr/jit/rangecheck.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);

Expand Down
Loading

0 comments on commit fd28c76

Please sign in to comment.