[webkit-reviews] review denied: [Bug 122460] Transition op_new_* JITStubs to JIT operations : [Attachment 213990] the patch.

[bugzilla-daemon at webkit.org]

Michael Saboff <msaboff at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 122460: Transition op_new_* JITStubs to JIT operations
https://bugs.webkit.org/show_bug.cgi?id=122460

Attachment 213990: the patch.
https://bugs.webkit.org/attachment.cgi?id=213990&action=review

------- Additional Comments from Michael Saboff <msaboff at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=213990&action=review


Please make the changes suggested.

> Source/JavaScriptCore/jit/JIT.h:872
> +	   MacroAssembler::Call callOperation(J_JITOperation_EAapJcpZ, int,
ArrayAllocationProfile*, GPRReg, int);
> +	   MacroAssembler::Call callOperation(J_JITOperation_EAapJcpZ, int,
ArrayAllocationProfile*, const JSValue*, int);

Make the int arguments int32_t.

> Source/JavaScriptCore/jit/JIT.h:879
> +	   MacroAssembler::Call callOperation(J_JITOperation_EP, int, void*);
> +	   MacroAssembler::Call callOperation(J_JITOperation_ESt, int,
Structure*);

Please move these up with the other J_* call operations.

> Source/JavaScriptCore/jit/JIT.h:893
> +#if USE(JSVALUE64)
> +	   MacroAssembler::Call callOperation(J_JITOperation_EAapJ, int,
ArrayAllocationProfile*, GPRReg);
> +#else
> +	   MacroAssembler::Call callOperation(J_JITOperation_EAapJ, int,
ArrayAllocationProfile*, GPRReg, GPRReg);
> +#endif
> +

Move these to be with the other callOperation(J_*) declarations.

> Source/JavaScriptCore/jit/JITInlines.h:43
> +#if USE(JSVALUE32_64)
> +// EncodedJSValue in JSVALUE32_64 is a 64-bit integer. When being compiled
in ARM EABI, it must be aligned even-numbered register (r0, r2 or [sp]).
> +// To avoid assemblies from using wrong registers, let's occupy r1 or r3
with a dummy argument when necessary.
> +#if (COMPILER_SUPPORTS(EABI) && CPU(ARM)) || CPU(MIPS)
> +#define EABI_32BIT_DUMMY_ARG      TrustedImm32(0),
> +#else
> +#define EABI_32BIT_DUMMY_ARG
> +#endif
> +#endif // (JSVALUE32_64)
> +

This is already in ToT along with the define of SH4_32BIT_DUMMY_ARG.  This may
be a better location for the defines.  Rebaseline and either put both defines
here or keep the current location.

> Source/JavaScriptCore/jit/JITInlines.h:264
> +ALWAYS_INLINE MacroAssembler::Call
JIT::callOperation(J_JITOperation_EAapJcpZ operation, int dst,
ArrayAllocationProfile* arg1, GPRReg arg2, int arg3)

Make the int argument int32_t.

> Source/JavaScriptCore/jit/JITInlines.h:270
> +ALWAYS_INLINE MacroAssembler::Call
JIT::callOperation(J_JITOperation_EAapJcpZ operation, int dst,
ArrayAllocationProfile* arg1, const JSValue* arg2, int arg3)

Make the int argument int32_t.

> Source/JavaScriptCore/jit/JITOpcodes.cpp:1244
> +    ArrayAllocationProfile* profile =
currentInstruction[4].u.arrayAllocationProfile;
> +    int valuesIndex = currentInstruction[2].u.operand;
> +    int size = currentInstruction[3].u.operand;
> +    move(TrustedImmPtr(valuesIndex * sizeof(Register)), regT0);
> +    addPtr(callFrameRegister, regT0);
> +    callOperation(operationNewArrayWithProfile, dst, profile, regT0, size);

Handle the arguments in byte code order.
I don't think you need temporaries for all the arguments.
Use the three op addPtr() variant - addPtr(TrustedImm32(valuesIndex *
sizeof(Register)), callFrameRegister, regT0)

> Source/JavaScriptCore/jit/JITOpcodes.cpp:1251
> +    int dst = currentInstruction[1].u.operand;
> +    ArrayAllocationProfile* profile =
currentInstruction[3].u.arrayAllocationProfile;
> +    int sizeIndex = currentInstruction[2].u.operand;

ditto

> Source/JavaScriptCore/jit/JITOpcodes.cpp:1267
> +    ArrayAllocationProfile* profile =
currentInstruction[4].u.arrayAllocationProfile;
> +    int valuesIndex = currentInstruction[2].u.operand;
> +    int size = currentInstruction[3].u.operand;
> +    const JSValue* values = codeBlock()->constantBuffer(valuesIndex);

ditto

> Source/JavaScriptCore/jit/JITOperations.cpp:784
> +EncodedJSValue JIT_OPERATION operationNewObjectReturningJSValue(ExecState*
exec, Structure* structure)
> +{
> +    VM* vm = &exec->vm();
> +    NativeCallFrameTracer tracer(vm, exec);
> +    
> +    return JSValue::encode(constructEmptyObject(exec, structure));
> +}

Instead of adding this new operation, move the DFG version of
operationNewObject to JITOperations and then make a
callOperation(C_JITOperation_ESt) for the baseline JIT that will
emitStoreCell() the result.

> Source/JavaScriptCore/jit/JITOperations.h:-59
> -    V: void
> -    J: JSValue
> -    P: pointer (void*)
> +    A: JSArray*
> +    Aap: ArrayAllocationProfile*
>      C: JSCell*
>      Cb: CodeBlock*
> -    A: JSArray*
> +    D: double
> +    I: StringImpl*
> +    J: JSValue
> +    Jcp: const JSValue*
> +    P: pointer (void*)
>      R: Register
>      S: size_t
> +    St: Structure*
> +    V: void
>      Z: int32_t
> -    D: double
> -    I: StringImpl*

If you are going to reorganize these, please add the missing ones
(InlineCallFrame*, JSString*, ...)