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)
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.
Assignee | ||
Comment 1•15 years ago
|
||
Assignee: general → gal
Assignee | ||
Comment 2•15 years ago
|
||
Attachment #398832 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Attachment #398833 -
Flags: review?(rreitmai)
Attachment #398833 -
Flags: review?(edwsmith)
Assignee | ||
Comment 3•15 years ago
|
||
(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 4•15 years ago
|
||
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+
Assignee | ||
Comment 5•15 years ago
|
||
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).
Assignee | ||
Comment 6•15 years ago
|
||
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.
Assignee | ||
Updated•15 years ago
|
Attachment #398872 -
Flags: review?(edwsmith)
Assignee | ||
Comment 7•15 years ago
|
||
This saves a bunch of stack adjustments.
Attachment #398872 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Attachment #405128 -
Flags: review?(dvander)
Updated•15 years ago
|
Attachment #405128 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 8•15 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/5c771bffa60b
Whiteboard: fixed-in-tracemonkey
Comment 9•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/5c771bffa60b
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•