1114981 - Don't increase the return typeset during inlining

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

We capture the result types of a MCall, so have precise information on this. Now during inlining the result type can get clobbered with extra types that we actually will never encounter:

> function asCoerceString(x) {
>     if (typeof x === "string") {                    
>         return x; 
>     }
>     else if (x == undefined) {
>         return null;                
>     }                
>     return x + ''; 
> }

if we never run x with null/undefined, the result typeset will be String. While if we inline this snippit we will increase the typeset to contain "null", as a result have worse type information when inlining.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: example.js

h4writer@h4writer-ThinkPad-W530:~/Build/mozilla-inbound/js/src/build-32-parallel$ IONFLAGS=logs,scripts js --ion-inlining=on --no-threads /tmp/example.js
3600
h4writer@h4writer-ThinkPad-W530:~/Build/mozilla-inbound/js/src/build-32-parallel$ IONFLAGS=logs,scripts js --ion-inlining=off --no-threads /tmp/example.js
2332

so inlining decreases the running time from 2332ms to 3600ms

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: specialize-inlined-call

Improves given testcase to 2040ms.

I actually wanted to do the TypeBarrier in each inlined script, since TypeBarrier has more chance to be removed/noped or transformed into a MBail. Now this allowed it to get hoisted and as a result bail too fast. We don't want that, so I moved the typebarrier to after the MPhi.

I think it shouldn't give a performance penalty, but if it does, I will adjust it to use MBail in the inlined cases where it a type is used that we didn't record during TI yet.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8540853 [details] [diff] [review]
specialize-inlined-call

Review of attachment 8540853 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thanks. I verified this fixes the splay.swf issue :) Just one issue below.

::: js/src/jit/IonBuilder.cpp
@@ +6855,5 @@
>      // must be a resume point capturing the original def, and resuming
>      // to that point will explicitly monitor the new type.
>  
>      if (kind == BarrierKind::NoBarrier) {
>          MDefinition *replace = ensureDefiniteType(def, observed->getKnownMIRType());

ensureDefiniteType and pushConstant below will use current, shouldn't we use block?

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> ensureDefiniteType and pushConstant below will use current, shouldn't we use
> block?

Hm actually, we can return the block argument from pushTypeBarrier I think, since returnBlock == current when we call it?

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: Patch

Good find and we are indeed fortunate returnBlock is actually current. Nice :D.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8540886 [details] [diff] [review]
Patch

Review of attachment 8540886 [details] [diff] [review]:
-----------------------------------------------------------------

Great :)

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8540886 [details] [diff] [review]
Patch

Review of attachment 8540886 [details] [diff] [review]:
-----------------------------------------------------------------

Great :)

::: js/src/jit/IonBuilder.cpp
@@ +4449,5 @@
> +    if (!setCurrentAndSpecializePhis(returnBlock))
> +        return false;
> +
> +    // Improve return types with observed typeset.
> +    types::TemporaryTypeSet *types = bytecodeTypes(pc);

Sorry submitted the review too soon. For inlined setters this will probably assert, there we push the original RHS and ignore the return value... Maybe we can check if pc has JOF_INVOKE?

Comment 8

[Hannes Verschore [:h4writer]]

Attachment: Patch

Oh stupid. I remember reading about the "Setter" issue, when doing the change. I just forgot to act on it. (I blame jetlag)

Comment 9

