Closed
Bug 1082448
Opened 10 years ago
Closed 9 years ago
Add filter in improveTypesAtTest for default case
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: eternalsushant, Assigned: eternalsushant, Mentored)
References
Details
Attachments
(1 file, 3 obsolete files)
2.49 KB,
patch
|
eternalsushant
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•10 years ago
|
Summary: Add filteringAtTest for default case → Add filter in improveTypesAtTest for default case
Comment 1•10 years ago
|
||
Could I get a DXR link here, so I can share this with the world?
Comment 2•10 years ago
|
||
(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.
Updated•10 years ago
|
Assignee: nobody → eternalsushant
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8517356 -
Flags: review?(hv1989)
Comment 4•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8517356 -
Flags: review?(hv1989)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8517356 -
Attachment is obsolete: true
Attachment #8517567 -
Flags: review?(hv1989)
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8517567 -
Attachment is obsolete: true
Attachment #8524640 -
Flags: review?(hv1989)
Comment 8•10 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
r+ from previous patch
Attachment #8524640 -
Attachment is obsolete: true
Attachment #8527642 -
Flags: review+
Assignee | ||
Comment 10•9 years ago
|
||
Pushed to try server: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=523263029d4a
Updated•9 years ago
|
Keywords: checkin-needed
Comment 12•9 years ago
|
||
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.
Description
•