Closed Bug 994016 Opened 8 years ago Closed 7 years ago

IonMonkey: Improve type information at branches with TypeOf

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(6 files, 3 obsolete files)

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.
Assignee: nobody → hv1989
Blocks: 1131537
Just to get things started.
Attachment #8562906 - Flags: review?(jdemooij)
Attached patch WIP: filter typeof (obsolete) — Splinter Review
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.
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.
Attachment #8563022 - Attachment is obsolete: true
Attachment #8563482 - Flags: review?(bhackett1024)
Attached patch Part 2: remove "types::" (obsolete) — Splinter Review
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.
Attachment #8563483 - Flags: review?(bhackett1024)
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.
Attachment #8563485 - Flags: review?(bhackett1024)
The final piece. Filtering types at MTypeOf now ;)
Attachment #8563488 - Flags: review?(jdemooij)
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 ;)
Attachment #8563483 - Flags: review?(bhackett1024) → review-
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.
Attachment #8563482 - Flags: review?(bhackett1024)
Attachment #8563485 - Flags: review?(bhackett1024) → review+
Attachment #8563483 - Attachment is obsolete: true
So now only primitives and anyobject are supported. (I need AnyObject for TypeOf).
The other comments are addressed in part3 ;)
Attachment #8563482 - Attachment is obsolete: true
Attachment #8564107 - Flags: review?(bhackett1024)
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?
Attachment #8564107 - Flags: review?(bhackett1024) → review+
(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 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.
Attachment #8562906 - Flags: review?(jdemooij) → review+
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
Attachment #8563488 - Flags: review?(jdemooij) → review+
This is the last stone and improves the undercore.js speed in:
bug 1131099 comment 2
Attachment #8567906 - Flags: review?(jdemooij)
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.
Attachment #8567906 - Flags: review?(jdemooij) → review+
(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.
Why is that a problem?  Structs can have private stuff and constructors and whatnot in C++.  They just become non-POD.
Depends on: 1139152
Note to myself: you forgot "typeof v == boolean"
Flags: needinfo?(hv1989)
Flags: needinfo?(hv1989)
Attachment #8572637 - Flags: review?(jdemooij)
Attachment #8572637 - Flags: review?(jdemooij) → review+
Depends on: 1146410
You need to log in before you can comment on or make changes to this bug.