Extend Range analysis to 64-bit range and mint operations

Extend Range analysis to 64-bit range and mint operations

- Convert many private static functions into public static methods on Range or RangeBoundary classes.
- Simplify ConstraintInstr::InferRange to just use RangeBoundary::Min and RangeBoundary::Max calls.
- Extend RangeBoundary to 64-bits.
- Include mints in RangeAnalysis.
- Introduce generic Range::BinaryOp.
- Require ranges passed to Range::BinaryOp are finite.
- Clamp ranges attached to BinarySmiOpInstr and BinaryMintOpInstr.
- InferRange for BinaryMintOp and UnboxInteger instructions.
- Correctly set mint constant ranges.
- Add many tests.

R=fschneider@google.com, vegorov@google.com

Committed: https://code.google.com/p/dart/source/detail?r=37521

[Cutch, 2014-06-10 16:37:51]

PTAL.

[Florian Schneider, 2014-06-11 10:41:01]

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2664: intptr_t RangeBoundary::Value() const
{
maybe add ASSERT(kind_ == kConstant);

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2666: return kMin;
I'm not sure this is ideal to return kMin/kMax. A constant range may also have
the same boundary value, since we want to represent constant range boundaries
with the min-int and max-int values, and therefore the value returned here can't
be used to compare two range boundaries.

Maybe better add UNREACHABLE() here?

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2669: return kMax;
UNREACHABLE()?

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:3174: ((range->min().IsConstant() ||
range->min().IsInfinity()) &&
IsNegativeInfinity()?

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:3175: (range->min().Value() <=
Smi::kMinValue));
Same comment as below.

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:3181: ((range->max().IsConstant() ||
range->max().IsInfinity()) &&
IsPositiveInfinity?

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:3182: (range->max().Value() >=
Smi::kMaxValue));
This is a piece of code that gets hard to read because of parenthesiszation... a
case where imo our 'style-guide' gets in the way.

Value() is not a valid boundary if the RangeBoundary is +/- infinity. I think
the condition rather should be:

range == NULL ||
range->max().IsPositiveInfinity() ||
(range->max().IsConstant() && range.max().Value >= Smi::kMaxValue)

[Vyacheslav Egorov (Google), 2014-06-11 17:36:37]

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2466: RangeBoundary
RangeBoundary::LowerBound() const {
I don't think having PositiveInfinity.LowerBound() == PositiveInfinity makes
sense. It should be ether undefined (assert?) or be safely equal to
NegativeInfinity.

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2475: if (IsConstantOrInfinity()) return
*this;
Ditto.

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2666: return kMin;
I second Florian here. Let's prohibit asking a value of unclamped infinity.

On 2014/06/11 10:41:00, Florian Schneider wrote:
> I'm not sure this is ideal to return kMin/kMax. A constant range may also have
> the same boundary value, since we want to represent constant range boundaries
> with the min-int and max-int values, and therefore the value returned here
can't
> be used to compare two range boundaries.
> 
> Maybe better add UNREACHABLE() here?

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2700: if
(Range::IsSmiMinimumOrUnderflow(value_range) &&
for simplicity I suggest we make this code different: just check for infinities,
don't check for `OrUnderflow`. 

Those ranges should never be underflowing, they should be clamped to the
effective value range (smi).

[Vyacheslav Egorov (Google), 2014-06-11 20:06:37]

more comments

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2700: if
(Range::IsSmiMinimumOrUnderflow(value_range) &&
I wonder if this code will become better if we introduce 

RangeBoundary::Max() and RangeBoundary::Min() that can work both on symbols,
infinities and constants. Then you just use that to compute RangeBoundary for
min and max of the range.

On 2014/06/11 17:36:37, Vyacheslav Egorov (Google) wrote:
> for simplicity I suggest we make this code different: just check for
infinities,
> don't check for `OrUnderflow`. 
> 
> Those ranges should never be underflowing, they should be clamped to the
> effective value range (smi).
> 
> 
> 
>

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2997: static bool SymbolicSub(const
RangeBoundary& a,
This funciton should be moved to RangeBoundary?

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:3059: bool should_update_overflow = range_
!= NULL;
I don't understand the reasoning behind this lack of update on the first call.
We will get out of sync. The old code did not have this behavior.

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:3187: bool Range::OnlyPositiveOrZero(const
Range& a, const Range& b) {
It feels like we have some kind of duplication here. We already have similar
check for range boundaries. Can that be used instead?

[Cutch, 2014-06-13 04:24:26]

Addressed both of your comments.

PTAL.

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2664: intptr_t RangeBoundary::Value() const
{
On 2014/06/11 10:41:00, Florian Schneider wrote:
> maybe add ASSERT(kind_ == kConstant);

Done.

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2666: return kMin;
On 2014/06/11 17:36:37, Vyacheslav Egorov (Google) wrote:
> I second Florian here. Let's prohibit asking a value of unclamped infinity.
> 
> On 2014/06/11 10:41:00, Florian Schneider wrote:
> > I'm not sure this is ideal to return kMin/kMax. A constant range may also
have
> > the same boundary value, since we want to represent constant range
boundaries
> > with the min-int and max-int values, and therefore the value returned here
> can't
> > be used to compare two range boundaries.
> > 
> > Maybe better add UNREACHABLE() here?
> 

Done.

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2669: return kMax;
On 2014/06/11 10:41:00, Florian Schneider wrote:
> UNREACHABLE()?

Done.

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2700: if
(Range::IsSmiMinimumOrUnderflow(value_range) &&
On 2014/06/11 17:36:37, Vyacheslav Egorov (Google) wrote:
> for simplicity I suggest we make this code different: just check for
infinities,
> don't check for `OrUnderflow`. 
> 
> Those ranges should never be underflowing, they should be clamped to the
> effective value range (smi).
> 
> 
> 
> 

Done.

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2700: if
(Range::IsSmiMinimumOrUnderflow(value_range) &&
On 2014/06/11 20:06:37, Vyacheslav Egorov (Google) wrote:
> I wonder if this code will become better if we introduce 
> 
> RangeBoundary::Max() and RangeBoundary::Min() that can work both on symbols,
> infinities and constants. Then you just use that to compute RangeBoundary for
> min and max of the range.
> 
> On 2014/06/11 17:36:37, Vyacheslav Egorov (Google) wrote:
> > for simplicity I suggest we make this code different: just check for
> infinities,
> > don't check for `OrUnderflow`. 
> > 
> > Those ranges should never be underflowing, they should be clamped to the
> > effective value range (smi).
> > 
> > 
> > 
> > 
> 

I've extended Min and Max to handle infinities but I'm not sure about putting
the else code paths into Min and Max- there is canonicalization going on with
opposite boundaries.

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2997: static bool SymbolicSub(const
RangeBoundary& a,
On 2014/06/11 20:06:36, Vyacheslav Egorov (Google) wrote:
> This funciton should be moved to RangeBoundary?

Done.

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:3059: bool should_update_overflow = range_
!= NULL;
On 2014/06/11 20:06:37, Vyacheslav Egorov (Google) wrote:
> I don't understand the reasoning behind this lack of update on the first call.
> We will get out of sync. The old code did not have this behavior.

I've updated the predicate to include the + and - case.

https://codereview.chromium.org/328503003/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:3187: bool Range::OnlyPositiveOrZero(const
Range& a, const Range& b) {
On 2014/06/11 20:06:36, Vyacheslav Egorov (Google) wrote:
> It feels like we have some kind of duplication here. We already have similar
> check for range boundaries. Can that be used instead?

Done.

[Cutch, 2014-06-13 04:27:59]

BTW,

I noticed that in ConstraintInstr::InferRange when computing the canonicalized
min value we don't break out of the loop (see line 2795) but when computing the
max value we do break out of the loop (see 2829). Is this intentional?

[Vyacheslav Egorov (Google), 2014-06-13 11:50:21]

https://codereview.chromium.org/328503003/diff/60001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/328503003/diff/60001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2533: const intptr_t offset = b.offset() +
a.ConstantValue();
return SymbolicAdd(b, a, result); to avoid duplicating code.

https://codereview.chromium.org/328503003/diff/60001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2733: if (DependOnSameSymbol(a, b)) {
I would move loop that tries to Canonicalize boundaries before comparing them
symbolically here.

https://codereview.chromium.org/328503003/diff/60001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2774: if
(Range::IncludesNegativeInfinity(value_range) &&
I would expect this code to actually use RangeBoundary::Max() why is this not
the case?

https://codereview.chromium.org/328503003/diff/60001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2795: : canonical_a;
I agree this should have break.

https://codereview.chromium.org/328503003/diff/60001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2807: if
(Range::IncludesPositiveInfinity(value_range) &&
I would expect this to be replaced by RangeBoundary::Min

https://codereview.chromium.org/328503003/diff/60001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:3181: if (max().UpperBound().ConstantValue()
< val) {
min().LowerBound().ConstantValue() >= val && max().UpperBound().ConstantValue()
< val 

is an impossible (empty) range.

[Cutch, 2014-06-13 22:45:22]

PTAL

https://codereview.chromium.org/328503003/diff/60001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/328503003/diff/60001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2533: const intptr_t offset = b.offset() +
a.ConstantValue();
On 2014/06/13 11:50:21, Vyacheslav Egorov (Google) wrote:
> return SymbolicAdd(b, a, result); to avoid duplicating code.

Done.

https://codereview.chromium.org/328503003/diff/60001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:2807: if
(Range::IncludesPositiveInfinity(value_range) &&
On 2014/06/13 11:50:21, Vyacheslav Egorov (Google) wrote:
> I would expect this to be replaced by RangeBoundary::Min

Done.

[Cutch, 2014-06-15 23:06:52]

I've extended this CL to include mint range analysis.

[Florian Schneider, 2014-06-17 10:50:11]

https://codereview.chromium.org/328503003/diff/200001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_test.cc (right):

https://codereview.chromium.org/328503003/diff/200001/runtime/vm/intermediate...
runtime/vm/intermediate_language_test.cc:272:
EXPECT(range_b->min().IsNegativeInfinity());
Where does it make sense to have +/- infinity as an input range?

If an operation has an infinity as a result range, it signals that it may
overflow. But I think the range will always be clamped before it's  used as
input in another operation.

How about asserting that input ranges are always finite?

https://codereview.chromium.org/328503003/diff/200001/runtime/vm/intermediate...
runtime/vm/intermediate_language_test.cc:312: #define TEST_RANGE_ADD(l_min,
l_max, r_min, r_max, result_min, result_max)     \
Can you re-use the TEST_RANGE_OP macro from above here? or something similar?

https://codereview.chromium.org/328503003/diff/200001/runtime/vm/intermediate...
runtime/vm/intermediate_language_test.cc:360: // [kMaxInt64, kMaxInt32 + 15] +
[10, kMaxInt64] = [-inf, kMaxInt32 + 35].
Lower bound should be smaller than upper bound.

Maybe s/kMaxInt64/kMinInt64/ here ?

https://codereview.chromium.org/328503003/diff/200001/runtime/vm/intermediate...
runtime/vm/intermediate_language_test.cc:415:
TEST_RANGE_SUB(static_cast<int64_t>(kMaxInt32),
Maybe move the static_cast<int64_t> into the macro for conciseness?

https://codereview.chromium.org/328503003/diff/200001/runtime/vm/intermediate...
runtime/vm/intermediate_language_test.cc:436:
RangeBoundary::PositiveInfinity());
I'd add also the test cases that you have for ADD to verify that

[a, b] - [c, d] results the same as [a, b] + [-d, -c]

https://codereview.chromium.org/328503003/diff/200001/runtime/vm/intermediate...
runtime/vm/intermediate_language_test.cc:498:
RangeBoundary(static_cast<int64_t>(0xffffffff)));
Please add a test case where both, left and right range can be negative: e.g.
[-20, 20] & [-20, 20]

[Cutch, 2014-06-17 21:49:04]

https://codereview.chromium.org/328503003/diff/200001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_test.cc (right):

https://codereview.chromium.org/328503003/diff/200001/runtime/vm/intermediate...
runtime/vm/intermediate_language_test.cc:272:
EXPECT(range_b->min().IsNegativeInfinity());
On 2014/06/17 10:50:11, Florian Schneider wrote:
> Where does it make sense to have +/- infinity as an input range?
> 
> If an operation has an infinity as a result range, it signals that it may
> overflow. But I think the range will always be clamped before it's  used as
> input in another operation.
> 
> How about asserting that input ranges are always finite?

Agreed. I've modified Range::BinaryOp to assert that both the left and right
ranges != +/- infinity.

I've also modified BinarySmiOpInstr and BinaryMintOpInstr to clamp the resulting
ranges. In the case of BinarySmiOpInstr I first determine if an overflow has
occurred before clamping the range.

https://codereview.chromium.org/328503003/diff/200001/runtime/vm/intermediate...
runtime/vm/intermediate_language_test.cc:312: #define TEST_RANGE_ADD(l_min,
l_max, r_min, r_max, result_min, result_max)     \
On 2014/06/17 10:50:11, Florian Schneider wrote:
> Can you re-use the TEST_RANGE_OP macro from above here? or something similar?

I can't reuse for add and subtract because they take an extra argument
(Definition* left_defn).

https://codereview.chromium.org/328503003/diff/200001/runtime/vm/intermediate...
runtime/vm/intermediate_language_test.cc:360: // [kMaxInt64, kMaxInt32 + 15] +
[10, kMaxInt64] = [-inf, kMaxInt32 + 35].
On 2014/06/17 10:50:10, Florian Schneider wrote:
> Lower bound should be smaller than upper bound.
> 
> Maybe s/kMaxInt64/kMinInt64/ here ?

Done.

https://codereview.chromium.org/328503003/diff/200001/runtime/vm/intermediate...
runtime/vm/intermediate_language_test.cc:415:
TEST_RANGE_SUB(static_cast<int64_t>(kMaxInt32),
On 2014/06/17 10:50:10, Florian Schneider wrote:
> Maybe move the static_cast<int64_t> into the macro for conciseness?

I can't because kMaxInt32 is an int32_t and it must be cast to int64_t before
the addition.

https://codereview.chromium.org/328503003/diff/200001/runtime/vm/intermediate...
runtime/vm/intermediate_language_test.cc:436:
RangeBoundary::PositiveInfinity());
On 2014/06/17 10:50:10, Florian Schneider wrote:
> I'd add also the test cases that you have for ADD to verify that
> 
> [a, b] - [c, d] results the same as [a, b] + [-d, -c]

Done.

https://codereview.chromium.org/328503003/diff/200001/runtime/vm/intermediate...
runtime/vm/intermediate_language_test.cc:498:
RangeBoundary(static_cast<int64_t>(0xffffffff)));
On 2014/06/17 10:50:10, Florian Schneider wrote:
> Please add a test case where both, left and right range can be negative: e.g.
> [-20, 20] & [-20, 20]

Done.

[Florian Schneider, 2014-06-18 10:14:10]

Lgtm.

https://codereview.chromium.org/328503003/diff/220001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/328503003/diff/220001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:2487: return overflow;
Can this case ever occur?

https://codereview.chromium.org/328503003/diff/220001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:2506: return overflow;
Is this case ever hit?

[Vyacheslav Egorov (Google), 2014-06-18 11:21:59]

not lgtm

in C++ signed values overflow is an *undefined behavior* so optimizer is free to
assume that stuff like ((a < 0) && (b < 0) && ((a + b) > 0)) can never be true.

Chromium contains safe and portable checked numerics in src/base/numerics/
consider either using that or writing an assembly for all supported platforms.

[Cutch, 2014-06-18 22:16:27]

On 2014/06/18 11:21:59, Vyacheslav Egorov (Google) wrote:
> not lgtm
> 
> in C++ signed values overflow is an *undefined behavior* so optimizer is free
to
> assume that stuff like ((a < 0) && (b < 0) && ((a + b) > 0)) can never be
true.
> 
> Chromium contains safe and portable checked numerics in src/base/numerics/
> consider either using that or writing an assembly for all supported platforms.

I've modified the checks so they are done before the operations. The logic for
those checks came from a CERT article:

https://www.securecoding.cert.org/confluence/display/seccode/INT32-C.+Ensure+...

I also have many unit tests which cover the addition and subtraction overflow
cases.

[Cutch, 2014-06-18 22:16:52]

https://codereview.chromium.org/328503003/diff/220001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/328503003/diff/220001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:2487: return overflow;
On 2014/06/18 10:14:10, Florian Schneider wrote:
> Can this case ever occur?

Not now that I clamp.

https://codereview.chromium.org/328503003/diff/220001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:2506: return overflow;
On 2014/06/18 10:14:10, Florian Schneider wrote:
> Is this case ever hit?

Not now that I clamp.

[Vyacheslav Egorov (Google), 2014-06-19 17:27:31]

lgtm

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:5106: // Initialize bitvector for quick
filtering of smi values.
s/smi/int/

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:2874: if (unboxed == NULL) {
When is this true? How can we be unboxing "nothing"?

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:2899: RangeBoundary::kRangeBoundarySmi);
Add an assertion that value()->definition() is a Smi?

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:2986: range_ = new
Range(RangeBoundary::FromConstant(kMinInt32),
CanDeoptimize() versions of Int32/Uint32 loads should be return values within a
Smi range.

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:3107: void PhiInstr::InferRange() {
Assert that Phi is not a Mint?

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:3242:
range_->Clamp(RangeBoundary::kRangeBoundaryInt64);
Any reason for not computing overflow state to match the code in the BinarySmiOp
almost exactly.

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:3396: RangeBoundary* min,
s/min/result_min/ and the same for max.

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2582: // Calculate the minimum of a and b
within smi range.
... within the given range.

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2596: // -/+ infinity are clamped to
MinConstant/MaxConstant.
the the minimum/maximum of the given type.

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2612: return *this;
Note: Symbolic ranges loose track of the fact that they are clamped. This might
lead to imprecision at later stages of the analysis.

[Cutch, 2014-06-19 18:39:36]

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:5106: // Initialize bitvector for quick
filtering of smi values.
On 2014/06/19 17:27:29, Vyacheslav Egorov (Google) wrote:
> s/smi/int/

Done.

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:2874: if (unboxed == NULL) {
On 2014/06/19 17:27:29, Vyacheslav Egorov (Google) wrote:
> When is this true? How can we be unboxing "nothing"?

Switched to an ASSERT.

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:2899: RangeBoundary::kRangeBoundarySmi);
On 2014/06/19 17:27:29, Vyacheslav Egorov (Google) wrote:
> Add an assertion that value()->definition() is a Smi?

This doesn't work because some Constraints are inserted based on a CheckSmi
instr. Specifically,

v11 <- Parameter:1758(9) T{null, dynamic, Type: class 'dynamic'}
CheckSmi:1514(v11)
v341 <- Constraint:2042(v11 ^ [-1073741824, 1073741823])

I could assert that it isn't a mint operation.

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:2986: range_ = new
Range(RangeBoundary::FromConstant(kMinInt32),
On 2014/06/19 17:27:29, Vyacheslav Egorov (Google) wrote:
> CanDeoptimize() versions of Int32/Uint32 loads should be return values within
a
> Smi range.

Done.

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:3107: void PhiInstr::InferRange() {
On 2014/06/19 17:27:30, Vyacheslav Egorov (Google) wrote:
> Assert that Phi is not a Mint?

I've asserted that Phi is a smi.

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:3242:
range_->Clamp(RangeBoundary::kRangeBoundaryInt64);
On 2014/06/19 17:27:29, Vyacheslav Egorov (Google) wrote:
> Any reason for not computing overflow state to match the code in the
BinarySmiOp
> almost exactly.

We currently don't track overflow / truncating on BinaryMintOpInstr. I'll add
that in a follow up CL.

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:3396: RangeBoundary* min,
On 2014/06/19 17:27:29, Vyacheslav Egorov (Google) wrote:
> s/min/result_min/ and the same for max.

Done here and elsewhere.

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2582: // Calculate the minimum of a and b
within smi range.
On 2014/06/19 17:27:30, Vyacheslav Egorov (Google) wrote:
> ... within the given range.

Done.

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2596: // -/+ infinity are clamped to
MinConstant/MaxConstant.
On 2014/06/19 17:27:30, Vyacheslav Egorov (Google) wrote:
> the the minimum/maximum of the given type.

Done.

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2612: return *this;
On 2014/06/19 17:27:30, Vyacheslav Egorov (Google) wrote:
> Note: Symbolic ranges loose track of the fact that they are clamped. This
might
> lead to imprecision at later stages of the analysis.

I've added a comment.

[Vyacheslav Egorov (Google), 2014-06-19 18:42:29]

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:2899: RangeBoundary::kRangeBoundarySmi);
On 2014/06/19 18:39:35, Cutch wrote:
> On 2014/06/19 17:27:29, Vyacheslav Egorov (Google) wrote:
> > Add an assertion that value()->definition() is a Smi?
> 
> This doesn't work because some Constraints are inserted based on a CheckSmi
> instr. Specifically,
> 
> v11 <- Parameter:1758(9) T{null, dynamic, Type: class 'dynamic'}
> CheckSmi:1514(v11)
> v341 <- Constraint:2042(v11 ^ [-1073741824, 1073741823])
> 
> I could assert that it isn't a mint operation.

In this case you can see reaching type for value() of the constraint to Smi when
you insert and check that.

[Vyacheslav Egorov (Google), 2014-06-19 18:42:53]

s/see reaching type/set reaching type/

[Cutch, 2014-06-19 20:21:50]

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/328503003/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:2899: RangeBoundary::kRangeBoundarySmi);
On 2014/06/19 18:42:29, Vyacheslav Egorov (Google) wrote:
> On 2014/06/19 18:39:35, Cutch wrote:
> > On 2014/06/19 17:27:29, Vyacheslav Egorov (Google) wrote:
> > > Add an assertion that value()->definition() is a Smi?
> > 
> > This doesn't work because some Constraints are inserted based on a CheckSmi
> > instr. Specifically,
> > 
> > v11 <- Parameter:1758(9) T{null, dynamic, Type: class 'dynamic'}
> > CheckSmi:1514(v11)
> > v341 <- Constraint:2042(v11 ^ [-1073741824, 1073741823])
> > 
> > I could assert that it isn't a mint operation.
> 
> In this case you can see reaching type for value() of the constraint to Smi
when
> you insert and check that.

Got it. Thanks!

[Cutch, 2014-06-19 21:13:34]

Committed patchset #16 manually as r37521 (presubmit successful).