Closed
Bug 1262453
Opened 8 years ago
Closed 8 years ago
Remove unneeded MNops
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: h4writer, Assigned: h4writer)
References
Details
Attachments
(2 files)
1.73 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
1.22 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
Assignee: nobody → hv1989
Attachment #8738564 -
Flags: review?(bhackett1024)
Comment 2•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8738564 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
(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 7•8 years ago
|
||
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+
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f4c7add07dc6
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Comment 9•8 years ago
|
||
(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.
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5d944b43c217
You need to log in
before you can comment on or make changes to this bug.
Description
•