With the testcase in Bug 678063 Comment 1, the useless move %rdx => %rbx (and the other one) produces the following code: > sub $0x8,%rsp > mov %rdx,%rbx > add $0x8,%rsp If stack traffic is not necessary to satisfy a movegroup, the stack pointer should not be modified.
We measured that this was faster than an exchange, and we also decided that keeping free register lists around in movegroups was very difficult. IIRC, Andrew was confident that we wouldn't need to break cycles unless the register pressure was very high - if that's not true, we could consider blacklisting a scratch register. On x64 and ARM that's feasible.
(In reply to David Anderson [:dvander] from comment #1) I think this comment belongs in bug 678063. Will reply there.
In that example, the movegroup extends beyond the stack traffic - it is necessary (unless you meant computing the stack traffic as part of the method frame, but that is hard).
Created attachment 552236 [details] [diff] [review] Reset MoveResolver state. The unnecessary stack traffic was caused by the MoveResolver not resetting hasCycles_. If one LMoveGroup contains a cycle, then it will assume that all future LMoveGroups also contain cycles. This patch adds an explicit, internal resetState() function, and calls it from addMove(). That function resets hasCycles_.
Attachment #552236 - Flags: review?(dvander)
Comment on attachment 552236 [details] [diff] [review] Reset MoveResolver state. Review of attachment 552236 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/MoveResolver.cpp @@ +62,5 @@ > return false; > new (pm) PendingMove(from, to, kind); > pending_.pushBack(pm); > + > + resetState(); Aha, nice catch. r=me with this at the start of resolve() rather than in addMove()
Attachment #552236 - Flags: review?(dvander) → review+
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.