1028910 - IonMonkey: Improve type information at branches with MIsObject

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

During IonBuilder we can improve type information at branches (very limited). Now we want to do this for "MIsObject". This is often/only used in selfhosting code, but quite important. Since in selfhosting the following code is used a lot:

if (IsObject(foo)) {
   // do something with foo
}

Currently TI will see all types of "foo" in the branch. We want to adjust it, so IonMonkey knows it will only see "objects" in the branch.

In bug 953164 there was some small framework landed to allow this for:
if (foo) {

}
Which filters undefined/null.

The idea is to use the same techniques to do this for MIsObject.

Comment 1

[Hannes Verschore [:h4writer]]

Till, what would be the best way to add tests for this? We want to be able to test JS code like:

if (IsObject(foo)) {
   // assertSomething
}

But the intrinsic IsObject is only defined in selfhosting code.

Comment 2

[Till Schneidereit [:till]]

You can get at functions defined in the self-hosting compartment by using `getSelfHostedValue` in the shell. I don't see a reason for not using that in tests.

Comment 3

[Sushant Dinesh]

Attachment: Improve type information for branching at MIsObject

Comment 4

[Hannes Verschore [:h4writer]]

Comment on attachment 8450012 [details] [diff] [review]
Improve type information for branching at MIsObject

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

Looks good. Most changes are only cosmetic.

::: js/src/jit/IonBuilder.cpp
@@ +3078,5 @@
> +    // then setTypeToObject is true. Similarly, if the instruction is !IsObject and
> +    // we're not in true branch.
> +    MDefinition *ins = test->getOperand(0);
> +    if (ins->isIsObject()) {
> +        if (trueBranch) {

You can move the "trueBranch" test also in the ins->isIsObject() test.

@@ +3084,5 @@
> +            subject = ins->getOperand(0);
> +        }
> +    }
> +    else if (!trueBranch && ins->isNot()) {
> +        if (ins->toNot()->getOperand(0)->isIsObject()) {

filtersUndefinedOrNull also has a ins->isNot() check. This will now not get executed.

So move the ...->isIsObject() into the "else if".

@@ +3103,5 @@
>  
>      // 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()))) &&
> +            (!setTypeToObject))

Remove the () around the setTypeToObject and remove some spaces to let it start at the same place as "if ((". So with the second "("  )

::: js/src/jsinfer.cpp
@@ +675,5 @@
>      return res;
>  }
>  
> +TemporaryTypeSet *
> +TypeSet::setToObject(LifoAlloc *alloc)

s/setToObject/cloneObjectsOnly/

Comment 5

[Sushant Dinesh]

Attachment: Improve type information for branching at MIsObject

Comment 6

[Hannes Verschore [:h4writer]]

Comment on attachment 8450032 [details] [diff] [review]
Improve type information for branching at MIsObject

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

::: js/src/jit/IonBuilder.cpp
@@ +3096,5 @@
>      // There is no TypeSet that can get filtered.
>      if (!subject->resultTypeSet() || subject->resultTypeSet()->unknown())
>          return true;
>  
>      // Only do this optimization if the typeset does contains null or undefined.

Oh I forgot this. Please update comment.

Comment 7

[Sushant Dinesh]

Attachment: Improve type information for branching at MIsObject

Comment 8

[Hannes Verschore [:h4writer]]

Comment on attachment 8450043 [details] [diff] [review]
Improve type information for branching at MIsObject

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

::: js/src/jit/IonBuilder.cpp
@@ +3081,5 @@
> +    if (ins->isIsObject() && trueBranch) {
> +        setTypeToObject = true;
> +        subject = ins->getOperand(0);
> +    }
> +    else if (!trueBranch && ins->isNot() && ins->toNot()->getOperand(0)->isIsObject()) {

} else if () {
(so on the same line)

@@ +3086,5 @@
> +        setTypeToObject = true;
> +        subject = ins->getOperand(0)->getOperand(0);
> +    }
> +    else
> +        test->filtersUndefinedOrNull(trueBranch, &subject, &removeUndefined, &removeNull);

Or there are no {} or we add it to all cases. So this else case should have {}.

@@ +3097,5 @@
>      if (!subject->resultTypeSet() || subject->resultTypeSet()->unknown())
>          return true;
>  
> +    // Only do this optimization if the typeset does contains null or undefined
> +    // or if we're looking at an object(setTypeToObject is true).

Updated comment a bit.

@@ +3102,3 @@
>      if ((!(removeUndefined && subject->resultTypeSet()->hasType(types::Type::UndefinedType())) &&
> +         !(removeNull && subject->resultTypeSet()->hasType(types::Type::NullType()))) &&
> +        !setTypeToObject)

Only do this optimization when subject type isn't already MIRType_Object.

Comment 9

[Hannes Verschore [:h4writer]]

Addressed comments myself, so this could already land:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a3f72171ac4

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/3a3f72171ac4