Closed Bug 953164 Opened 10 years ago Closed 10 years ago

IonMonkey: Improve type information at branches

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(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.

i.e.
function test(a) {
   if (a) {
     return a+1;
   }
   return 0;
}
test();
test(1);

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]
bug953164-remove-undefined-at-if

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 http://www.msn.com/.  The regression is:


Good:  https://hg.mozilla.org/integration/mozilla-inbound/rev/d6473daed318

Bad:   https://hg.mozilla.org/integration/mozilla-inbound/rev/58a6e689c472


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:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a13fa6115e9
Comment on attachment 8372227 [details] [diff] [review]
bug953164-remove-undefined-at-if

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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a23bcbc4e60
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f7f5787e46e
sorry had to backout this changes for errors like https://tbpl.mozilla.org/php/getParsedLog.php?id=35273678&tree=Mozilla-Inbound 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.

s/has/have
Attachment #8389933 - Flags: review?(jdemooij) → review+
Try build looks good (though there was orange due to the dis() in testcase.).

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c18da81043e
https://hg.mozilla.org/mozilla-central/rev/9c18da81043e
Status: NEW → RESOLVED
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.