Bug 122460 – Transition op_new_* JITStubs to JIT operations

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Move the following JITStubs to CCallHelper functions placing the new function in JITOperations:
    cti_op_new_array
    cti_op_new_array_with_size
    cti_op_new_array_buffer
    cti_op_new_func
    cti_op_new_func_exp
    cti_op_new_object

[Mark Lam]

Created attachment 213990 [details]
the patch.

[WebKit Commit Bot]

Attachment 213990 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/dfg/DFGOperations.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp', u'Source/JavaScriptCore/jit/CCallHelpers.h', u'Source/JavaScriptCore/jit/JIT.h', u'Source/JavaScriptCore/jit/JITInlines.h', u'Source/JavaScriptCore/jit/JITOpcodes.cpp', u'Source/JavaScriptCore/jit/JITOpcodes32_64.cpp', u'Source/JavaScriptCore/jit/JITOperations.cpp', u'Source/JavaScriptCore/jit/JITOperations.h', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/jit/JITStubs.h', u'Source/JavaScriptCore/jit/JSInterfaceJIT.h']" exit_code: 1
Source/JavaScriptCore/jit/JIT.h:54:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

Comment on attachment 213990 [details]
the patch.

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*, ...)

[Mark Lam]

Created attachment 214003 [details]
patch 2: with Michael's feedback applied.

[WebKit Commit Bot]

Attachment 214003 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/dfg/DFGOperations.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp', u'Source/JavaScriptCore/jit/CCallHelpers.h', u'Source/JavaScriptCore/jit/JIT.h', u'Source/JavaScriptCore/jit/JITInlines.h', u'Source/JavaScriptCore/jit/JITOpcodes.cpp', u'Source/JavaScriptCore/jit/JITOpcodes32_64.cpp', u'Source/JavaScriptCore/jit/JITOperations.cpp', u'Source/JavaScriptCore/jit/JITOperations.h', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/jit/JITStubs.h', u'Source/JavaScriptCore/jit/JSInterfaceJIT.h']" exit_code: 1
Source/JavaScriptCore/jit/JIT.h:54:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

Comment on attachment 214003 [details]
patch 2: with Michael's feedback applied.

View in context: https://bugs.webkit.org/attachment.cgi?id=214003&action=review

r+

> Source/JavaScriptCore/jit/JITInlines.h:1068
> +#undef EABI_32BIT_DUMMY_ARG
> +#undef SH4_32BIT_DUMMY_ARG

This should be inside the #if USE(JSVALUE32_64) / #endif.

Move back to ToT placement for these defines. This puts the #define / #undef immediately around the use.

[Mark Lam]

Thanks for the review.

Per our offline discussion, I've moved the J_JITOperation_EAapJ callOperation() into the JSVALUE32_64 block along with the EABI macros. 

Landed in r157313: <http://trac.webkit.org/r157313>.

[Geoffrey Garen]

Nice!