994016 - IonMonkey: Improve type information at branches with TypeOf

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

Currently in selfhosted code we have a function "IsObject", which uses:

> function IsObject(v) {
>     return (typeof v === "object" && v !== null) ||
>            typeof v === "function" ||
>            (typeof v === "undefined" && v !== undefined);
> }

Now we have two issues here. We should optimize MTypeOf to MTypeOfIs.
(bug 725966)

Secondly just like (bug 953164) we should optimize and improve our types at the branch. So we can optimize:

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

Because atm it will just have all types that v had before the branch.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Add TypeOf folding to MCompare

Just to get things started.

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: WIP: filter typeof

Was quite easy. Supports the true branch of "typeof x == 'foo'".

For the false branch I might want to add a jsinfer function to ease the TI interaction. Could/Should have done that earlier. Other paths can use that too to simplify code.

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Part 1: Remove filter and add removeset function

The filter function is too specific. Lets remove it and add a "removeSet" function, which returns the relative complement, or just remove the types in the second set from the first set.

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: Part 2: remove "types::"

IonBuilder uses a lot of functionality of TypeInference. As a result it has a lot of functionality with the "types::" prefix. This is very verbose and also increases the length of the lines a lot. I think we should remove the "types::" to make reading IonBuilder easier.

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: Part 3: Use more api of TI

The improve types at tests use a lot of low level functionality of TI. (Mimicking the flags etc). Let's remove that and use the higher level API with ResultTypeSet and addType.

Comment 6

[Hannes Verschore [:h4writer]]

Attachment: Do TypeOf testing in the improve types at test infrastructure

The final piece. Filtering types at MTypeOf now ;)

Comment 7

[Hannes Verschore [:h4writer]]

Comment on attachment 8563483 [details] [diff] [review]
Part 2: remove "types::"

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

So this seems not needed anymore on a more recent revision ;)

Comment 8

[Brian Hackett [Laid off!]]

Comment on attachment 8563482 [details] [diff] [review]
Part 1: Remove filter and add removeset function

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

::: js/src/jit/IonBuilder.cpp
@@ +3456,5 @@
> +            flags |= types::TYPE_FLAG_UNDEFINED;
> +        if (altersNull)
> +            flags |= types::TYPE_FLAG_NULL;
> +
> +        types::TemporaryTypeSet remove(flags, static_cast<types::TypeSetObjectKey**>(nullptr));

Hmm, can you just allocate this with the LifoAlloc and then add types to it via the normal type set API?  The TYPE_FLAG flags really ought to be private to TypeSet.

@@ +3581,5 @@
>          {
>              return true;
>          }
> +        uint32_t flags = types::TYPE_FLAG_UNDEFINED | types::TYPE_FLAG_NULL;
> +        types::TemporaryTypeSet remove(flags, static_cast<types::TypeSetObjectKey**>(nullptr));

Ditto.

::: js/src/jsinfer.cpp
@@ +787,5 @@
> +        return res;
> +
> +    // A TypeSet cannot capture 'any object except for X, Y, Z'. One need to
> +    // specify specific objects or any object. So suboptimally the exclusion
> +    // of these specific objects is lost.

The logic below is also not quite right for objects with unknown properties, which implicitly render the type set as if it was unknownObject().  Given this and the above gotcha, can we just restrict this function via assertions so that it can only filter out type sets containing some set of primitive types?  That will satisfy the current (and likely future) users of this method, avoid these thorny semantic issues, and simplify the code.

Comment 9

[Hannes Verschore [:h4writer]]

Attachment: Part 1: Remove filter and add removeset function

So now only primitives and anyobject are supported. (I need AnyObject for TypeOf).
The other comments are addressed in part3 ;)

Comment 10

[Brian Hackett [Laid off!]]

Comment on attachment 8564107 [details] [diff] [review]
Part 1: Remove filter and add removeset function

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

