Closed
Bug 1258748
Opened 8 years ago
Closed 8 years ago
adjustPhiInputs should add MBox in the predecessor blocks only.
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: nbp, Assigned: nbp)
Details
Attachments
(1 file)
955 bytes,
patch
|
jolesen
:
review+
|
Details | Diff | Splinter Review |
Currently adjustPhiInputs adds MBox instruction next to the instruction which flow into the phi node. This can cause performance issues if the previous instruction is in an inner loop, while we only care about the exit path of the inner loop.
Assignee | ||
Comment 1•8 years ago
|
||
This is the same issue as what we previously observed with MSimdBox. Except that MBox is cheaper than MSimdBox.
Attachment #8733431 -
Flags: review?(jolesen)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(nicolas.b.pierron)
Comment 2•8 years ago
|
||
It is also possible that the phi predecessor is in a loop and the definition is outside the loop. Is there a way of finding the least nested loop in the dominator tree path from the phi predecessor to the definition?
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Jakob Stoklund Olesen [:jolesen] from comment #2) > It is also possible that the phi predecessor is in a loop and the definition > is outside the loop. > > Is there a way of finding the least nested loop in the dominator tree path > from the phi predecessor to the definition? Oh, that's a good point. We have the loopDepth counter for each basic block. I do not think we have any way to find the basic block which has the lower loopDepth. I guess we can rely on LICM to move these instruction above the loops if they do not depend on variables which are within the loop.
Comment 4•8 years ago
|
||
Comment on attachment 8733431 [details] [diff] [review] adjustPhiInputs: Add MBox in the predecessor block instead of the definition block. Review of attachment 8733431 [details] [diff] [review]: ----------------------------------------------------------------- Yes, if LICM will move the boxes, then it's fine to insert them in the phi predecessor.
Attachment #8733431 -
Flags: review?(jolesen) → review+
Comment 6•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/86019126140c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(nicolas.b.pierron)
You need to log in
before you can comment on or make changes to this bug.
Description
•