910807 - Strengthen beta node analysis

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Dan Gohman [:sunfish]]

In order to make it possible to write simple testcases for bugs like bug 909494, it's necessary to extend beta node analysis to support code like this:

  if (x < 200)
     return;
  if (x > -200)
     return;
  do_something_with_a_restricted_range(x);

This requires two changes. First, beta node analysis needs to support comparisons with double constants, and second, it needs to correctly update beta operands so that beta nodes can be combined.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: range-spew-help.patch

Comment 2

[Dan Gohman [:sunfish]]

Attachment: range-nobe.patch

Comment 3

[Dan Gohman [:sunfish]]

Attachment: range-union-with.patch

Comment 4

[Dan Gohman [:sunfish]]

Attachment: range-doubles.patch

Comment 5

[Dan Gohman [:sunfish]]

Attachment: range-beta-val.patch

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 797373 [details] [diff] [review]
range-doubles.patch

Review of attachment 797373 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/RangeAnalysis.cpp
@@ +159,5 @@
>          if (left->isConstant() && left->toConstant()->value().isInt32()) {
>              bound = left->toConstant()->value().toInt32();
>              val = right;
>              jsop = analyze::ReverseCompareOp(jsop);
> +        } else if (left->isConstant() && left->toConstant()->value().isDouble()) {

Instead, you should replace the int32 specialization by value().isNumber() and value().toNumber(), which are doing both int32 and double.

@@ +167,4 @@
>          } else if (right->isConstant() && right->toConstant()->value().isInt32()) {
>              bound = right->toConstant()->value().toInt32();
>              val = left;
> +        } else if (right->isConstant() && right->toConstant()->value().isDouble()) {

same here.

@@ +207,4 @@
>  
>          Range comp;
>          if (val->type() == MIRType_Int32)
>              comp.setInt32();

Move these 2 lines after the switch, and use setInt32() & rectifyExponent().

@@ +215,5 @@
>            case JSOP_LT:
> +            if (val->type() == MIRType_Int32) {
> +                int32_t intbound;
> +                if (DoubleIsInt32(bound, &intbound) && SafeSub(intbound, 1, &intbound))
> +                    bound = intbound;

The double fit on an int64, then it will set the infinity flags if it overflows, and moving the setInt32 will remove the need of the original SafeAdd and SafeSub. Checking if this is an int32 sounds no longer necessary and we can directly execute the ceil function.

Also, note that with the current change, you might set an infinity flag even if the MIRType is Int32.

Comment 7

[Dan Gohman [:sunfish]]

Attachment: updated per review feedback

(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> Comment on attachment 797373 [details] [diff] [review]
> range-doubles.patch
> 
> Review of attachment 797373 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/RangeAnalysis.cpp
> @@ +159,5 @@
> >          if (left->isConstant() && left->toConstant()->value().isInt32()) {
> >              bound = left->toConstant()->value().toInt32();
> >              val = right;
> >              jsop = analyze::ReverseCompareOp(jsop);
> > +        } else if (left->isConstant() && left->toConstant()->value().isDouble()) {
> 
> Instead, you should replace the int32 specialization by value().isNumber()
> and value().toNumber(), which are doing both int32 and double.

Ok, that makes it much cleaner.

> @@ +207,4 @@
> >  
> >          Range comp;
> >          if (val->type() == MIRType_Int32)
> >              comp.setInt32();
> 
> Move these 2 lines after the switch, and use setInt32() & rectifyExponent().

Ok.

> @@ +215,5 @@
> >            case JSOP_LT:
> > +            if (val->type() == MIRType_Int32) {
> > +                int32_t intbound;
> > +                if (DoubleIsInt32(bound, &intbound) && SafeSub(intbound, 1, &intbound))
> > +                    bound = intbound;
> 
> The double fit on an int64, then it will set the infinity flags if it
> overflows, and moving the setInt32 will remove the need of the original
> SafeAdd and SafeSub. Checking if this is an int32 sounds no longer necessary
> and we can directly execute the ceil function.

The SafeAdd and SafeSub are still useful. This is saying that if x < c, the upper bound on x is c-1. That only holds for integers, and only when c isn't the minimum representable integer. I added comments for this.

> Also, note that with the current change, you might set an infinity flag even
> if the MIRType is Int32.

I've now changed the earlier check to filter for double values that fit in the int32 range instead of int64, since there's no point in creating beta nodes with infinity ranges.

Comment 8

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f9e1bd94899
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f5cc98a1c54
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f5132a52477

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f9e1bd94899
https://hg.mozilla.org/mozilla-central/rev/7f5cc98a1c54
https://hg.mozilla.org/mozilla-central/rev/0f5132a52477

Comment 10

[Nicolas B. Pierron [:nbp]]

Comment on attachment 797371 [details] [diff] [review]
range-nobe.patch

Review of attachment 797371 [details] [diff] [review]:
-----------------------------------------------------------------

rpearl> nbp: it amuses me but if people actually want it gone it doesn't *actually* make a difference

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 797371 [details] [diff] [review]
range-nobe.patch

Review of attachment 797371 [details] [diff] [review]:
-----------------------------------------------------------------

I think being consistent is important, independently of the joke.
Having joke is good for the mood of contributors as long as they are not confusing.

The inconsistencies between nobe and node make this joke confusing.
I would have preferred a patch going the other way around, but this patch fix the inconsistencies and has no reason to be refused knowing the previous comment from rpearl.

Comment 12

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c1931a68abc

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8c1931a68abc

Comment 14

[Dan Gohman [:sunfish]]

The two patches remaining here are now being tracked in bug 915846, where they've been updated for some API changes.

Comment 15

[Till Schneidereit [:till]]

Given that this has landed patches, let's make it FIXED, instead.