Skip to content

Fix range check upper limit. #198

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

Merged
sandreenko merged 6 commits into from
Nov 27, 2019
Merged

Fix range check upper limit. #198

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
1860741
add a new test case to osr015.
Nov 21, 2019
4239890
Improve JitDump output for range checks.
Nov 21, 2019
9ef0ca3
Rename `monotonic` to `monIncreasing`.
Nov 21, 2019
396198f
Fix the bug.
Nov 21, 2019
36ad47d
Fix the test.
Nov 22, 2019
e1d2796
Fix comments/printings.
Nov 22, 2019
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
61 changes: 38 additions & 23 deletions src/coreclr/src/jit/rangecheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,9 +313,9 @@ void RangeCheck::Widen(BasicBlock* block, GenTree* tree, Range* pRange)
{
// To determine the lower bound, ask if the loop increases monotonically.
bool increasing = IsMonotonicallyIncreasing(tree, false);
JITDUMP("IsMonotonicallyIncreasing %d", increasing);
if (increasing)
{
JITDUMP("[%06d] is monotonically increasing.\n", Compiler::dspTreeID(tree));
GetRangeMap()->RemoveAll();
*pRange = GetRange(block, tree, true DEBUGARG(0));
}
Expand All @@ -338,7 +338,7 @@ bool RangeCheck::IsBinOpMonotonicallyIncreasing(GenTreeOp* binop)
}
if (op1->OperGet() != GT_LCL_VAR)
{
JITDUMP("Not monotonic because op1 is not lclVar.\n");
JITDUMP("Not monotonically increasing because op1 is not lclVar.\n");
return false;
}
switch (op2->OperGet())
Expand All @@ -351,7 +351,7 @@ bool RangeCheck::IsBinOpMonotonicallyIncreasing(GenTreeOp* binop)
return (op2->AsIntConCommon()->IconValue() >= 0) && IsMonotonicallyIncreasing(op1, false);

