1059187 - filterTypesAtTest - set TypeSet to Undefined/Null in else branch

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Sushant Dinesh]

We're currently filtering undefined/null when we're in the trueBranch. But if we're in the else branch we know for sure that it has to be undefined/null.
This bug is to implement this.

Comment 1

[Sushant Dinesh]

Attachment: [WIP] Set undefined/null in else

Comment 2

[Hannes Verschore [:h4writer]]

Comment on attachment 8480081 [details] [diff] [review]
[WIP] Set undefined/null in else

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

::: js/src/jit/IonBuilder.cpp
@@ +3212,5 @@
>      }
>  
> +    else if (setUndefined || setNull) {
> +        type = subject->resultTypeSet()->setUndefinedOrNull(alloc_->lifoAlloc(), setUndefined
> +                                                                               , setNull);

This is a bit suboptimal. We want to improve typeset to contain the intersection between the old type and undefined/null. Now we just add the undefined/null types. E.g. if the subject's typeset is undefined/anyobject and setUndefined/setNull is true, we only want to have undefined as a result.

(I also think we don't need a new method in jsinfer. I think we can do it with the current available functions.)

Comment 3

[Hannes Verschore [:h4writer]]

So there are also some other cases where if (a) escapes to the the else branch:
http://www.ecma-international.org/ecma-262/5.1/#sec-9.2

So we cannot remove Strings/Int32/double/Booleans.

Following that diagram we can remove the Objects, but we deviate from the spec here for a certain case. We have some objects that "emulate undefined". If they "emulate undefined" they will also go to the "else case". In MIR.cpp we have a check for that: "MaybeEmulatesUndefined". If that is false, we can remove the objects from the else branch.

Comment 4

[Sushant Dinesh]

Attachment: [WIP] Set undefined/null in else

Comment 5

[Sushant Dinesh]

Attachment: Add Undefined/Null in else branch

Comment 6

[Hannes Verschore [:h4writer]]

Comment on attachment 8499731 [details] [diff] [review]
Add Undefined/Null in else branch

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

Something was lost in this patch?
We have two separate paths. One for "removal of undefined/null" and one for "only retain undefined/null".
This patch only has the "only retain undefined/null" path anymore ...

Comment 7

[Sushant Dinesh]

Attachment: Add Undefined/Null in else branch

So I tried to make it as we discussed on IRC. However, I think I did not understand you correctly and found it hard to implement the way you asked me to.
However I combined your idea of having altersUndefined/Null with mine and I think the code is pretty understandable and correct now.
(I've added in more comments too)
Take a look and let me know :)

Comment 8

[Hannes Verschore [:h4writer]]

Comment on attachment 8501352 [details] [diff] [review]
Add Undefined/Null in else branch

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

Actually you are almost there. You defined "filtersUndefined and filtersNull", which is just the same value.
But it is that value you need to differentiate between "filtering/setting".

So if you group everything where "filters" is true, and everything where filters is false. You are there.
I added the "if/else" in the correct place here. (Best look inside the review itself to have the full code.)

::: js/src/jit/IonBuilder.cpp
@@ +3312,5 @@
> +    // |    0     |  0  |  1  |
> +    // ------------------------
> +    // |    1     |  1  |  0  |    0 => setUndefined/Null. 1 => filtersUndefined/Null.
> +    // ------------------------
> +    filtersUndefined = filtersNull = (op == JSOP_STRICTEQ || op == JSOP_EQ) ^ (trueBranch);

bool filters = (op == JSOP_STRICTEQ || op == JSOP_EQ) ^ (trueBranch);

@@ +3315,5 @@
> +    // ------------------------
> +    filtersUndefined = filtersNull = (op == JSOP_STRICTEQ || op == JSOP_EQ) ^ (trueBranch);
> +
> +    // We filter the typeSet. If both the parameters to filter is false,
> +    // The resulting return is just a clone of the original TypeSet.

if (filters) {

@@ +3318,5 @@
> +    // We filter the typeSet. If both the parameters to filter is false,
> +    // The resulting return is just a clone of the original TypeSet.
> +    types::TemporaryTypeSet *type;
> +    type = subject->resultTypeSet()->filter(alloc_->lifoAlloc(), filtersUndefined && altersUndefined,
> +                                                                 filtersNull && altersNull);

} else {

@@ +3326,5 @@
> +
> +    if (altersUndefined && !filtersUndefined) {
> +        flags = flags | types::TYPE_FLAG_UNDEFINED;
> +        if (!subject->resultTypeSet()->maybeEmulatesUndefined()) {
> +            type = type->cloneWithoutObjects(alloc_->lifoAlloc());

I think you can do something like. So we don't need to alloc two type objects:
flags &= ~types::TYPE_FLAG_OBJECT

@@ +3339,3 @@
>      {
> +        types::TemporaryTypeSet *base =
> +                  alloc_->lifoAlloc()->new_<types::TemporaryTypeSet>(flags, static_cast<types::TypeObjectKey**>(nullptr));

Can we have a stack allocation here? I could be wrong but I think that is possible.

@@ +3340,5 @@
> +        types::TemporaryTypeSet *base =
> +                  alloc_->lifoAlloc()->new_<types::TemporaryTypeSet>(flags, static_cast<types::TypeObjectKey**>(nullptr));
> +
> +        type = types::TypeSet::intersectSets(base, type, alloc_->lifoAlloc());
> +    }

}

Comment 9

[Sushant Dinesh]

Attachment: Add Undefined/Null in else branch

Comment 10

[Sushant Dinesh]

Attachment: Add Undefined/Null in else branch

Comment 11

[Sushant Dinesh]

Attachment: Add Undefined/Null in else branch

Comment 12

[Hannes Verschore [:h4writer]]

Comment on attachment 8504544 [details] [diff] [review]
Add Undefined/Null in else branch

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

All nitpicking.

::: js/src/jit/IonBuilder.cpp
@@ +3264,5 @@
>  
> +    // altersUndefined/Null represents if we can filter/set Undefined/Null.
> +    bool altersUndefined, altersNull;
> +
> +    JSOp op = ins->jsop();

Can you remove the newline above.

@@ +3267,5 @@
> +
> +    JSOp op = ins->jsop();
> +
> +    MOZ_ASSERT(op == JSOP_STRICTNE || op == JSOP_NE ||
> +               op == JSOP_STRICTEQ || op == JSOP_EQ);

Plz remove the MOZ_ASSERT.

@@ +3273,5 @@
> +    switch(op) {
> +      case JSOP_STRICTNE:
> +      case JSOP_STRICTEQ:
> +        altersUndefined = (ins->compareType() == MCompare::Compare_Undefined);
> +        altersNull = (ins->compareType() == MCompare::Compare_Null);

No need for ()

@@ +3280,5 @@
> +      case JSOP_EQ:
> +        altersUndefined = altersNull = true;
> +        break;
> +      default:
> +        return true;

replace this line with MOZ_CRASH("relational compares not supported.");

@@ +3296,5 @@
> +    altersUndefined &= subject->mightBeType(MIRType_Undefined);
> +    altersNull &= subject->mightBeType(MIRType_Null);
> +
> +    if (!altersUndefined && !altersNull)
> +        return true;

You add the mightBeType to "altersXXX". This is correct, but I think it complicates the reasoning about what altersXXX contains.

Can you revert this back to the original code?

@@ +3300,5 @@
> +        return true;
> +
> +    types::TemporaryTypeSet *type;
> +
> +    // Filters Undefined/Null.

Replace the comment with:
// Decide if we need to filter the types or set the type.

@@ +3301,5 @@
> +
> +    types::TemporaryTypeSet *type;
> +
> +    // Filters Undefined/Null.
> +    if ((op == JSOP_STRICTEQ || op == JSOP_EQ) ^ (trueBranch)) {

- No need for () on 'trueBranch'
- Move the comment to here.
// Filter undefined/null.

@@ +3308,2 @@
>      } else {
> +        // Set Undefined/Null.

lowercase Undefined/Null

@@ +3308,3 @@
>      } else {
> +        // Set Undefined/Null.
> +        uint32_t flags;

flags = 0;

@@ +3309,5 @@
> +        // Set Undefined/Null.
> +        uint32_t flags;
> +        if (altersUndefined) {
> +            flags |= types::TYPE_FLAG_UNDEFINED;
> +            if (subject->resultTypeSet()->maybeEmulatesUndefined())

Can you add a comment about the emulatesUndefined?

Comment 13

[Sushant Dinesh]

Attachment: Add Undefined/Null in else branch

r+ from previous patch.

Comment 14

[Sushant Dinesh]

Pushed to try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=36c05e240e82

Comment 15

[Hannes Verschore [:h4writer]]

(In reply to Sushant Dinesh from comment #14)
> Pushed to try:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=36c05e240e82

Looks green to me.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f7a7cce07b6

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f7a7cce07b6