[webkit-reviews] review denied: [Bug 123615] [Win] Enable DFG JIT. : [Attachment 234647] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied peavo at outlook.com's request for
review:
Bug 123615: [Win] Enable DFG JIT.
https://bugs.webkit.org/show_bug.cgi?id=123615

Attachment 234647: Patch
https://bugs.webkit.org/attachment.cgi?id=234647&action=review

------- Additional Comments from Mark Lam <mark.lam at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=234647&action=review


I'd suggested some renames of function names so that they are a little more
consistent with each other, and reads more naturally.  Please also look into
the storing of the CallerFrame pinter issue in callWithSlowPathReturnType.

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:155
> +    Call callSlowPathReturnType()

Let's rename "callSlowPathReturnType" to "callWithSlowPathReturnType".

>> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:163
>> +	    // Note: this implementation supports up to 3 parameters.
> 
> I think this comment needs to be removed.

I disagree.  I think this comment is needed.  I would prefer an assertion on
the number of parameters instead, but I don't think there's a way to assert
this right now given that the parameters are set up by a different function
(i.e. setupArgumentsWithExecStateForCallWithSlowPathReturnType).

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:183
> +	   // We also need to allocate the shadow space on the stack for the 4
parameter registers.
> +	   // In addition, we need to allocate 16 bytes for the return value.
> +	   sub64(TrustedImm32(6 * sizeof(int64_t)), X86Registers::esp);
> +
> +	   // The first parameter register should contain a pointer to the
stack allocated space for the return value.
> +	   move(X86Registers::esp, X86Registers::ecx);
> +	   add64(TrustedImm32(4 * sizeof(int64_t)), X86Registers::ecx);
> +
> +	   DataLabelPtr label = moveWithPatch(TrustedImmPtr(0),
scratchRegister);
> +	   Call result = Call(m_assembler.call(scratchRegister),
Call::Linkable);
> +
> +	   add64(TrustedImm32(6 * sizeof(int64_t)), X86Registers::esp);
> +
> +	   // Copy the return value into rax and rdx.
> +	   load64(Address(X86Registers::eax, sizeof(int64_t)),
X86Registers::edx);
> +	   load64(Address(X86Registers::eax), X86Registers::eax);
> +
> +	   ASSERT_UNUSED(label, differenceBetween(label, result) ==
REPTACH_OFFSET_CALL_R11);
> +	   return result;

This code looks stale compared to call() below.  Specifically, I don't see you
storing the ebp (CallerFrame*) on the stack, and adjusting the stack
accordingly for that.  Is that not needed here?

> Source/JavaScriptCore/jit/CCallHelpers.h:802
> +	   // On Windows, an argument maps to the same register (based on its
position), even when there are floating point arguments.

I suggest rephrasing this comment slightly as "On Windows, arguments map to
designated registers based on the argument positions, even when there are
interlaced scalar and floating point arguments."

> Source/JavaScriptCore/jit/CCallHelpers.h:816
> +	   // On Windows, an argument maps to the same register (based on its
position), even when there are floating point arguments.

Ditto with comment.  I suggest using the rephrased comment above.

> Source/JavaScriptCore/jit/CCallHelpers.h:817
> +	   // See http://msdn.microsoft.com/en-us/library/7572ztz4.aspx

This document at this url talks about "Return Values".	I think you meant to us
this url instead; http://msdn.microsoft.com/en-us/library/zthk2dkh.aspx

> Source/JavaScriptCore/jit/CCallHelpers.h:1078
> +    ALWAYS_INLINE void
setupArgumentsSlowPathReturnTypeCallWithExecState(TrustedImm32 arg1)

Please rename "setupArgumentsSlowPathReturnTypeCallWithExecState" to
"setupArgumentsWithExecStateForCallWithSlowPathReturnType".

> Source/JavaScriptCore/jit/JIT.h:268
> +	   Call appendSlowPathReturnTypeCall(const FunctionPtr& function)

Please rename "appendSlowPathReturnTypeCall" to
"appendCallWithSlowPathReturnType".

> Source/JavaScriptCore/jit/JIT.h:270
> +	       Call functionCall = callSlowPathReturnType();

Please rename "callSlowPathReturnType" to "callWithSlowPathReturnType".

> Source/JavaScriptCore/jit/JIT.h:661
> +	   MacroAssembler::Call
appendSlowPathReturnTypeCallWithExceptionCheck(const FunctionPtr&);

Please rename "appendSlowPathReturnTypeCallWithExceptionCheck" to
"appendCallWithExceptionCheckAndSlowPathReturnType".

> Source/JavaScriptCore/jit/JITInlines.h:122
> +ALWAYS_INLINE MacroAssembler::Call
JIT::appendSlowPathReturnTypeCallWithExceptionCheck(const FunctionPtr&
function)

Please rename "appendSlowPathReturnTypeCallWithExceptionCheck" to
"appendCallWithExceptionCheckAndSlowPathReturnType".

> Source/JavaScriptCore/jit/JITInlines.h:125
> +    MacroAssembler::Call call = appendSlowPathReturnTypeCall(function);

Please rename "appendSlowPathReturnTypeCall" to
"appendCallWithSlowPathReturnType".

> Source/JavaScriptCore/jit/JITInlines.h:250
> +    setupArgumentsSlowPathReturnTypeCallWithExecState(TrustedImm32(op));
> +    return appendSlowPathReturnTypeCallWithExceptionCheck(operation);

Please rename "setupArgumentsSlowPathReturnTypeCallWithExecState" to
"setupArgumentsWithExecStateForCallWithSlowPathReturnType", and
"appendSlowPathReturnTypeCallWithExceptionCheck" to
"appendCallWithExceptionCheckAndSlowPathReturnType".