default:
JITDUMP("Not monotonic because expression is not recognized.\n");
JITDUMP("Not monotonically increasing because expression is not recognized.\n");
return false;
}
}
Expand Down Expand Up @@ -414,7 +414,7 @@ bool RangeCheck::IsMonotonicallyIncreasing(GenTree* expr, bool rejectNegativeCon
}
if (!IsMonotonicallyIncreasing(use.GetNode(), rejectNegativeConst))
{
JITDUMP("Phi argument not monotonic\n");
JITDUMP("Phi argument not monotonically increasing\n");
return false;
}
}
Expand Down Expand Up @@ -771,7 +771,7 @@ 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 monotonic DEBUGARG(int indent))
Range RangeCheck::ComputeRangeForBinOp(BasicBlock* block, GenTreeOp* binop, bool monIncreasing DEBUGARG(int indent))
{
assert(binop->OperIs(GT_ADD));

Expand All @@ -791,7 +791,7 @@ Range RangeCheck::ComputeRangeForBinOp(BasicBlock* block, GenTreeOp* binop, bool
}
else
{
op1Range = GetRange(block, op1, monotonic DEBUGARG(indent));
op1Range = GetRange(block, op1, monIncreasing DEBUGARG(indent));
}
MergeAssertion(block, op1, &op1Range DEBUGARG(indent + 1));
}
Expand All @@ -813,7 +813,7 @@ Range RangeCheck::ComputeRangeForBinOp(BasicBlock* block, GenTreeOp* binop, bool
}
else
{
op2Range = GetRange(block, op2, monotonic DEBUGARG(indent));
op2Range = GetRange(block, op2, monIncreasing DEBUGARG(indent));
}
MergeAssertion(block, op2, &op2Range DEBUGARG(indent + 1));
}
Expand All @@ -831,7 +831,7 @@ Range RangeCheck::ComputeRangeForBinOp(BasicBlock* block, GenTreeOp* binop, bool
// Compute the range for a local var definition.
Range RangeCheck::ComputeRangeForLocalDef(BasicBlock* block,
GenTreeLclVarCommon* lcl,
bool monotonic DEBUGARG(int indent))
bool monIncreasing DEBUGARG(int indent))
{
LclSsaVarDsc* ssaDef = GetSsaDefAsg(lcl);
if (ssaDef == nullptr)
Expand All @@ -846,7 +846,7 @@ Range RangeCheck::ComputeRangeForLocalDef(BasicBlock* block,
JITDUMP("----------------------------------------------------\n");
}
#endif
Range range = GetRange(ssaDef->GetBlock(), ssaDef->GetAssignment()->gtGetOp2(), monotonic DEBUGARG(indent));
Range range = GetRange(ssaDef->GetBlock(), ssaDef->GetAssignment()->gtGetOp2(), monIncreasing DEBUGARG(indent));
if (!BitVecOps::MayBeUninit(block->bbAssertionIn))
{
JITDUMP("Merge assertions from " FMT_BB ":%s for assignment about [%06d]\n", block->bbNum,
Expand Down Expand Up @@ -1039,17 +1039,32 @@ bool RangeCheck::ComputeDoesOverflow(BasicBlock* block, GenTree* expr)
}
GetOverflowMap()->Set(expr, overflows, OverflowMap::Overwrite);
m_pSearchPath->Remove(expr);
JITDUMP("[%06d] %s\n", Compiler::dspTreeID(expr), ((overflows) ? "overflows" : "does not overflow"));
return overflows;
}

// Compute the range recursively by asking for the range of each variable in the dependency chain.
// eg.: c = a + b; ask range of "a" and "b" and add the results.
// If the result cannot be determined i.e., the dependency chain does not terminate in a value,
// but continues to loop, which will happen with phi nodes. We end the looping by calling the
// value as "dependent" (dep).
// If the loop is proven to be "monotonic", then make liberal decisions while merging phi node.
// eg.: merge((0, dep), (dep, dep)) = (0, dep)
Range RangeCheck::ComputeRange(BasicBlock* block, GenTree* expr, bool monotonic DEBUGARG(int indent))
//------------------------------------------------------------------------
// ComputeRange: Compute the range recursively by asking for the range of each variable in the dependency chain.
//
// Arguments:
// block - the block that contains `expr`;
// expr - expression to compute the range for;
// monIncreasing - true if `expr` is proven to be monotonically increasing;
// indent - debug printing indent.
//
// Return value:
// 'expr' range as lower and upper limits.
//
// Notes:
// eg.: c = a + b; ask range of "a" and "b" and add the results.
// If the result cannot be determined i.e., the dependency chain does not terminate in a value,
// but continues to loop, which will happen with phi nodes we end the looping by calling the
// value as "dependent" (dep).
// If the loop is proven to be "monIncreasing", then make liberal decisions for the lower bound
// while merging phi node. eg.: merge((0, dep), (dep, dep)) = (0, dep),
// merge((0, 1), (dep, dep)) = (0, dep), merge((0, 5), (dep, 10)) = (0, 10).
//
Range RangeCheck::ComputeRange(BasicBlock* block, GenTree* expr, bool monIncreasing DEBUGARG(int indent))
{
bool newlyAdded = !m_pSearchPath->Set(expr, block, SearchPath::Overwrite);
Range range = Limit(Limit::keUndef);
Expand Down Expand Up @@ -1099,13 +1114,13 @@ Range RangeCheck::ComputeRange(BasicBlock* block, GenTree* expr, bool monotonic
// If local, find the definition from the def map and evaluate the range for rhs.
else if (expr->IsLocal())
{
range = ComputeRangeForLocalDef(block, expr->AsLclVarCommon(), monotonic DEBUGARG(indent + 1));
range = ComputeRangeForLocalDef(block, expr->AsLclVarCommon(), monIncreasing DEBUGARG(indent + 1));
MergeAssertion(block, expr, &range DEBUGARG(indent + 1));
}
// If add, then compute the range for the operands and add them.
else if (expr->OperGet() == GT_ADD)
{
range = ComputeRangeForBinOp(block, expr->AsOp(), monotonic DEBUGARG(indent + 1));
range = ComputeRangeForBinOp(block, expr->AsOp(), monIncreasing DEBUGARG(indent + 1));
}
// If phi, then compute the range for arguments, calling the result "dependent" when looping begins.
else if (expr->OperGet() == GT_PHI)
Expand All @@ -1120,14 +1135,14 @@ Range RangeCheck::ComputeRange(BasicBlock* block, GenTree* expr, bool monotonic
}
else
{
argRange = GetRange(block, use.GetNode(), monotonic DEBUGARG(indent + 1));
argRange = GetRange(block, use.GetNode(), monIncreasing DEBUGARG(indent + 1));
}
assert(!argRange.LowerLimit().IsUndef());
assert(!argRange.UpperLimit().IsUndef());
MergeAssertion(block, use.GetNode(), &argRange DEBUGARG(indent + 1));
JITDUMP("Merging ranges %s %s:", range.ToString(m_pCompiler->getAllocatorDebugOnly()),
argRange.ToString(m_pCompiler->getAllocatorDebugOnly()));
range = RangeOps::Merge(range, argRange, monotonic);
range = RangeOps::Merge(range, argRange, monIncreasing);
JITDUMP("%s\n", range.ToString(m_pCompiler->getAllocatorDebugOnly()));
}
}
Expand Down Expand Up @@ -1176,7 +1191,7 @@ void Indent(int indent)
#endif

// Get the range, if it is already computed, use the cached range value, else compute it.
Range RangeCheck::GetRange(BasicBlock* block, GenTree* expr, bool monotonic DEBUGARG(int indent))
Range RangeCheck::GetRange(BasicBlock* block, GenTree* expr, bool monIncreasing DEBUGARG(int indent))
{
#ifdef DEBUG
if (m_pCompiler->verbose)
Expand All @@ -1191,7 +1206,7 @@ Range RangeCheck::GetRange(BasicBlock* block, GenTree* expr, bool monotonic DEBU

Range* pRange = nullptr;
Range range =
GetRangeMap()->Lookup(expr, &pRange) ? *pRange : ComputeRange(block, expr, monotonic DEBUGARG(indent));
GetRangeMap()->Lookup(expr, &pRange) ? *pRange : ComputeRange(block, expr, monIncreasing DEBUGARG(indent));

#ifdef DEBUG
if (m_pCompiler->verbose)
Expand Down
37 changes: 15 additions & 22 deletions src/coreclr/src/jit/rangecheck.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@
//
// **Step 4. Check if the dependency chain is monotonic.
//
// **Step 5. If monotonic is true, then perform a widening step, where we assume, the
// SSA variables that are "dependent" get their values from the definitions in the
// **Step 5. If monotonic increasing is true, then perform a widening step, where we assume, the
Copy link
Member

@AndyAyersMS AndyAyersMS Nov 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit from pre-existing comment: I think this really should be called a "narrowing step" as (dep,dep) abstractly describes a wider set of values than (0,dep).

Copy link
Contributor Author

@sandreenko sandreenko Nov 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In classic compiler books that step is called "widening" (or widening operator) because it also sets the upper limit to +INF.
I do not see any immediate benefits if we start replacing (dep) with (inf) right now. However, I think it is better to keep the preexisting name so people can find the articles that the algorithm was based on.

Copy link
Member

@AndyAyersMS AndyAyersMS Nov 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so people can find the articles that the algorithm was based on.

Perhaps we could mention those articles in a comment? I am curious to learn if range check is actually based on any sort of analysis formalism.

Copy link
Contributor Author

@sandreenko sandreenko Nov 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is hard for me to find the first article that describes that part of the interval analysis. A good example that is available in preview is in "Compiler Design: Analysis and Transformation, Helmut Seidl, Reinhard Wilhelm, Sebastian Hack" . There are earlier articles that have more mathematical formalism but they are far from our implementation.

// SSA variables that are "dependent" get their lower bound values from the definitions in the
// dependency loop and their initial values must be the definitions that are not in
// the dependency loop, in this case i_0's value which is 0.
//
Expand Down Expand Up @@ -292,9 +292,9 @@ struct RangeOps
return result;
}

// Given two ranges "r1" and "r2", do a Phi merge. If "monotonic" is true,
// then ignore the dependent variables.
static Range Merge(Range& r1, Range& r2, bool monotonic)
// 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)
{
Limit& r1lo = r1.LowerLimit();
Limit& r1hi = r1.UpperLimit();
Expand All @@ -314,7 +314,7 @@ struct RangeOps
}
else if (r1lo.IsDependent() || r2lo.IsDependent())
{
if (monotonic)
if (monIncreasing)
{
result.lLimit = r1lo.IsDependent() ? r2lo : r1lo;
}
Expand All @@ -335,14 +335,7 @@ struct RangeOps
}
else if (r1hi.IsDependent() || r2hi.IsDependent())
{
if (monotonic)
{
result.uLimit = r1hi.IsDependent() ? r2hi : r1hi;
}
else
{
result.uLimit = Limit(Limit::keDependent);
}
result.uLimit = Limit(Limit::keDependent);
}

if (r1lo.IsConstant() && r2lo.IsConstant())
Expand Down Expand Up @@ -460,19 +453,19 @@ class RangeCheck
// Given the index expression try to find its range.
// The range of a variable depends on its rhs which in turn depends on its constituent variables.
// The "path" is the path taken in the search for the rhs' range and its constituents' range.
// If "monotonic" is true, the calculations are made more liberally assuming initial values
// at phi definitions.
Range GetRange(BasicBlock* block, GenTree* expr, bool monotonic DEBUGARG(int indent));
// If "monIncreasing" is true, the calculations are made more liberally assuming initial values
// at phi definitions for the lower bound.
Range GetRange(BasicBlock* block, GenTree* expr, bool monIncreasing DEBUGARG(int indent));

// Given the local variable, first find the definition of the local and find the range of the rhs.
// Helper for GetRange.
Range ComputeRangeForLocalDef(BasicBlock* block, GenTreeLclVarCommon* lcl, bool monotonic DEBUGARG(int indent));
Range ComputeRangeForLocalDef(BasicBlock* block, GenTreeLclVarCommon* lcl, bool monIncreasing DEBUGARG(int indent));

// Compute the range, rather than retrieve a cached value. Helper for GetRange.
Range ComputeRange(BasicBlock* block, GenTree* expr, bool monotonic DEBUGARG(int indent));
Range ComputeRange(BasicBlock* block, GenTree* expr, bool monIncreasing DEBUGARG(int indent));

// Compute the range for the op1 and op2 for the given binary operator.
Range ComputeRangeForBinOp(BasicBlock* block, GenTreeOp* binop, bool monotonic DEBUGARG(int indent));
Range ComputeRangeForBinOp(BasicBlock* block, GenTreeOp* binop, bool monIncreasing DEBUGARG(int indent));

// Merge assertions from AssertionProp's flags, for the corresponding "phiArg."
// Requires "pRange" to contain range that is computed partially.
Expand Down Expand Up @@ -504,8 +497,8 @@ class RangeCheck
// Does the current "expr" which is a use involve a definition, that overflows.
bool DoesOverflow(BasicBlock* block, GenTree* tree);

// Widen the range by first checking if the induction variable is monotonic. Requires "pRange"
// to be partially computed.
// Widen the range by first checking if the induction variable is monotonically increasing.
// Requires "pRange" to be partially computed.
void Widen(BasicBlock* block, GenTree* tree, Range* pRange);

// Is the binary operation increasing the value.
Expand Down
72 changes: 72 additions & 0 deletions src/coreclr/tests/src/JIT/jit64/opt/osr/osr015.il
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1485,6 +1485,29 @@ TRY_END_b029:
TRY_END_b030:


.try
{
call void test::test_001c()
ldstr "Fail: test_001c"
call void [System.Console]System.Console::WriteLine(string)
ldc.i4 1
stloc 0
leave TRY_END_c001
}
catch [mscorlib]System.IndexOutOfRangeException
{
leave TRY_END_c001
}
catch [mscorlib]System.Exception
{
ldstr "Fail: test_001c"
call void [System.Console]System.Console::WriteLine(string)
call void [System.Console]System.Console::WriteLine(object)
ldc.i4 1
stloc 0
leave TRY_END_c001
}
TRY_END_c001:



Expand Down Expand Up @@ -4138,6 +4161,55 @@ FOR_TEST:



.method public static void test_001c() cil managed
{
// Check for exception
// use int32
// Exceed array bounds with greater than
// with a back branch that goes to a try region
// so Jit can't clone the loop check and transofm it
// to a postcondition loop.


.locals init (int32[], int32)


ldc.i4 100
newarr int32
stloc 0


FOR_START:
ldloc 1
ldc.i4 100
bgt FOR_END

.try
{
ldloc 0
ldloc 1
ldloc 1
stelem.i4
leave TRY_END_a
}
catch [mscorlib]System.NullReferenceException
{
leave TRY_END_a
}
TRY_END_a:

ldloc 1
ldc.i4 1
add
stloc 1
br FOR_START

FOR_END:

ret

}




Expand Down