Bug 225771

Summary: Implement Baseline JIT property access slow paths using JIT thunks.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch.
ews-feeder: commit-queue-
proposed patch.
none
proposed patch. ysuzuki: review+

Description Mark Lam 2021-05-13 10:46:17 PDT 
Patch coming.
Comment 1 Mark Lam 2021-05-13 12:07:48 PDT 
Created attachment 428543 [details]
proposed patch.
Comment 2 Yusuke Suzuki 2021-05-13 12:58:31 PDT 
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.
Comment 3 Mark Lam 2021-05-13 13:06:12 PDT 
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.
Comment 4 Saam Barati 2021-05-13 13:09:19 PDT 
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"
Comment 5 Mark Lam 2021-05-13 13:18:40 PDT 
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.
Comment 6 Mark Lam 2021-05-13 16:17:52 PDT 
(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.
Comment 7 Mark Lam 2021-05-13 20:48:51 PDT 
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.
Comment 8 Mark Lam 2021-05-13 20:49:52 PDT 
Comment on attachment 428595 [details]
proposed patch.

Wrong patch.
Comment 9 Mark Lam 2021-05-13 21:02:10 PDT 
Created attachment 428598 [details]
proposed patch.
Comment 10 Mark Lam 2021-05-14 08:33:46 PDT 
Comment on attachment 428598 [details]
proposed patch.

Ready for a review.
Comment 11 Mark Lam 2021-05-14 11:36:24 PDT 
Thanks for the review.  Landed in r277500: <http://trac.webkit.org/r277500>.
Comment 12 Radar WebKit Bug Importer 2021-05-14 11:37:16 PDT 
<rdar://problem/78025955>