::: js/src/jsinfer.cpp
@@ +773,5 @@
>  /* static */ TemporaryTypeSet *
> +TypeSet::removeSet(TemporaryTypeSet *input, TemporaryTypeSet *removal, LifoAlloc *alloc)
> +{
> +    // Only allow removal of primitives and the "AnyObject" flag.
> +    MOZ_ASSERT(!!(~(TYPE_FLAG_PRIMITIVE | TYPE_FLAG_ANYOBJECT) & removal->baseFlags()));

Maybe MOZ_ASSERT(removal->getObjectCount() == 0 && !removal->unknown()), for clarity?

Comment 11

[Brian Hackett [Laid off!]]

(In reply to Brian Hackett (:bhackett) from comment #10)
> Maybe MOZ_ASSERT(removal->getObjectCount() == 0 && !removal->unknown()), for
> clarity?

Actually, to avoid other API assertions I guess this would need to be:

MOZ_ASSERT(!removal->unknown());
MOZ_ASSERT_IF(!removal->unknownObject(), removal->getObjectCount() == 0);

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8562906 [details] [diff] [review]
Add TypeOf folding to MCompare

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

Nice! r=me with comments below addressed.

::: js/src/jit/MIR.cpp
@@ +3272,5 @@
> +    {
> +        return false;
> +    }
> +
> +    JSAtomState names = GetJitContext()->runtime->names();

const JSAtomState &names

JSAtomState is pretty big so we don't want to copy it to the stack.

@@ +3277,5 @@
> +    if (constant->toString() == TypeName(JSTYPE_VOID, names)) {
> +        if (!typeOf->input()->mightBeType(MIRType_Undefined) &&
> +            !typeOf->inputMaybeCallableOrEmulatesUndefined())
> +        {
> +            *result = false;

I think this (and below) should be:

*result = (jsop() == JSOP_NE || jsop() == JSOP_STRICTNE);

If jit-tests didn't catch this, can you add a test that fails without that change?

@@ +3305,5 @@
> +            return true;
> +        }
> +    } else if (constant->toString() == TypeName(JSTYPE_OBJECT, names)) {
> +        if (!typeOf->input()->mightBeType(MIRType_Object) &&
> +            !typeOf->input()->mightBeType(MIRType_Null)) {

Nit: { on next line.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8563488 [details] [diff] [review]
Do TypeOf testing in the improve types at test infrastructure

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

Looks good.

::: js/src/jit/IonBuilder.cpp
@@ +3436,5 @@
> +
> +    bool equal = ins->jsop() == JSOP_EQ || ins->jsop() == JSOP_STRICTEQ;
> +    bool notEqual = ins->jsop() == JSOP_NE || ins->jsop() == JSOP_STRICTNE;
> +
> +    trueBranch = trueBranch ^ notEqual;

I think this would be clearer as:

if (notEqual)
    trueBranch = !trueBranch;

@@ +3450,5 @@
> +    // Note: we cannot remove the AnyObject type in the false branch,
> +    // since there are multiple ways to get an object. That is the reason
> +    // for the 'trueBranch' test.
> +    TemporaryTypeSet filter;
> +    JSAtomState names = GetJitContext()->runtime->names();

const JSAtomState &names. Please add this to the end of JSAtomState:

private:
  JSAtomState(const JSAtomState &) = delete;
  operator=(const JSAtomState &) = delete;

To make copying JSAtomState a compile error.

::: js/src/jit/IonBuilder.h
@@ +361,1 @@
>      // Used to detect triangular structure at test.

Nit: add a newline before this comment

Comment 14

[Hannes Verschore [:h4writer]]

Attachment: Create typeset when there is no typeset.

This is the last stone and improves the undercore.js speed in:
bug 1131099 comment 2

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8567906 [details] [diff] [review]
Create typeset when there is no typeset.

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

Cool!

::: js/src/jit/IonBuilder.cpp
@@ +3440,5 @@
>      if (!equal && !notEqual)
>          return true;
>  
>      MDefinition *subject = typeOf->input();
> +    TemporaryTypeSet *input = subject->resultTypeSet();

Nit: maybe inputTypes or subjectTypes to make it clear when reading the code below that input is a TypeSet and not a MDefinition?

@@ +3516,5 @@
>          MOZ_CRASH("Relational compares not supported");
>      }
>  
>      MDefinition *subject = ins->lhs();
> +    TemporaryTypeSet *input = subject->resultTypeSet();

And here.

Comment 16

[Hannes Verschore [:h4writer]]

(In reply to Jan de Mooij [:jandem] from comment #13)
> @@ +3450,5 @@
> > +    // Note: we cannot remove the AnyObject type in the false branch,
> > +    // since there are multiple ways to get an object. That is the reason
> > +    // for the 'trueBranch' test.
> > +    TemporaryTypeSet filter;
> > +    JSAtomState names = GetJitContext()->runtime->names();
> 
> const JSAtomState &names. Please add this to the end of JSAtomState:
> 
> private:
>   JSAtomState(const JSAtomState &) = delete;
>   operator=(const JSAtomState &) = delete;
> 
> To make copying JSAtomState a compile error.

Sadly not quite possible since JSAtomState is a struct.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Why is that a problem?  Structs can have private stuff and constructors and whatnot in C++.  They just become non-POD.

Comment 18

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14df9078d7a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/68c6f7e33f1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/f21653ee56e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/40403a726ac2
https://hg.mozilla.org/integration/mozilla-inbound/rev/490afdad9ba1

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/14df9078d7a7
https://hg.mozilla.org/mozilla-central/rev/68c6f7e33f1e
https://hg.mozilla.org/mozilla-central/rev/f21653ee56e5
https://hg.mozilla.org/mozilla-central/rev/40403a726ac2
https://hg.mozilla.org/mozilla-central/rev/490afdad9ba1

Comment 20

[Hannes Verschore [:h4writer]]

Note to myself: you forgot "typeof v == boolean"

Comment 21

[Hannes Verschore [:h4writer]]

Attachment: Boolean, you are worthy too

Comment 22

[Hannes Verschore [:h4writer]]

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

Comment 23

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

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