Bug 232746 - [JSC] Use CallLinkInfo in LLInt
Summary: [JSC] Use CallLinkInfo in LLInt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-04 23:13 PDT by Yusuke Suzuki
Modified: 2021-11-14 18:44 PST (History)
11 users (show)

See Also:


Attachments
Patch (30.76 KB, patch)
2021-11-04 23:14 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (31.28 KB, patch)
2021-11-05 01:21 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (127.21 KB, patch)
2021-11-11 01:05 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (145.81 KB, patch)
2021-11-11 10:27 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (152.65 KB, patch)
2021-11-11 12:08 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (156.02 KB, patch)
2021-11-11 12:58 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (383.96 KB, patch)
2021-11-11 15:31 PST, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (389.12 KB, patch)
2021-11-11 15:44 PST, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (389.33 KB, patch)
2021-11-11 15:45 PST, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (389.31 KB, patch)
2021-11-11 16:18 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (389.28 KB, patch)
2021-11-11 16:31 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (397.09 KB, patch)
2021-11-11 18:09 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (399.57 KB, patch)
2021-11-11 19:20 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (400.80 KB, patch)
2021-11-12 13:06 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (402.26 KB, patch)
2021-11-12 13:10 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (402.41 KB, patch)
2021-11-12 13:22 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (402.47 KB, patch)
2021-11-12 17:13 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (224.56 KB, patch)
2021-11-12 18:19 PST, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff
Patch (222.52 KB, patch)
2021-11-14 02:31 PST, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (222.52 KB, patch)
2021-11-14 11:27 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2021-11-04 23:13:25 PDT
And purge LLIntCallLinkInfo. Let's use CallIC in LLInt too.
Comment 1 Yusuke Suzuki 2021-11-04 23:14:55 PDT
Created attachment 443379 [details]
Patch
Comment 2 Yusuke Suzuki 2021-11-04 23:16:28 PDT
Purged CallLinkInfo::doneLocation() dependency to make it usable in LLInt and Baseline without changing doneLocation.
Comment 3 Yusuke Suzuki 2021-11-05 01:21:17 PDT
Created attachment 443382 [details]
Patch
Comment 4 Yusuke Suzuki 2021-11-11 01:05:26 PST
Created attachment 443919 [details]
Patch
Comment 5 Yusuke Suzuki 2021-11-11 10:27:30 PST
Created attachment 443969 [details]
Patch
Comment 6 Yusuke Suzuki 2021-11-11 12:08:28 PST
Created attachment 443987 [details]
Patch
Comment 7 Yusuke Suzuki 2021-11-11 12:58:42 PST
Created attachment 443992 [details]
Patch
Comment 8 Yusuke Suzuki 2021-11-11 15:31:09 PST
Created attachment 444010 [details]
Patch
Comment 9 Yusuke Suzuki 2021-11-11 15:44:24 PST
Created attachment 444014 [details]
Patch
Comment 10 Yusuke Suzuki 2021-11-11 15:45:56 PST
Created attachment 444015 [details]
Patch
Comment 11 Yusuke Suzuki 2021-11-11 16:18:57 PST
Created attachment 444018 [details]
Patch
Comment 12 Yusuke Suzuki 2021-11-11 16:31:59 PST
Created attachment 444021 [details]
Patch
Comment 13 Yusuke Suzuki 2021-11-11 18:09:51 PST
Created attachment 444032 [details]
Patch
Comment 14 Yusuke Suzuki 2021-11-11 19:20:25 PST
Created attachment 444036 [details]
Patch
Comment 15 Radar WebKit Bug Importer 2021-11-11 22:14:23 PST
<rdar://problem/85330561>
Comment 16 Yusuke Suzuki 2021-11-12 13:06:44 PST
Created attachment 444090 [details]
Patch
Comment 17 Yusuke Suzuki 2021-11-12 13:10:12 PST
Created attachment 444093 [details]
Patch
Comment 18 Yusuke Suzuki 2021-11-12 13:22:29 PST
Created attachment 444101 [details]
Patch
Comment 19 Yusuke Suzuki 2021-11-12 17:13:44 PST
Created attachment 444126 [details]
Patch
Comment 20 Yusuke Suzuki 2021-11-12 18:19:43 PST
Created attachment 444130 [details]
Patch
Comment 21 Saam Barati 2021-11-12 18:20:24 PST
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?
Comment 22 Saam Barati 2021-11-12 18:52:53 PST
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?
Comment 23 Yusuke Suzuki 2021-11-14 02:16:39 PST
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.
Comment 24 Yusuke Suzuki 2021-11-14 02:28:51 PST
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...".
Comment 25 Yusuke Suzuki 2021-11-14 02:31:45 PST
Created attachment 444173 [details]
Patch
Comment 26 Yusuke Suzuki 2021-11-14 11:27:35 PST
Created attachment 444181 [details]
Patch
Comment 27 EWS 2021-11-14 18:44:24 PST
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].