Closed Bug 514827 Opened 15 years ago Closed 15 years ago

TM: avoid redundant stack pointer updates when calling functions [nanojit]

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: gal, Assigned: gal)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 3 obsolete files)

We currently generate pretty naive code for function calls, in particular when it comes to floating point arguments. Each floating point push is emulated via sub esp, 8 and then a FP move and the call does a separate sub esp, 4 for stack alignment.

  00002e9f9a   sub esp,4              ecx(cx) edx($global0) ebx(state) esi(sp) xmm0(#0x3ff80000:0)
  00002e9f9d   sub esp,8              ecx(cx) edx($global0) ebx(state) esi(sp) xmm0(#0x3ff80000:0)
  00002e9fa0   movq 0(esp),xmm0       ecx(cx) edx($global0) ebx(state) esi(sp) xmm0(#0x3ff80000:0)
  00002e9fa5   push 0                 ecx(cx) edx($global0) ebx(state) esi(sp)
  00002e9fa7   call js_Array_dense_se ebx(state) esi(sp)

The attached patch adjust the stack once, and then uses move instructions instead of push to place the arguments.

  00002e8f97   sub esp,16             ecx(cx) edx($global0) ebx(state) esi(sp) xmm0(#0x3ff80000:0)
  00002e8f9a   movq 4(esp),xmm0       ecx(cx) edx($global0) ebx(state) esi(sp) xmm0(#0x3ff80000:0)
  00002e8fa0   mov 0(esp),0           ecx(cx) edx($global0) ebx(state) esi(sp)
  00002e8fa7   call js_Array_dense_se ebx(state) esi(sp)

I can't measure any speed from this. We save a couple redundant stack moves, but the encoding is less compact (push is often a single byte). Also, since push can do memory to stack transfers but moves can't do memory to memory, I have to reload into a register in case of an argument thats currently spilled. So overall a wash.

I mostly want to land this patch because its a prerequisite for omitting the frame pointer (EBP), which I am working on in a separate bug.
Attached patch patch (obsolete) — Splinter Review
Assignee: general → gal
Attached patch patch, this time not empty (obsolete) — Splinter Review
Attachment #398832 - Attachment is obsolete: true
Attachment #398833 - Flags: review?(rreitmai)
Attachment #398833 - Flags: review?(edwsmith)
(GpRegs ^ SavedRegs) can become ESP, which is the last place we want to load arguments into. (GpRegs & (~SavedRegs)) is what we want.
Attachment #398833 - Attachment is obsolete: true
Attachment #398872 - Flags: review?(rreitmai)
Attachment #398872 - Flags: review?(edwsmith)
Attachment #398833 - Flags: review?(rreitmai)
Attachment #398833 - Flags: review?(edwsmith)
Comment on attachment 398872 [details] [diff] [review]
be more careful about which register to load a memory argument into

Are the macros called from asm_farg() supposed to be part of this patch?   

Also, have you seen more spilling with the change for findReg to use the set (~Saved & GpRegs) ?
Attachment #398872 - Flags: review?(rreitmai) → review+
Saved = EBX, EI, EDI
GpRegs = Saved | EAX ECX EDX

~Saved & GpRegs = EAX ECX EDX
Saved ^ GpRegs = EAX ECX EDX

So no, its identical right now, but ~Saved & GpRegs seems like the right way to say what I am trying to say.

If you are wondering whether the findReg itself causes more spilling, I am wondering that too. Its not something I can measure, but I know we hit that path. I will micro-benchmark it before committing. If its a problem, the order in which we "push" (move) arguments in place is flexible now, so I could push out any args currently held in EAX ECX EDX before clobbering them (if thats really an issue here).
The changes in farg are necessary because we are no longer physically move ESP as we write args. stkd instead virtually moves it, ad farg was update to write to a non-0 offset relative to ESP (stkd) and it also doesn't have to update ESP any more. We do that once and separately.
Attachment #398872 - Flags: review?(edwsmith)
Attachment #405128 - Flags: review?(dvander)
Attachment #405128 - Flags: review?(dvander) → review+
http://hg.mozilla.org/tracemonkey/rev/5c771bffa60b
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/5c771bffa60b
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Blocks: 522314
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: