Bug 225771 – Implement Baseline JIT property access slow paths using JIT thunks.

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

[Mark Lam]

Patch coming.

[Mark Lam]

Created attachment 428543 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 428543 [details]
proposed patch.

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

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:128
> +{

Let's add a comment that this works only in Baseline and ensure that this is only used in Baseline. It is getting JSGlobalObject from CodeBlock in CallFrame. This is valid only in Baseline and LLInt since this JSGlobalObject is different from the expected one in DFG / FTL if inlining happens.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:150
> +    jit.prepareCallOperation(vm);
> +

Now, it is already set up the arguments. So how about tail-call to the operation function from this thunk instead of returning to the caller?

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:252
> +MacroAssemblerCodeRef<JITThunkPtrTag> JIT::slow_op_xxx_private_name_prepareCallGenerator(VM& vm)
> +{
> +    JIT jit(vm);
> +
> +    jit.tagReturnAddress();
> +
> +    constexpr GPRReg bytecodeOffsetReg = argumentGPR3;
> +    jit.store32(bytecodeOffsetReg, tagFor(CallFrameSlot::argumentCountIncludingThis));
> +
> +    // arg3 now set to regT1 (argumentGPR1 on ARM64 & X86_64). argumentGPR1 is now free.
> +    jit.move(argumentGPR1, argumentGPR3);
> +    // arg1 is now set to stubInfo. argumentGPR2 is now free.
> +    jit.move(argumentGPR2, argumentGPR1);
> +    // arg2 is now set to regT0.
> +    // - ARM64: regT0 is argumentGPR0: argumentGPR0 is now free.
> +    // - X86_64: regT0 is not an argument GPR. So, don't care.
> +    jit.move(regT0, argumentGPR2);
> +    // arg0 is now set to globalObject.
> +    jit.loadPtr(addressFor(CallFrameSlot::codeBlock), argumentGPR0);
> +    jit.loadPtr(Address(argumentGPR0, CodeBlock::offsetOfGlobalObject()), argumentGPR0);
> +
> +    jit.prepareCallOperation(vm);
> +    jit.ret();
> +
> +    LinkBuffer patchBuffer(jit, GLOBAL_THUNK_ID, LinkBuffer::Profile::Thunk);
> +    return FINALIZE_CODE(patchBuffer, JITThunkPtrTag, "Baseline: slow_op_xxx_private_name_prepareCall");
> +}
> +#endif // ENABLE(EXTRA_CTI_THUNKS)

Ditto

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:633
> +MacroAssemblerCodeRef<JITThunkPtrTag> JIT::slow_op_put_xxx_prepareCallGenerator(VM& vm)
> +{
> +    JIT jit(vm);
> +
> +    jit.tagReturnAddress();
> +
> +    constexpr GPRReg bytecodeOffsetReg = argumentGPR0;
> +    jit.store32(bytecodeOffsetReg, tagFor(CallFrameSlot::argumentCountIncludingThis));
> +
> +    jit.loadPtr(JIT::addressFor(CallFrameSlot::codeBlock), JIT::argumentGPR0);
> +    jit.loadPtr(JIT::Address(argumentGPR0, CodeBlock::offsetOfGlobalObject()), JIT::argumentGPR0);
> +
> +    jit.prepareCallOperation(vm);
> +    jit.ret();
> +
> +    LinkBuffer patchBuffer(jit, GLOBAL_THUNK_ID, LinkBuffer::Profile::Thunk);
> +    return FINALIZE_CODE(patchBuffer, JITThunkPtrTag, "Baseline: slow_op_put_xxx_prepareCall");
> +}
> +#endif // ENABLE(EXTRA_CTI_THUNKS)

Ditto.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:838
> +#if ENABLE(EXTRA_CTI_THUNKS)
> +MacroAssemblerCodeRef<JITThunkPtrTag> JIT::slow_op_del_by_xxx_prepareCallGenerator(VM& vm)
> +{
> +    JIT jit(vm);
> +
> +    jit.tagReturnAddress();
> +
> +    constexpr GPRReg bytecodeOffsetReg = argumentGPR0;
> +    jit.store32(bytecodeOffsetReg, tagFor(CallFrameSlot::argumentCountIncludingThis));
> +
> +    // arg4 already has ecmaMode.
> +    // arg3 already has CacheableIdentifier / property.
> +    // arg2 already has base.
> +    // arg1 already has stubInfo.
> +    // arg0 is now set to globalObject.
> +    jit.loadPtr(JIT::addressFor(CallFrameSlot::codeBlock), JIT::argumentGPR0);
> +    jit.loadPtr(JIT::Address(argumentGPR0, CodeBlock::offsetOfGlobalObject()), JIT::argumentGPR0);
> +
> +    jit.prepareCallOperation(vm);
> +    jit.ret();
> +
> +    LinkBuffer patchBuffer(jit, GLOBAL_THUNK_ID, LinkBuffer::Profile::Thunk);
> +    return FINALIZE_CODE(patchBuffer, JITThunkPtrTag, "Baseline: slow_op_del_by_id_prepareCall");
> +}
> +#endif // ENABLE(EXTRA_CTI_THUNKS)

Ditto.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1157
> +#if ENABLE(EXTRA_CTI_THUNKS)
> +MacroAssemblerCodeRef<JITThunkPtrTag> JIT::slow_op_get_by_id_prepareCallGenerator(VM& vm)
> +{
> +    JIT jit(vm);
> +
> +    jit.tagReturnAddress();
> +
> +    constexpr GPRReg bytecodeOffsetReg = argumentGPR2;
> +    jit.store32(bytecodeOffsetReg, tagFor(CallFrameSlot::argumentCountIncludingThis));
> +
> +    // arg3 already has CacheableIdentifier.
> +    // arg2 is now set to regT0.
> +    // - ARM64: regT0 is argumentGPR0: argumentGPR0 is now free.
> +    // - X86_64: regT0 is not an argument GPR. So, don't care.
> +    jit.move(regT0, argumentGPR2);
> +    // arg1 already has stubInfo.
> +    // arg0 is now set to globalObject.
> +    jit.loadPtr(JIT::addressFor(CallFrameSlot::codeBlock), JIT::argumentGPR0);
> +    jit.loadPtr(JIT::Address(argumentGPR0, CodeBlock::offsetOfGlobalObject()), JIT::argumentGPR0);
> +
> +    jit.prepareCallOperation(vm);
> +    jit.ret();
> +
> +    LinkBuffer patchBuffer(jit, GLOBAL_THUNK_ID, LinkBuffer::Profile::Thunk);
> +    return FINALIZE_CODE(patchBuffer, JITThunkPtrTag, "Baseline: slow_op_get_by_id_prepareCall");
> +}
> +#endif // ENABLE(EXTRA_CTI_THUNKS)

Ditto.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1228
> +#if ENABLE(EXTRA_CTI_THUNKS)
> +MacroAssemblerCodeRef<JITThunkPtrTag> JIT::slow_op_get_by_id_with_this_prepareCallGenerator(VM& vm)
> +{
> +    JIT jit(vm);
> +
> +    jit.tagReturnAddress();
> +
> +    constexpr GPRReg bytecodeOffsetReg = argumentGPR3;
> +    jit.store32(bytecodeOffsetReg, tagFor(CallFrameSlot::argumentCountIncludingThis));
> +
> +    // arg4 already has CacheableIdentifier.
> +    // arg3 is now set to regT1.
> +    // - ARM64 & X86_64: regT1 is argumentGPR1: argumentGPR1 is now free.
> +    jit.move(regT1, argumentGPR3);
> +    // arg1 is now set to stubInfo. argumentGPR2 is now free.
> +    jit.move(argumentGPR2, argumentGPR1);
> +    // arg2 is now regT0.
> +    // - ARM64: regT0 is argumentGPR0: argumentGPR0 is now free.
> +    // - X86_64: regT0 is not an argument GPR. So, don't care.
> +    jit.move(regT0, argumentGPR2);
> +    // arg0 is now set to globalObject.
> +    jit.loadPtr(JIT::addressFor(CallFrameSlot::codeBlock), JIT::argumentGPR0);
> +    jit.loadPtr(JIT::Address(argumentGPR0, CodeBlock::offsetOfGlobalObject()), JIT::argumentGPR0);
> +
> +    jit.prepareCallOperation(vm);
> +    jit.ret();
> +
> +    LinkBuffer patchBuffer(jit, GLOBAL_THUNK_ID, LinkBuffer::Profile::Thunk);
> +    return FINALIZE_CODE(patchBuffer, JITThunkPtrTag, "Baseline: slow_op_get_by_id_with_this_prepareCall");
> +}
> +#endif // ENABLE(EXTRA_CTI_THUNKS)

Ditto.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1326
> +#if ENABLE(EXTRA_CTI_THUNKS)
> +MacroAssemblerCodeRef<JITThunkPtrTag> JIT::slow_op_put_by_id_prepareCallGenerator(VM& vm)
> +{
> +    JIT jit(vm);
> +
> +    jit.tagReturnAddress();
> +
> +    constexpr GPRReg bytecodeOffsetReg = argumentGPR2;
> +    jit.store32(bytecodeOffsetReg, tagFor(CallFrameSlot::argumentCountIncludingThis));
> +
> +    // arg4 already has CacheableIdentifier.
> +    // arg2 is now set to regT1.
> +    // - ARM64 & X86_64: regT1 is argumentGPR1: argumentGPR1 is now free.
> +    jit.move(regT1, argumentGPR2);
> +    // arg1 is now set to stubInfo. argumentGPR3 is now free.
> +    jit.move(argumentGPR3, argumentGPR1);
> +    // arg3 is now regT0.
> +    // - ARM64: regT0 is argumentGPR0: argumentGPR0 is now free.
> +    // - X86_64: regT0 is not an argument GPR. So, don't care.
> +    jit.move(regT0, argumentGPR3);
> +    // arg0 is now set to globalObject.
> +    jit.loadPtr(JIT::addressFor(CallFrameSlot::codeBlock), JIT::argumentGPR0);
> +    jit.loadPtr(JIT::Address(argumentGPR0, CodeBlock::offsetOfGlobalObject()), JIT::argumentGPR0);
> +
> +    jit.prepareCallOperation(vm);
> +    jit.ret();
> +
> +    LinkBuffer patchBuffer(jit, GLOBAL_THUNK_ID, LinkBuffer::Profile::Thunk);
> +    return FINALIZE_CODE(patchBuffer, JITThunkPtrTag, "Baseline: slow_op_put_by_id_prepareCall");
> +}
> +#endif // ENABLE(EXTRA_CTI_THUNKS)

Ditto.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1715
> +#if ENABLE(EXTRA_CTI_THUNKS)
> +MacroAssemblerCodeRef<JITThunkPtrTag> JIT::slow_op_get_from_scopeGenerator(VM& vm)
> +{
> +    JIT jit(vm);
> +
> +#if CPU(X86_64)
> +    jit.push(X86Registers::ebp);
> +#elif CPU(ARM64)
> +    jit.tagReturnAddress();
> +    jit.pushPair(framePointerRegister, linkRegister);
> +#endif
> +
> +    constexpr GPRReg bytecodeOffsetReg = argumentGPR2;
> +    jit.store32(bytecodeOffsetReg, tagFor(CallFrameSlot::argumentCountIncludingThis));
> +
> +    jit.loadPtr(addressFor(CallFrameSlot::codeBlock), argumentGPR3);
> +    jit.loadPtr(Address(argumentGPR3, CodeBlock::offsetOfGlobalObject()), argumentGPR0);
> +    jit.loadPtr(Address(argumentGPR3, CodeBlock::offsetOfInstructionsRawPointer()), argumentGPR1);
> +    jit.addPtr(bytecodeOffsetReg, argumentGPR1);
> +
> +    jit.prepareCallOperation(vm);
> +    CCallHelpers::Call operation = jit.call(OperationPtrTag);
> +    CCallHelpers::Jump exceptionCheck = jit.emitExceptionCheck(vm);
> +
> +#if CPU(X86_64)
> +    jit.pop(X86Registers::ebp);
> +#elif CPU(ARM64)
> +    jit.popPair(framePointerRegister, linkRegister);
> +#endif
> +    jit.ret();
> +
> +    LinkBuffer patchBuffer(jit, GLOBAL_THUNK_ID, LinkBuffer::Profile::Thunk);
> +    patchBuffer.link(operation, FunctionPtr<OperationPtrTag>(operationGetFromScope));
> +    auto handler = vm.jitStubs->existingCTIStub(popThunkStackPreservesAndHandleExceptionGenerator, NoLockingNecessary);
> +    patchBuffer.link(exceptionCheck, CodeLocationLabel(handler.retaggedCode<NoPtrTag>()));
> +    return FINALIZE_CODE(patchBuffer, JITThunkPtrTag, "Baseline: slow_op_get_from_scope");
>  }
> +#endif // ENABLE(EXTRA_CTI_THUNKS)

Ditto.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1928
> +#if ENABLE(EXTRA_CTI_THUNKS)
> +MacroAssemblerCodeRef<JITThunkPtrTag> JIT::slow_op_put_to_scopeGenerator(VM& vm)
> +{
> +    JIT jit(vm);
> +
> +#if CPU(X86_64)
> +    jit.push(X86Registers::ebp);
> +#elif CPU(ARM64)
> +    jit.tagReturnAddress();
> +    jit.pushPair(framePointerRegister, linkRegister);
> +#endif
> +
> +    constexpr GPRReg bytecodeOffsetReg = argumentGPR2;
> +    jit.store32(bytecodeOffsetReg, tagFor(CallFrameSlot::argumentCountIncludingThis));
> +
> +    jit.loadPtr(addressFor(CallFrameSlot::codeBlock), argumentGPR3);
> +    jit.loadPtr(Address(argumentGPR3, CodeBlock::offsetOfGlobalObject()), argumentGPR0);
> +    jit.loadPtr(Address(argumentGPR3, CodeBlock::offsetOfInstructionsRawPointer()), argumentGPR1);
> +    jit.addPtr(bytecodeOffsetReg, argumentGPR1);
> +
> +    jit.prepareCallOperation(vm);
> +    CCallHelpers::Call operation = jit.call(OperationPtrTag);
> +    CCallHelpers::Jump exceptionCheck = jit.emitExceptionCheck(vm);
> +
> +#if CPU(X86_64)
> +    jit.pop(X86Registers::ebp);
> +#elif CPU(ARM64)
> +    jit.popPair(framePointerRegister, linkRegister);
> +#endif
> +    jit.ret();
> +
> +    LinkBuffer patchBuffer(jit, GLOBAL_THUNK_ID, LinkBuffer::Profile::Thunk);
> +    patchBuffer.link(operation, FunctionPtr<OperationPtrTag>(operationPutToScope));
> +    auto handler = vm.jitStubs->existingCTIStub(popThunkStackPreservesAndHandleExceptionGenerator, NoLockingNecessary);
> +    patchBuffer.link(exceptionCheck, CodeLocationLabel(handler.retaggedCode<NoPtrTag>()));
> +    return FINALIZE_CODE(patchBuffer, JITThunkPtrTag, "Baseline: slow_op_put_to_scope");
>  }
> +#endif

Ditto.

[Mark Lam]

Comment on attachment 428543 [details]
proposed patch.

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

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:128
>> +{
> 
> Let's add a comment that this works only in Baseline and ensure that this is only used in Baseline. It is getting JSGlobalObject from CodeBlock in CallFrame. This is valid only in Baseline and LLInt since this JSGlobalObject is different from the expected one in DFG / FTL if inlining happens.

Good point.  I should just rename all these thunks to baseline_... to make their purpose clear.

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:150
>> +
> 
> Now, it is already set up the arguments. So how about tail-call to the operation function from this thunk instead of returning to the caller?

The call target is patchable.  So, in order to tail call there, we'll need to pass the target in a register.  That will either increase the emitted code side, or complicate the patchable call instruction.

[Saam Barati]

Comment on attachment 428543 [details]
proposed patch.

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

LGTM, just a few structural questions

> Source/JavaScriptCore/jit/JIT.h:787
> +        static MacroAssemblerCodeRef<JITThunkPtrTag> slow_op_put_xxx_prepareCallGenerator(VM&);

the key here is that we're put_by_val. If you didn't know that, this name is a bit confusing. Maybe ...put_by_val_xxx...?

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:113
> +        // ARM64: argumentGPR0 amd argumentGPR1 already taken by incoming regT0 and regT1.
> +        // X86_64: argumentGPR1 already taken by incoming regT1.
> +        move(TrustedImmPtr(gen.stubInfo()), argumentGPR3);
> +        move(TrustedImmPtr(profile), argumentGPR2);
> +        emitNakedNearCall(vm.getCTIStub(slow_op_get_by_val_prepareCallGenerator).retaggedCode<NoPtrTag>());

can we define these as constants somewhere more canonical and give them a name?

Also, "amd" -> "and"

Also, it's confusing to refer to things as regTX in the comment and then use argument gprs.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:210
> +    // ARM64: argumentGPR0 amd argumentGPR1 already taken by incoming regT0 and regT1.
> +    // X86_64: argumentGPR1 already taken by incoming regT1.

ditto

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:234
> +    // arg3 now set to regT1 (argumentGPR1 on ARM64 & X86_64). argumentGPR1 is now free.

asserts are better than comments

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:239
> +    // - ARM64: regT0 is argumentGPR0: argumentGPR0 is now free.

asserts are better than comments

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:240
> +    // - X86_64: regT0 is not an argument GPR. So, don't care.

asserts are better than comments

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:241
> +    jit.move(regT0, argumentGPR2);

Do we really need all this manual arguments shuffling code? We already have code that handles that with setupArguments, right? Why can't we just use that everywhere?

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:300
> +    // ARM64: argumentGPR0 amd argumentGPR1 already taken by incoming regT0 and regT1.

asserts are better than comments

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:342
> +#else

same comments apply as before

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:836
> +    return FINALIZE_CODE(patchBuffer, JITThunkPtrTag, "Baseline: slow_op_del_by_id_prepareCall");

wrong name for "slow_op_del_by_id_prepareCall"

[Mark Lam]

Comment on attachment 428543 [details]
proposed patch.

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

>>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:128
>>> +{
>> 
>> Let's add a comment that this works only in Baseline and ensure that this is only used in Baseline. It is getting JSGlobalObject from CodeBlock in CallFrame. This is valid only in Baseline and LLInt since this JSGlobalObject is different from the expected one in DFG / FTL if inlining happens.
> 
> Good point.  I should just rename all these thunks to baseline_... to make their purpose clear.

Wait a minute ... these are all Baseline JIT methods (e.g. JIT::slow_op_get_by_val_prepareCallGenerator).  So, it should be implied that they only work with the Baseline JIT and not upper tier JITs.

[Mark Lam]

(In reply to Mark Lam from comment #5)
> Comment on attachment 428543 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428543&action=review
> 
> >>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:128
> >>> +{
> >> 
> >> Let's add a comment that this works only in Baseline and ensure that this is only used in Baseline. It is getting JSGlobalObject from CodeBlock in CallFrame. This is valid only in Baseline and LLInt since this JSGlobalObject is different from the expected one in DFG / FTL if inlining happens.
> > 
> > Good point.  I should just rename all these thunks to baseline_... to make their purpose clear.
> 
> Wait a minute ... these are all Baseline JIT methods (e.g.
> JIT::slow_op_get_by_val_prepareCallGenerator).  So, it should be implied
> that they only work with the Baseline JIT and not upper tier JITs.

Talked to Yusuke offline.  I'll add the comment as documentation in case these thunks get refactored out of the Baseline JIT later and the relationship becomes less obvious.

[Mark Lam]

Created attachment 428595 [details]
proposed patch.

Addressed Yusuke and Saam's feedback, and also should have fixed the Debug EWS failures.  Let's try this on the EWS first.

[Mark Lam]

Comment on attachment 428595 [details]
proposed patch.

Wrong patch.

[Mark Lam]

Created attachment 428598 [details]
proposed patch.

[Mark Lam]

Comment on attachment 428598 [details]
proposed patch.

Ready for a review.

[Mark Lam]

Thanks for the review.  Landed in r277500: <http://trac.webkit.org/r277500>.

[Radar WebKit Bug Importer]

<rdar://problem/78025955>