Closed Bug 910807 Opened 11 years ago Closed 11 years ago

Strengthen beta node analysis

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: sunfish, Assigned: sunfish)

Details

Attachments

(5 files, 1 obsolete file)

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.
Attachment #797370 - Flags: review?(nicolas.b.pierron)
Attached patch range-nobe.patchSplinter Review
Attachment #797371 - Flags: review?(nicolas.b.pierron)
Attachment #797372 - Flags: review?(nicolas.b.pierron)
Attached patch range-doubles.patch (obsolete) — Splinter Review
Attachment #797373 - Flags: review?(nicolas.b.pierron)
Attachment #797375 - Flags: review?(nicolas.b.pierron)
Attachment #797370 - Flags: review?(nicolas.b.pierron) → review+
Attachment #797372 - Flags: review?(nicolas.b.pierron) → review+
Attachment #797371 - Flags: feedback?(rpearl)
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.
Attachment #797373 - Flags: review?(nicolas.b.pierron)
Attachment #797375 - Flags: review?(nicolas.b.pierron) → review+
(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.
Attachment #797373 - Attachment is obsolete: true
Attachment #798173 - Flags: review?(nicolas.b.pierron)
Summary: Strength beta node analysis → Strengthen beta node analysis
Whiteboard: [leave open]
Attachment #798173 - Flags: review?(nicolas.b.pierron) → review+
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
Attachment #797371 - Flags: feedback?(rpearl)
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.
Attachment #797371 - Flags: review?(nicolas.b.pierron) → review+
The two patches remaining here are now being tracked in bug 915846, where they've been updated for some API changes.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
Given that this has landed patches, let's make it FIXED, instead.
Resolution: DUPLICATE → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: