Closed Bug 1082448 Opened 10 years ago Closed 9 years ago

Add filter in improveTypesAtTest for default case

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: eternalsushant, Assigned: eternalsushant, Mentored)

References

Details

Attachments

(1 file, 3 obsolete files)

Right now the filterTypesAtTest include cases like MIsObject, MIsCompare etc. But there is no filtering for the default case. i.e. for something like:
if (a) {
 ...
} else {
 ...
}

The aim of this bug is to add filtering in these cases too.
Summary: Add filteringAtTest for default case → Add filter in improveTypesAtTest for default case
Could I get a DXR link here, so I can share this with the world?
(In reply to Mike Hoye [:mhoye] from comment #1)
> Could I get a DXR link here, so I can share this with the world?

Sushant will probably fix this himself. He has the hardest part of the patch already figured out.
Depends on: 1034184
Assignee: nobody → eternalsushant
Attachment #8517356 - Flags: review?(hv1989)
Comment on attachment 8517356 [details] [diff] [review]
[WIP] Add filtering for default case

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

::: js/src/jit/IonBuilder.cpp
@@ +3407,5 @@
>  
>        case MDefinition::Op_Compare:
>          return improveTypesAtCompare(ins->toCompare(), trueBranch, test);
>  
> +      default: {

Can you add comment here:
// By default a MTest tests ToBoolean(input). As a result in the true branch we can filter undefined and null. In the false branch we can only encounter undefined, null, false, 0, "" and objects that emulate undefined.

@@ +3414,5 @@
> +        if (!ins->resultTypeSet() || (!ins->mightBeType(MIRType_Undefined) && 
> +                                      !ins->mightBeType(MIRType_Null)))
> +        {
> +            return true;
> +        }

if (!ins->resultTypeSet())
    return true;

Needs to stay here. But the extra tests should be part of trueBranch.

@@ +3421,5 @@
> +
> +        // Decide either to set or filter.
> +        if (trueBranch) {
> +            // Filter undefined/null.
> +            type = ins->resultTypeSet()->filter(alloc_->lifoAlloc(), true, true);

Add the mightBeType tests here.

@@ +3423,5 @@
> +        if (trueBranch) {
> +            // Filter undefined/null.
> +            type = ins->resultTypeSet()->filter(alloc_->lifoAlloc(), true, true);
> +        } else {
> +            // Set undefined/null.

Remove this comment.

@@ +3430,5 @@
> +            uint32_t flags = types::TYPE_FLAG_PRIMITIVE;
> +            // If the typeset does not emulate undefined. Then we can filter out objects.
> +            if (!ins->resultTypeSet()->maybeEmulatesUndefined()) {
> +                flags &= ~types::TYPE_FLAG_ANYOBJECT;
> +            }

Test here if the intersection will remove types, before doing the intersectSets function:
if (flags == resultTypeSet().flags() && resultTypeSet().getObjectCount() == 0)
     return true;

(Btw remove the {} if there is only one line in the if body)
Attachment #8517356 - Flags: review?(hv1989)
Attached patch Add filtering to default case (obsolete) — Splinter Review
Attachment #8517356 - Attachment is obsolete: true
Attachment #8517567 - Flags: review?(hv1989)
Comment on attachment 8517567 [details] [diff] [review]
Add filtering to default case

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

::: js/src/jit/IonBuilder.cpp
@@ +3413,5 @@
> +      // and objects that emulate undefined.
> +      default: {
> +        // If ins does not have a typeset we return as we cannot optimize.
> +        if (!ins->resultTypeSet() || !ins->resultTypeSet()->unknown())
> +            return true;

"!ins->resultTypeSet()->unknown()" seems totally incorrect to me. Only optimize if typeset is unknown?

@@ +3435,5 @@
> +            // If the typeset does not emulate undefined. Then we can filter out objects.
> +            if (!ins->resultTypeSet()->maybeEmulatesUndefined())
> +                flags &= ~types::TYPE_FLAG_ANYOBJECT;
> +
> +            if (!ins->resultTypeSet()->hasAnyFlag(~flags) &&

Can you add comment above:
// Only intersect the typset if it will generate a more narrow typeset.
Attachment #8517567 - Flags: review?(hv1989)
Attached patch Add filtering to default case (obsolete) — Splinter Review
Attachment #8517567 - Attachment is obsolete: true
Attachment #8524640 - Flags: review?(hv1989)
Comment on attachment 8524640 [details] [diff] [review]
Add filtering to default case

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

Can you adjust title to:
Bug 1082448: IonMonkey: ..., r=h4writer ?
Attachment #8524640 - Flags: review?(hv1989) → review+
r+ from previous patch
Attachment #8524640 - Attachment is obsolete: true
Attachment #8527642 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ef4e7eebec3b
Status: UNCONFIRMED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.