847045 - IonMonkey: Fix ExcludeType bailouts

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

When inlinining a call with type barriers, ExcludeType instructions are added for any argument type present in the caller's TypeSet but not in the callee's TypeSet.

With the baseline compiler this is causing tons of bailouts on v8-deltablue though. The problem is that the TypeSets look like this:

Caller: AnyObject
Callee: TypeObject 1, TypeObject 2

So we will add an ExcludeType(AnyObject) instruction, but this bailout is overzealous if we see an object with TypeObject 1 or 2.

The attached patch removes ExcludeType and just adds a TypeBarrier with the callee's observed types.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Comment 2

[Nicolas B. Pierron [:nbp]]

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

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

The excluded type is a filter which bails when if the type matches.
A type barrier is a filter which bails if the type *does not* match.

You cannot substitute one by the other.
The problem is likely to be a TI issue which is overzealous in its excluded type.

::: js/src/ion/MIR.h
@@ +452,5 @@
>          return virtualRegister_;
>      }
>  
> +#ifdef JS_NUNBOX32
> +    virtual bool isBoxing() const {

I removed that from the original patch.

Comment 3

[Jan de Mooij [:jandem]]

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

(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> The excluded type is a filter which bails when if the type matches.
> A type barrier is a filter which bails if the type *does not* match.
> 
> You cannot substitute one by the other.

The only job of IonBuilder::addTypeBarrier is to make sure no type flows through that's not in calleeObs. My patch does exactly that: if a type is not in calleeObs, the type barrier bails out.

Internally TI uses a linked list of types that it didn't observe, but we don't use that anywhere else and I don't see why we have to use it here.

> The problem is likely to be a TI issue which is overzealous in its excluded
> type.

See the example in comment 1: type information is fine, but Ion is not handling it correctly.

> I removed that from the original patch.

Yeah, we need these for MTypeBarrier when the input is an MBox. I don't think there's a good way to avoid it..

Comment 4

[Nicolas B. Pierron [:nbp]]

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

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

Ok, I feel dumb because I saw the excluded->type list and felt like we should use it, but it appears that we could just do the checks on our own and don't rely on these excluded types.

Still, this patch should at least promote double to int when the callee request it, and prevent adding multiple times the same type barrier.
I think the redefine & multiple type barriers are related to your need of the isBoxing trick.

::: js/src/ion/IonBuilder.cpp
@@ +3032,5 @@
>  
>      while (excluded) {
>          if (excluded->target == calleeObs && callerObs->hasType(excluded->type)) {
> +            MTypeBarrier *barrier = MTypeBarrier::New(ins, cloneTypeSet(calleeObs),
> +                                                      Bailout_Normal);

This code might add multiple time the same type barriers per argument.  You should set a flag and if the flag is set at the end of the loop you can add this type barrier.

@@ -3032,5 @@
>  
>      while (excluded) {
>          if (excluded->target == calleeObs && callerObs->hasType(excluded->type)) {
> -            if (excluded->type == types::Type::DoubleType() &&
> -                calleeObs->hasType(types::Type::Int32Type())) {

You should at least keep the double to int32 convertion added here, otherwise you might have a performance issue where operations involved are making double computations, but we are unable to prove that the input is an Int.

  for (var i = 0; i < 10000; i += 0.5) {
    f = ((i | 0) == i) ? intCallee : dblCallee;
    f(i);
  }

::: js/src/ion/Lowering.cpp
@@ +1636,5 @@
>      if (!useBox(barrier, LTypeBarrier::Input, ins->input()))
>          return false;
>      if (!assignSnapshot(barrier, ins->bailoutKind()))
>          return false;
> +    return redefine(ins, ins->input()) && add(barrier, ins);

remove redefine from here, this intruction is now a guard.

::: js/src/ion/MIR.h
@@ +452,5 @@
>          return virtualRegister_;
>      }
>  
> +#ifdef JS_NUNBOX32
> +    virtual bool isBoxing() const {

As you are moving the MTypeBarrier to a guard this should be unnecessary.  Dvander insisted on the fact that if we need this, this means there is a latent bug elsewhere.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Yeah, thanks Nicolas, agree that it's best to keep boxing-format-specific stuff out of MIR.h.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Patch v2

Addressed review comments. We can't get rid of the redefine though, MTypeBarrier has type MIRType_Value and we use that in other places.

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 721401 [details] [diff] [review]
Patch v2

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

::: js/src/ion/IonBuilder.cpp
@@ +3032,4 @@
>      while (excluded) {
>          if (excluded->target == calleeObs && callerObs->hasType(excluded->type)) {
>              if (excluded->type == types::Type::DoubleType() &&
>                  calleeObs->hasType(types::Type::Int32Type())) {

nit (sorry, my fault): put the '{' on a new line.

@@ +3061,2 @@
>              } else {
> +                needsBarrier = true;

nit: You can remove this else, as you have a break above it.

Comment 8

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1250c1464755

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1250c1464755