filterTypesAtTest - set TypeSet to Undefined/Null in else branch

RESOLVED FIXED in mozilla36

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Sushant Dinesh, Assigned: Sushant Dinesh)

Tracking

unspecified
mozilla36
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Depends on: 1034184
(Assignee)

Comment 1

3 years ago
Created attachment 8480081 [details] [diff] [review]
[WIP] Set undefined/null in else
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.)
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.
(Assignee)

Comment 4

3 years ago
Created attachment 8499626 [details] [diff] [review]
[WIP] Set undefined/null in else
Attachment #8480081 - Attachment is obsolete: true
(Assignee)

Comment 5

3 years ago
Created attachment 8499731 [details] [diff] [review]
Add Undefined/Null in else branch
Attachment #8499626 - Attachment is obsolete: true
Attachment #8499731 - Flags: review?(hv1989)
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 ...
Attachment #8499731 - Flags: review?(hv1989)
(Assignee)

Comment 7

3 years ago
Created attachment 8501352 [details] [diff] [review]
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 :)
Attachment #8499731 - Attachment is obsolete: true
Attachment #8501352 - Flags: review?(hv1989)
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());
> +    }

}
Attachment #8501352 - Flags: review?(hv1989)
(Assignee)

Comment 9

3 years ago
Created attachment 8501674 [details] [diff] [review]
Add Undefined/Null in else branch
Attachment #8501352 - Attachment is obsolete: true
Attachment #8501674 - Flags: review?(hv1989)
(Assignee)

Comment 10

3 years ago
Created attachment 8501948 [details] [diff] [review]
Add Undefined/Null in else branch
Attachment #8501674 - Attachment is obsolete: true
Attachment #8501674 - Flags: review?(hv1989)
Attachment #8501948 - Flags: review?(hv1989)
(Assignee)

Comment 11

3 years ago
Created attachment 8504544 [details] [diff] [review]
Add Undefined/Null in else branch
Attachment #8501948 - Attachment is obsolete: true
Attachment #8501948 - Flags: review?(hv1989)
Attachment #8504544 - Flags: review?(hv1989)
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?
Attachment #8504544 - Flags: review?(hv1989) → review+
(Assignee)

Comment 13

3 years ago
Created attachment 8504587 [details] [diff] [review]
Add Undefined/Null in else branch

r+ from previous patch.
Attachment #8504544 - Attachment is obsolete: true
Attachment #8504587 - Flags: review+
(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.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f7a7cce07b6
Assignee: nobody → eternalsushant
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2f7a7cce07b6
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.