Deal with more conditional expressions in the value range analyzer.

Deal with more conditional expressions in the value range analyzer.
Committed: https://code.google.com/p/dart/source/detail?r=13213

[Søren Gjesse, 2012-10-02 08:07:20]

LGTM with a few comments.

http://codereview.chromium.org/10986085/diff/6001/lib/compiler/implementation...
File lib/compiler/implementation/constant_system_dart.dart (right):

http://codereview.chromium.org/10986085/diff/6001/lib/compiler/implementation...
lib/compiler/implementation/constant_system_dart.dart:283: apply(left, right) =>
left >= right;
Oops

http://codereview.chromium.org/10986085/diff/6001/lib/compiler/implementation...
File lib/compiler/implementation/ssa/value_range_analyzer.dart (right):

http://codereview.chromium.org/10986085/diff/6001/lib/compiler/implementation...
lib/compiler/implementation/ssa/value_range_analyzer.dart:512: if (left.lower ==
left.upper && left == right) {
Maybe have an isSingleValue on Range and express this as (left.isSingleValue &&
right.isSingleValue && left == right)

http://codereview.chromium.org/10986085/diff/6001/lib/compiler/implementation...
lib/compiler/implementation/ssa/value_range_analyzer.dart:625: Range range =
computeNewRange(operation, rightRange);
Wouldn't it be better to include the intersection in the computeNewRange and
pass both ranges to it?

http://codereview.chromium.org/10986085/diff/6001/tests/compiler/dart2js/valu...
File tests/compiler/dart2js/value_range_test.dart (right):

http://codereview.chromium.org/10986085/diff/6001/tests/compiler/dart2js/valu...
tests/compiler/dart2js/value_range_test.dart:159: // Force [value] to be an int.
I don't understand how this forces value to be an int.

[ngeoffray, 2012-10-02 16:03:09]

Thanks Soren

http://codereview.chromium.org/10986085/diff/6001/lib/compiler/implementation...
File lib/compiler/implementation/ssa/value_range_analyzer.dart (right):

http://codereview.chromium.org/10986085/diff/6001/lib/compiler/implementation...
lib/compiler/implementation/ssa/value_range_analyzer.dart:625: Range range =
computeNewRange(operation, rightRange);
On 2012/10/02 08:07:20, Søren Gjesse wrote:
> Wouldn't it be better to include the intersection in the computeNewRange and
> pass both ranges to it?

Not sure I understand that comment. Could you expand a bit?

http://codereview.chromium.org/10986085/diff/6001/tests/compiler/dart2js/valu...
File tests/compiler/dart2js/value_range_test.dart (right):

http://codereview.chromium.org/10986085/diff/6001/tests/compiler/dart2js/valu...
tests/compiler/dart2js/value_range_test.dart:159: // Force [value] to be an int.
On 2012/10/02 08:07:20, Søren Gjesse wrote:
> I don't understand how this forces value to be an int

It makes the type guard computation want value to be an int. I will change the
comment.

[Søren Gjesse, 2012-10-02 16:26:56]

http://codereview.chromium.org/10986085/diff/6001/lib/compiler/implementation...
File lib/compiler/implementation/ssa/value_range_analyzer.dart (right):

http://codereview.chromium.org/10986085/diff/6001/lib/compiler/implementation...
lib/compiler/implementation/ssa/value_range_analyzer.dart:625: Range range =
computeNewRange(operation, rightRange);
On 2012/10/02 16:03:09, ngeoffray wrote:
> On 2012/10/02 08:07:20, Søren Gjesse wrote:
> > Wouldn't it be better to include the intersection in the computeNewRange and
> > pass both ranges to it?
> 
> Not sure I understand that comment. Could you expand a bit?

What I ment was to change

  Range range = computeNewRange(operation, rightRange);
  range = range.intersection(leftRange);

to one function like this:

  Range range = computeNewRange(operation, leftRange, rightRange);

and maybe call it something else.

[ngeoffray, 2012-10-04 08:51:07]

Thanks Soren,

I also updated the code that updates trueBranch/falseBranch to update only if
the branch is the single predecessor. If there are more predecessors (eg with
break), we should not update.

http://codereview.chromium.org/10986085/diff/6001/lib/compiler/implementation...
File lib/compiler/implementation/ssa/value_range_analyzer.dart (right):

http://codereview.chromium.org/10986085/diff/6001/lib/compiler/implementation...
lib/compiler/implementation/ssa/value_range_analyzer.dart:512: if (left.lower ==
left.upper && left == right) {
On 2012/10/02 08:07:20, Søren Gjesse wrote:
> Maybe have an isSingleValue on Range and express this as (left.isSingleValue
&&
> right.isSingleValue && left == right)

Done.

http://codereview.chromium.org/10986085/diff/6001/lib/compiler/implementation...
lib/compiler/implementation/ssa/value_range_analyzer.dart:625: Range range =
computeNewRange(operation, rightRange);
On 2012/10/02 16:26:56, Søren Gjesse wrote:
> On 2012/10/02 16:03:09, ngeoffray wrote:
> > On 2012/10/02 08:07:20, Søren Gjesse wrote:
> > > Wouldn't it be better to include the intersection in the computeNewRange
and
> > > pass both ranges to it?
> > 
> > Not sure I understand that comment. Could you expand a bit?
> 
> What I ment was to change
> 
>   Range range = computeNewRange(operation, rightRange);
>   range = range.intersection(leftRange);
> 
> to one function like this:
> 
>   Range range = computeNewRange(operation, leftRange, rightRange);
> 
> and maybe call it something else.

Done.

[Søren Gjesse, 2012-10-04 10:09:38]

lgtm