Don't increase the return typeset during inlining

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

(Depends on 1 bug)

unspecified
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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.
Assignee: nobody → hv1989
Posted patch example.jsSplinter Review
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
Blocks: 1103441
Posted patch specialize-inlined-call (obsolete) — Splinter Review
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.
Attachment #8540853 - Flags: review?(jdemooij)
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?
(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?
Posted patch Patch (obsolete) — Splinter Review
Good find and we are indeed fortunate returnBlock is actually current. Nice :D.
Attachment #8540853 - Attachment is obsolete: true
Attachment #8540853 - Flags: review?(jdemooij)
Attachment #8540886 - Flags: review?(jdemooij)
Comment on attachment 8540886 [details] [diff] [review]
Patch

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

Great :)
Attachment #8540886 - Flags: review?(jdemooij) → review+
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?
Posted patch Patch (obsolete) — Splinter Review
Oh stupid. I remember reading about the "Setter" issue, when doing the change. I just forgot to act on it. (I blame jetlag)
Attachment #8540886 - Attachment is obsolete: true
Attachment #8541186 - Flags: review?(jdemooij)
Attachment #8541186 - Flags: review?(jdemooij) → review+
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.
Depends on: 1115740
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.
Attachment #8541186 - Attachment is obsolete: true
Attachment #8542105 - Flags: review?(jdemooij)
(In reply to Hannes Verschore [:h4writer] from comment #11)
> This reduces the testcase to 1554ms.

Correction: 2036ms
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 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.
Attachment #8542105 - Flags: review?(jdemooij) → review+
(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.
(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.
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
https://hg.mozilla.org/mozilla-central/rev/e2d5ec4286aa
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.