Closed Bug 953164 Opened 10 years ago Closed 10 years ago

IonMonkey: Improve type information at branches


(Core :: JavaScript Engine: JIT, defect)

Not set





(Reporter: h4writer, Assigned: h4writer)




(2 files, 1 obsolete file)

We have a pass, quite late that looks if a typebarrier tests for a undefined/null value. We can improve this, by doing it in IonBuilder. That way we can better optimize the construct.

function test(a) {
   if (a) {
     return a+1;
   return 0;

In the above case we won't be able to specialize the type of a+1 to integer. It will be MIRType_Value, since it saw Undefined/Integer. With the test we can specialize it to MIRType_Integer.
Attached patch Rough WIP patch (obsolete) — Splinter Review
Same as rough WIP patch, but now done properly.

Richards: 24 hits
Crypto: 72 hits
Raytrace: 102 hits
EarleyBoyer: 65 hits
RegExp: 4 hits
PdfJS: 108 hits
CodeLoad: 318 hits
Typescript: 676 hits
Assignee: nobody → hv1989
Attachment #8351490 - Attachment is obsolete: true
Attachment #8372227 - Flags: review?(jdemooij)
Comment on attachment 8372227 [details] [diff] [review]

Review of attachment 8372227 [details] [diff] [review]:

Looks great, patch was pretty easy to understand. r=me with nits below addressed.

::: js/src/jit/IonAnalysis.cpp
@@ +1657,3 @@
>          return;
> +    // Make sure the subject the test test equals the input to the TypeBarrier

Nit: fix comment.

::: js/src/jit/IonBuilder.cpp
@@ +3057,5 @@
>      return ControlStatus_Jumped;
>  }
>  bool
> +IonBuilder::improveTypesAtTest(MTest *test)

Nit: I think filterTypesAtTest and s/MImproveTypeSet/MFilterTypeSet is slightly clearer and matches MTest::filtersUndefinedOrNull. What do you think?

@@ +6370,5 @@
>      return replace;
>  }
> +MDefinition *
> +IonBuilder::ensureDefiniteTypeSet(MDefinition *def, types::TemporaryTypeSet *type)

Nit: s/type/types for consistency with most other functions.
Attachment #8372227 - Flags: review?(jdemooij) → review+
I'm running this patch from inbound and I crash Fx going to  The regression is:



Mozilla/5.0 (Windows NT 6.3; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
Backed out due to Talos red. I assume that also caused the above mentioned crashes:
Comment on attachment 8372227 [details] [diff] [review]

Review of attachment 8372227 [details] [diff] [review]:

::: js/src/jit/IonBuilder.cpp
@@ +3079,5 @@
> +        return true;
> +
> +    // Only do this optimization if the typeset does contains null or undefined.
> +    if ((removeUndefined && subject->resultTypeSet()->hasType(types::Type::UndefinedType())) ||
> +        (removeNull && subject->resultTypeSet()->hasType(types::Type::NullType())))

Off course this optimization shouldn't be done, if the above statement is false, instead of true. I'll try to create a test for the wrong behaviour.
I wasn't able to create a test for this wrong behaviour. It was green on try and I ran a debug browser build for a day to be sure.
sorry had to backout this changes for errors like on OS X and Windows at least
Attached patch Additional fixesSplinter Review
Should fix the talos reds. Added the testcase I created based on the issue. Also fixes stupid typo!
Attachment #8389933 - Flags: review?(jdemooij)
Comment on attachment 8389933 [details] [diff] [review]
Additional fixes

Review of attachment 8389933 [details] [diff] [review]:

::: js/src/jit/IonBuilder.cpp
@@ +3057,5 @@
>              replace = ensureDefiniteTypeSet(subject, type);
> +            // Make sure we don't hoist it above the MTest, we can use the
> +            // 'dependency' of an MInstruction. This is normally used by
> +            // Alias Analysis, but won't get overwritten, since this
> +            // instruction doesn't has an AliasSet.

Attachment #8389933 - Flags: review?(jdemooij) → review+
Try build looks good (though there was orange due to the dis() in testcase.).
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.