Summary: | [JSC] Use CallLinkInfo in LLInt | ||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | annulen, ews-watchlist, gyuyoung.kim, keith_miller, mark.lam, msaboff, ryuan.choi, saam, sergio, tzagallo, webkit-bug-importer | ||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Yusuke Suzuki
2021-11-04 23:13:25 PDT
Created attachment 443379 [details]
Patch
Purged CallLinkInfo::doneLocation() dependency to make it usable in LLInt and Baseline without changing doneLocation. Created attachment 443382 [details]
Patch
Created attachment 443919 [details]
Patch
Created attachment 443969 [details]
Patch
Created attachment 443987 [details]
Patch
Created attachment 443992 [details]
Patch
Created attachment 444010 [details]
Patch
Created attachment 444014 [details]
Patch
Created attachment 444015 [details]
Patch
Created attachment 444018 [details]
Patch
Created attachment 444021 [details]
Patch
Created attachment 444032 [details]
Patch
Created attachment 444036 [details]
Patch
Created attachment 444090 [details]
Patch
Created attachment 444093 [details]
Patch
Created attachment 444101 [details]
Patch
Created attachment 444126 [details]
Patch
Created attachment 444130 [details]
Patch
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 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 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 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...". Created attachment 444173 [details]
Patch
Created attachment 444181 [details]
Patch
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]. |