IonMonkey: Improve type information at branches

RESOLVED FIXED in mozilla30

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

unspecified
mozilla30
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8351490 [details] [diff] [review]
Rough WIP patch
(Assignee)

Comment 2

3 years ago
Created attachment 8372227 [details] [diff] [review]
bug953164-remove-undefined-at-if

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+
(Assignee)

Comment 4

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/58a6e689c472

Comment 5

3 years ago
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
(Assignee)

Comment 6

3 years ago
Backed out due to Talos red. I assume that also caused the above mentioned crashes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a13fa6115e9
(Assignee)

Comment 7

3 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=58a6e689c472
(Assignee)

Comment 8

3 years ago
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.
(Assignee)

Comment 9

3 years ago
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
(Assignee)

Comment 11

3 years ago
Created attachment 8389933 [details] [diff] [review]
Additional fixes

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+
(Assignee)

Comment 13

3 years ago
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
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 984830
You need to log in before you can comment on or make changes to this bug.