Bug 232746 – [JSC] Use CallLinkInfo in LLInt

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

And purge LLIntCallLinkInfo. Let's use CallIC in LLInt too.

[Yusuke Suzuki]

Created attachment 443379 [details]
Patch

[Yusuke Suzuki]

Purged CallLinkInfo::doneLocation() dependency to make it usable in LLInt and Baseline without changing doneLocation.

[Yusuke Suzuki]

Created attachment 443382 [details]
Patch

[Yusuke Suzuki]

Created attachment 443919 [details]
Patch

[Yusuke Suzuki]

Created attachment 443969 [details]
Patch

[Yusuke Suzuki]

Created attachment 443987 [details]
Patch

[Yusuke Suzuki]

Created attachment 443992 [details]
Patch

[Yusuke Suzuki]

Created attachment 444010 [details]
Patch

[Yusuke Suzuki]

Created attachment 444014 [details]
Patch

[Yusuke Suzuki]

Created attachment 444015 [details]
Patch

[Yusuke Suzuki]

Created attachment 444018 [details]
Patch

[Yusuke Suzuki]

Created attachment 444021 [details]
Patch

[Yusuke Suzuki]

Created attachment 444032 [details]
Patch

[Yusuke Suzuki]

Created attachment 444036 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/85330561>

[Yusuke Suzuki]

Created attachment 444090 [details]
Patch

[Yusuke Suzuki]

Created attachment 444093 [details]
Patch

[Yusuke Suzuki]

Created attachment 444101 [details]
Patch

[Yusuke Suzuki]

Created attachment 444126 [details]
Patch

[Yusuke Suzuki]

Created attachment 444130 [details]
Patch

[Saam Barati]

Comment on attachment 444126 [details]
Patch

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

> Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:126
> +#if ENABLE(JIT)

Should we assert we don't take this path if !ENABLE(JIT)?

> Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:140
> +#if ENABLE(JIT)

ditto

> Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:231
> +#if ENABLE(JIT)

this shouldn't be runtime useJIT()?

> Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:504
> +#if ENABLE(JIT)

should we assert we don't take this path if !ENABLE(JIT)

> Source/JavaScriptCore/bytecode/CallLinkInfo.h:395
> +#if ENABLE(JIT)

ditto about assert we're not here without ENABLE(JIT)

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:526
> +            metadata.m_callLinkInfo.initializeDataIC(vm, CallLinkInfo::callTypeFor(opcodeID), instruction.index(), nullptr);

could this just be a macro that takes CallFrameShuffleData* as param? Seems like a lot of similar code below. But it's also ok to keep it as is if it's easier.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1803
> +        FOR_EACH_OPCODE_WITH_LLINT_CALL_LINK_INFO(CASE)

