[Webkit-unassigned] [Bug 123615] [Win] Enable DFG JIT.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=123615


Mark Lam <mark.lam at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #234647|review?                     |review-
               Flag|                            |




--- Comment #39 from Mark Lam <mark.lam at apple.com>  2014-07-10 14:23:24 PST ---
(From update of attachment 234647)
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".

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.