Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add additional binary operations into the RangeCheck analysis. #61662

Merged
merged 6 commits into from
Dec 9, 2021
Merged

Add additional binary operations into the RangeCheck analysis. #61662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ebc2288
Add additional binary operations into the RangeCheck analysis.
anthonycanino Nov 9, 2021
162ed95
RangeCheck multiply overflow fix and tests.
anthonycanino Nov 18, 2021
33a3b43
Use CheckedOps::Signed instead of false.
anthonycanino Nov 18, 2021
7872399
Reduced overflow test array size to more reasonable value.
anthonycanino Nov 19, 2021
9b4470c
Update src/coreclr/jit/rangecheck.cpp
anthonycanino Dec 6, 2021
01e4faa
Update src/coreclr/jit/rangecheck.cpp
anthonycanino Dec 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 70 additions & 18 deletions src/coreclr/jit/rangecheck.cpp
View file
Open in desktop
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
Open in desktop
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