[webkit-reviews] review granted: [Bug 21319] Avoid restoring the caller's 'r' value in op_ret : [Attachment 24342] avoid memory writes of callFrame as much as possible

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Maciej Stachowiak <mjs at apple.com>'s
request for review:
Bug 21319: Avoid restoring the caller's 'r' value in op_ret
https://bugs.webkit.org/show_bug.cgi?id=21319

Attachment 24342: avoid memory writes of callFrame as much as possible
https://bugs.webkit.org/attachment.cgi?id=24342&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Great change! Not only faster, but cleaner code too.

-    "call " SYMBOL_STRING(_ZN3JSC7Machine12cti_vm_throwEPPv) "\n"
+    "movl 0x38(%esp), %edx" "\n"
+    "call " SYMBOL_STRING(_ZN3JSC7Machine12cti_vm_throwEPPvPNS_9ExecStateE)
"\n"

+	     mov edi, [esp + 0x38];

I'd like the see a comment like this one:

    "movl 0x38(%esp), %edi" "\n" // Ox38 = 0x0E * 4, 0x0E = CTI_ARGS_callFrame
(see assertion above)

on those lines of code.

It would make this patch smaller and easier to review if the emitCTICall()
change was done first.

+    m_jit.movl_mr(8 + sizeof(void*), X86::ecx, X86::edx);

+    m_jit.movl_mr(8 + sizeof(void*), X86::ecx, X86::edx);

I don't understand the meaning of the magic number 8 here.

+	 m_jit.cmpl_i32r(reinterpret_cast<intptr_t> (jsNull()), X86::edx);

Strange space after ">" on this line of code.

-	 doSetReturnAddressVMThrowTrampoline(&CTI_RETURN_ADDRESS); \
+	 ARG_setCallFrame(callFrame); \
+	 doSetReturnAddressVMThrowTrampoline(&CTI_RETURN_ADDRESS);	\

Unexpected change in whitespace here.

+void Machine::cti_op_put_by_id_generic(void** args, CallFrame* callFrame)

Maybe we should use a type other than void** such as CTIArgs* so we don't have
to do all the typecasting in the ARG_ macros.