this is a bad macro name now. Can we rename it for the new world where it's not just LLInt?

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2366
>          if (JITCode::isOptimizingJIT(jitType())) {
> -            DFG::CommonData* dfgCommon = m_jitCode->dfgCommon();
> -            for (auto* callLinkInfo : dfgCommon->m_callLinkInfos)
> -                callLinkInfo->setClearedByJettison();
> -        }
> +#if ENABLE(DFG_JIT)
> +            if (JITCode::isOptimizingJIT(jitType())) {

is this diff right? It looks like we check isOptimizingJIT twice in a row in branches?

[Saam Barati]

Comment on attachment 444130 [details]
Patch

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

Nice patch, I like this direction in architecture. r=me

> Source/JavaScriptCore/jit/Repatch.cpp:1855
> +    // 2. If it is tail-call, linkRegister is not pointing the doneLocation for slow-call case.

nit: Maybe write "3. If we're a data IC, then the return address is already correct"

> Source/JavaScriptCore/jit/Repatch.cpp:1861
> +        ASSERT(isDataIC);

nit: not sure we really need this assert given the above branch

Also, can we assert we're not DFG/FTL code given below returnFromBaseline stub.

> Source/JavaScriptCore/jit/Repatch.cpp:1865
> +        if (callLinkInfo.isTailCall()) {
> +            stubJit.move(CCallHelpers::TrustedImmPtr(vm.getCTIStub(JIT::returnFromBaselineGenerator).code().untaggedExecutableAddress()), GPRInfo::regT4);
> +            stubJit.restoreReturnAddressBeforeReturn(GPRInfo::regT4);
> +        }

Can we put a FIXME here maybe? Based on what we talked about on Slack. Since tail calls are relying on returning here, it seems like when we take the slow path we're not doing real tail calls. So maybe it's something we should fix.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:2557
> +        move NotCellMask, t1
> +        btqnz t0, t1, slowCase

this can't be one insn?

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2251
> +        # Those are r0 and r1

what is "Those" here?

[Yusuke Suzuki]

Comment on attachment 444126 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:126
>> +#if ENABLE(JIT)
> 
> Should we assert we don't take this path if !ENABLE(JIT)?

Nice, added.

>> Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:140
>> +#if ENABLE(JIT)
> 
> ditto

Done.

>> Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:231
>> +#if ENABLE(JIT)
> 
> this shouldn't be runtime useJIT()?

We cannot do that since PolymorphicCallStubRoutine does not have an implementation if it is !ENABLE(JIT).

>> Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:504
>> +#if ENABLE(JIT)
> 
> should we assert we don't take this path if !ENABLE(JIT)

Added.

>> Source/JavaScriptCore/bytecode/CallLinkInfo.h:395
>> +#if ENABLE(JIT)
> 
> ditto about assert we're not here without ENABLE(JIT)

Added

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:526
>> +            metadata.m_callLinkInfo.initializeDataIC(vm, CallLinkInfo::callTypeFor(opcodeID), instruction.index(), nullptr);
> 
> could this just be a macro that takes CallFrameShuffleData* as param? Seems like a lot of similar code below. But it's also ok to keep it as is if it's easier.

Done.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1803
>> +        FOR_EACH_OPCODE_WITH_LLINT_CALL_LINK_INFO(CASE)
> 
> this is a bad macro name now. Can we rename it for the new world where it's not just LLInt?

Right. Changed it to FOR_EACH_OPCODE_WITH_CALL_LINK_INFO.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2366
>> +            if (JITCode::isOptimizingJIT(jitType())) {
> 
> is this diff right? It looks like we check isOptimizingJIT twice in a row in branches?

Ah, second one is not necessary. Removed.

[Yusuke Suzuki]

Comment on attachment 444130 [details]
Patch

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

Thanks

>> Source/JavaScriptCore/jit/Repatch.cpp:1855
>> +    // 2. If it is tail-call, linkRegister is not pointing the doneLocation for slow-call case.
> 
> nit: Maybe write "3. If we're a data IC, then the return address is already correct"

Nice. Added.

>> Source/JavaScriptCore/jit/Repatch.cpp:1861
>> +        ASSERT(isDataIC);
> 
> nit: not sure we really need this assert given the above branch
> 
> Also, can we assert we're not DFG/FTL code given below returnFromBaseline stub.

Sounds good. Changed.

>> Source/JavaScriptCore/jit/Repatch.cpp:1865
>> +        }
> 
> Can we put a FIXME here maybe? Based on what we talked about on Slack. Since tail calls are relying on returning here, it seems like when we take the slow path we're not doing real tail calls. So maybe it's something we should fix.

Sounds good. Added.

>> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:2557
>> +        btqnz t0, t1, slowCase
> 
> this can't be one insn?

Nice. Changed.

>> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2251
>> +        # Those are r0 and r1
> 
> what is "Those" here?

restoredPCOrThrow, calleeFramePtr parameters. Changed it to "Those parameters are...".

[Yusuke Suzuki]

Created attachment 444173 [details]
Patch

[Yusuke Suzuki]

Created attachment 444181 [details]
Patch

[EWS]

Committed r285795 (244239@main): <https://commits.webkit.org/244239@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444181 [details].