Closed Bug 678066 Opened 13 years ago Closed 13 years ago

IonMonkey: LSRA movegroups always modify stack pointer.

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sstangl, Unassigned)

Details

Attachments

(1 file)

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).
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+
http://hg.mozilla.org/projects/ionmonkey/rev/0fb0af47c876
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.