Closed Bug 1262453 Opened 8 years ago Closed 8 years ago

Remove unneeded MNops

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(2 files)

We currently always add a MNop in the hope it will decrease the lifeness of operations. But that is not always the case. When we have further uses of the operands the MNop is not needed. In that case remove the MNop. It decreases the amount of resumepoints we need to encode and also helps recover instructions a little bit.
Attached patch PatchSplinter Review
Assignee: nobody → hv1989
Attachment #8738564 - Flags: review?(bhackett1024)
Comment on attachment 8738564 [details] [diff] [review]
Patch

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

::: js/src/jit/ValueNumbering.cpp
@@ +728,5 @@
> +            size_t numOperandsLive = 0;
> +            MResumePoint* rp = nop->resumePoint();
> +            MDefinition* result = rp->getOperand(0);
> +            for (size_t j = 0; j < result->numOperands(); j++) {
> +                for (size_t i = 1; i < rp->numOperands(); i++) {

You might also want to check the caller's of the resume point.
Attachment #8738564 - Flags: review?(bhackett1024) → review+
Since constants are "use at uses". It doesn't decrease aliveness when adding a NOP after popping a constant.
Attachment #8739363 - Flags: review?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> Comment on attachment 8738564 [details] [diff] [review]
> Patch
> 
> Review of attachment 8738564 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/ValueNumbering.cpp
> @@ +728,5 @@
> > +            size_t numOperandsLive = 0;
> > +            MResumePoint* rp = nop->resumePoint();
> > +            MDefinition* result = rp->getOperand(0);
> > +            for (size_t j = 0; j < result->numOperands(); j++) {
> > +                for (size_t i = 1; i < rp->numOperands(); i++) {
> 
> You might also want to check the caller's of the resume point.

Yeah, I did this on purpose and now double checked it. However theoretically what you say is correct, it will almost never happen. E.g. it doesn't happen in all of octane. Therefore I don't think it is needed to iterate all caller resume points. It only costs us time.
Comment on attachment 8739363 [details] [diff] [review]
Don't add resume point for constants

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

Q: Don't we want to do that in after the constant propagation, in GVN?

::: js/src/jit/IonBuilder.cpp
@@ +1813,1 @@
>          current->pop();

nit: use MDefinition* def = current->pop();  and check def->isConstant() below.
Attachment #8739363 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/f4c7add07dc6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> Comment on attachment 8739363 [details] [diff] [review]
> Don't add resume point for constants
> 
> Review of attachment 8739363 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Q: Don't we want to do that in after the constant propagation, in GVN?

Yes and no.

Yes: would be more accurate and would potentially find more cases and we could even check other things.
No: We need to keep track of the previous resume point and compare the list of resume points of both to each other.
=> this was just low hanging fruit that is quickly doable resulting in less NOPs needed in the whole graph. Doing this in GVN might yield in even less NOPs but increase in complexity (both in speed and code). And not sure if it worth it yet.
You need to log in before you can comment on or make changes to this bug.