[Hannes Verschore [:h4writer]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d56903c1c02

https://hg.mozilla.org/integration/mozilla-inbound/rev/b17f6c6b10a1

Comment 10

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f12de4aa974

I backed it out, since AWFY wasn't happy with this one. So I will probably will need to do the MBail thing I described.

Comment 11

[Hannes Verschore [:h4writer]]

Attachment: Specialize before the accumalate return blocks

This improves type information before going to the big accumulate block. This is better, since it only checks paths which increase typeset beyond what we observed on that inlined path only.

This reduces the testcase to 1554ms.

This also doesn't have the issue of mandreel and even allows us to do "int" arith instead of "double". Which doesn't make a perf difference here.

Comment 12

[Hannes Verschore [:h4writer]]

(In reply to Hannes Verschore [:h4writer] from comment #11)
> This reduces the testcase to 1554ms.

Correction: 2036ms

Comment 13

[Hannes Verschore [:h4writer]]

Comment on attachment 8542105 [details] [diff] [review]
Specialize before the accumalate return blocks

Review of attachment 8542105 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/IonBuilder.cpp
@@ +4503,5 @@
> +            return rdef;
> +    } else {
> +        // Don't specialize if types are inaccordance, except for MIRType_Value
> +        // and MIRType_Object (when not unknown object), since the typeset
> +        // contains more specific information. 

Remove whitespace

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8542105 [details] [diff] [review]
Specialize before the accumalate return blocks

Review of attachment 8542105 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good :)

::: js/src/jit/IonBuilder.cpp
@@ +4493,5 @@
> +    if (types->empty() || types->unknown())
> +        return rdef;
> +
> +    // Decide if spezializing is need testing the result typeset if available,
> +    // else use the result type.

Nit: fix the comment

@@ +4497,5 @@
> +    // else use the result type.
> +
> +    if (rdef->resultTypeSet()) {
> +        // Don't spezialize if return typeset is a subset of the
> +        // observed typeset. The return typeset is already more specific.

Nit: specialize (typo)

@@ +4510,5 @@
> +            observedType != MIRType_Value &&
> +            (observedType != MIRType_Object || types->unknownObject()))
> +        {
> +            return rdef;
> +        }

It's probably common to have a function that returns int32 somewhere and double somewhere else, so maybe also add:

if (observedType == MIRType_Double && IsNumberType(rdef->type())
    return rdef;

Or change observedType == rdef->type() to

  types->mightBeMIRType(rdef->type());

And it should handle this case automatically I think.

Comment 15

[Hannes Verschore [:h4writer]]

(In reply to Jan de Mooij [:jandem] from comment #14)
> @@ +4510,5 @@
> > +            observedType != MIRType_Value &&
> > +            (observedType != MIRType_Object || types->unknownObject()))
> > +        {
> > +            return rdef;
> > +        }
> 
> It's probably common to have a function that returns int32 somewhere and
> double somewhere else, so maybe also add:
> 
> if (observedType == MIRType_Double && IsNumberType(rdef->type())
>     return rdef;
> 
> Or change observedType == rdef->type() to
> 
>   types->mightBeMIRType(rdef->type());
> 
> And it should handle this case automatically I think.

Actually I don't think that matters. We have a similar issue with observed:"object[xxx] null" and return MIR type MIRType_Object. In that case we will add a TypeBarrier with "object[xxx] null" (which is strictly more than MIRType_Object that can only get returned).

Now the reason this isn't an issue is:
"observed" contains the typeset of all observed return paths. So when we disallow anything that is more broad than "observed", the summation (phi) of all returns will be exactly the "observed" path. Which is what we want.

E.g having int32 return in that one path and double as observedType, means one of the return paths will definitely contain double. So the phi will become double. Doing it during phi specialization or during inline return specialization doesn't make a difference.

Comment 16

[Jan de Mooij [:jandem]]

(In reply to Hannes Verschore [:h4writer] from comment #15)
> E.g having int32 return in that one path and double as observedType, means
> one of the return paths will definitely contain double. So the phi will
> become double. Doing it during phi specialization or during inline return
> specialization doesn't make a difference.

True, sorry I wasn't clearer, I just suggested it to avoid adding an unnecessary TypeBarrier that we know will be eliminated (or replaced with an Unbox) later.

Comment 17

[Hannes Verschore [:h4writer]]

Oh ok. Yeah, it is an interesting suggestion, I didn't think about that before you suggested it. I eventually just left it like I posted it (which went already through a try build).

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2d5ec4286aa

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e2d5ec4286aa