953164 - IonMonkey: Improve type information at branches

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

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.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Rough WIP patch

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: 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

Comment 3

[Jan de Mooij [:jandem]]

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.

Comment 4

[Hannes Verschore [:h4writer]]

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

Comment 5

[Gary [:streetwolf52]]

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

Comment 6

[Hannes Verschore [:h4writer]]

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 7

[Hannes Verschore [:h4writer]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=58a6e689c472

Comment 8

[Hannes Verschore [:h4writer]]

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.

Comment 9

[Hannes Verschore [:h4writer]]

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

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Hannes Verschore [:h4writer]]

Attachment: Additional fixes

Should fix the talos reds. Added the testcase I created based on the issue. Also fixes stupid typo!

Comment 12

[Jan de Mooij [:jandem]]

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

Comment 13

[Hannes Verschore [:h4writer]]

Try build looks good (though there was orange due to the dis() in testcase.).

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c18da81043e

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c18da81043e