Skip to content

Use SystemV ABI for Baseline JS JIT on Windows#29582

Merged
webkit-commit-queue merged 1 commit into
WebKit:mainfrom
iangrunert:ig/enable-baseline-jit-win
Jun 20, 2024
Merged

Use SystemV ABI for Baseline JS JIT on Windows#29582
webkit-commit-queue merged 1 commit into
WebKit:mainfrom
iangrunert:ig/enable-baseline-jit-win

Conversation

@iangrunert
Copy link
Copy Markdown
Contributor

@iangrunert iangrunert commented Jun 6, 2024

0b1e421

Use SystemV ABI for Baseline JS JIT on Windows
https://bugs.webkit.org/show_bug.cgi?id=275213

Reviewed by Yusuke Suzuki.

Using SystemV ABI for C++ entrypoints and JIT operations.

Removed ExceptionOperationResultBase from OperationResult. Due to more
conservative empty base class optimization on Windows, this was causing
register spills which (surprisingly) added an extra register parameter
to the compiled function, which broke when called from JIT generated
assembly.

USE_BUILTIN_FRAME_ADDRESS is still disabled, this requires work in clang
to enable __builtin_stack_address().

Disabled CSS Selector JIT, requires further work
Disable DFG JIT at runtime, requires further work
Disable YARR JIT at runtime, requires further work

* Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:
(JSC::AbstractMacroAssembler::TrustedImmPtr::TrustedImmPtr):
* Source/JavaScriptCore/assembler/MacroAssembler.cpp:
(JSC::stdFunctionCallback):
* Source/JavaScriptCore/assembler/MacroAssembler.h:
* Source/JavaScriptCore/assembler/MacroAssemblerPrinter.cpp:
(JSC::Printer::printCallback):
* Source/JavaScriptCore/assembler/MacroAssemblerPrinter.h:
* Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:
(JSC::MacroAssemblerX86_64::call):
(JSC::MacroAssemblerX86_64::callWithUGPRPair): Deleted.
* Source/JavaScriptCore/assembler/ProbeContext.h:
* Source/JavaScriptCore/bytecode/CodeBlock.h:
* Source/JavaScriptCore/bytecode/InlineCacheCompiler.cpp:
(JSC::InlineCacheCompiler::emitDataICPrologue):
* Source/JavaScriptCore/dfg/DFGArithMode.h:
* Source/JavaScriptCore/dfg/DFGJITCompiler.h:
(JSC::DFG::JITCompiler::appendCallWithUGPRPair): Deleted.
* Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:
(JSC::DFG::SpeculativeJIT::appendCallSetResult):
(JSC::DFG::SpeculativeJIT::appendCallWithUGPRPair): Deleted.
* Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::emitCall):
* Source/JavaScriptCore/interpreter/CallFrame.h:
* Source/JavaScriptCore/jit/CCallHelpers.h:
(JSC::CCallHelpers::ArgCollection::argCount):
(JSC::CCallHelpers::calculatePokeOffset):
(JSC::CCallHelpers::marshallArgumentRegister):
(JSC::CCallHelpers::setupArgumentsImpl):
* Source/JavaScriptCore/jit/JIT.h:
* Source/JavaScriptCore/jit/JITCall.cpp:
(JSC::JIT::emit_op_iterator_next):
* Source/JavaScriptCore/jit/JITOpcodes.cpp:
(JSC::JIT::op_enter_handlerGenerator):
* Source/JavaScriptCore/jit/JITOperations.cpp:
* Source/JavaScriptCore/jit/JITOperations.h:
* Source/JavaScriptCore/jit/JITPropertyAccess.cpp:
* Source/JavaScriptCore/jit/OperationResult.h:
* Source/JavaScriptCore/jit/RegisterSet.cpp:
(JSC::RegisterSetBuilder::wasmPinnedRegisters):
* Source/JavaScriptCore/jit/SlowPathCall.cpp:
(JSC::JITSlowPathCall::generateThunk):
* Source/JavaScriptCore/jit/ThunkGenerators.cpp:
(JSC::nativeForGenerator):
(JSC::arityFixupGenerator):
* Source/JavaScriptCore/llint/LLIntData.cpp:
* Source/JavaScriptCore/llint/LLIntData.h:
* Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:
(JSC::LLInt::logWasmPrologue):
(JSC::LLInt::llint_write_barrier_slow):
* Source/JavaScriptCore/llint/LLIntThunks.h:
* Source/JavaScriptCore/runtime/Options.cpp:
(JSC::Options::notifyOptionsChanged):
* Source/JavaScriptCore/runtime/SlowPathFunction.h:
* Source/JavaScriptCore/tools/JSDollarVM.cpp:
* Source/WTF/wtf/FunctionTraits.h:
(WTF::SYSV_ABI):
* Source/WTF/wtf/PlatformEnable.h:
* Source/WTF/wtf/PlatformUse.h:

Canonical link: https://commits.webkit.org/280216@main

ce82f3c

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 wincairo-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ⏳ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
⏳ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
⏳ 🧪 vision-wk2 🛠 jsc-armv7
✅ 🛠 🧪 unsafe-merge ✅ 🛠 tv 🧪 jsc-armv7-tests
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

Starting EWS tests for 94bdf19. Live statuses available at the PR page, #29582

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 6, 2024
@iangrunert iangrunert force-pushed the ig/enable-baseline-jit-win branch from 94bdf19 to dc9443d Compare June 6, 2024 18:06
@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

Starting EWS tests for dc9443d. Live statuses available at the PR page, #29582

@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

Starting EWS tests for dc9443d. Live statuses available at the PR page, #29582

@iangrunert iangrunert force-pushed the ig/enable-baseline-jit-win branch from dc9443d to 71a3ac0 Compare June 6, 2024 23:41
@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

Starting EWS tests for 71a3ac0. Live statuses available at the PR page, #29582

@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

Starting EWS tests for 71a3ac0. Live statuses available at the PR page, #29582

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can prolly just get rid of this define either now or in a later patch.

Comment thread Source/WTF/wtf/PlatformEnable.h Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel like the OS and PLATFORM checks can go and just rely on whether or not JIT is enabled. @Constellation thoughts?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. We must enable it only for ARM64 and x64 and that's the most critical condition.

@Jarred-Sumner

This comment was marked as outdated.

@iangrunert
Copy link
Copy Markdown
Contributor Author

Enabled more baseline JIT things for parity with the other platforms, it now fails even earlier. Test program:

throwAfter = (n) => { if (n === 0) { throw new Error(); } return throwAfter(n-1); }; throwAfter(1000);

Fails in call to operationDefaultCall JITOperations.cpp#2390 calleeFrame is set to the same value as callLinkInfo. This is before baseline JIT kicks in, it's just calling via the generated thunk.

If I switch defaultCallThunk to the llint_default_call_trampoline in LLIntEntrypoint.cpp, it gets further and the baseline jitted call to operationThrow fails, it gets a null call frame (and also looks like the globalObject->vm pointer is sometimes trashed).

Here's the generated assembly for operationDefaultCall. It's a bit odd to me that it's using RSI and RDX for calleeFrame / callLinkInfo, what happened to RDI?

JSC_DEFINE_JIT_OPERATION(operationDefaultCall, UCPURegister, (CallFrame* calleeFrame, CallLinkInfo* callLinkInfo))
{
 sub         rsp,128h  
 mov         qword ptr [rsp+30h],rdi  
 mov         rax,rdi  
 mov         qword ptr [rsp+38h],rax  
 mov         qword ptr [rsp+120h],rdi  
 mov         qword ptr [rsp+118h],rdx  
 mov         qword ptr [rsp+110h],rsi  
    JSCell* owner = callLinkInfo->ownerForSlowPath(calleeFrame);
 mov         rcx,qword ptr [rsp+118h] ; callLinkInfo, passed in RDX
 mov         rdx,qword ptr [rsp+110h] ; calleeFrame, passed in RSI

Compare that with LLIntSlowPaths.cpp llint_default_call (which is used by llint_default_call_trampoline):

extern "C" UGPRPair SYSV_ABI llint_default_call(CallFrame* calleeFrame, CallLinkInfo* callLinkInfo)
{
 sub         rsp,0D8h  
 mov         qword ptr [rsp+0C0h],rsi  
 mov         qword ptr [rsp+0B8h],rdi  
    JSCell* owner = callLinkInfo->ownerForSlowPath(calleeFrame);
 mov         rcx,qword ptr [rsp+0C0h] ; callLinkInfo, passed in RSI
 mov         rdx,qword ptr [rsp+0B8h] ; calleeFrame, passed in RDI

Much simpler, we're using RSI and RDI as expected for a two-parameter function.

I'll have to dig into why we're getting such odd assembly output for JSC_DEFINE_JIT_OPERATION.

@iangrunert
Copy link
Copy Markdown
Contributor Author

The above is still a problem after upgrading to clang 18.1.7 (previously using clang 16.0.5).

@iangrunert
Copy link
Copy Markdown
Contributor Author

I've got a minimal-ish repro on compiler explorer: https://godbolt.org/z/aT6xMhTfa

If compiled without optimizations on Windows, operationDefaultCall ends up passing the parameters in rsi and rdx. I'm not sure if this counts as a clang bug?

@iangrunert iangrunert force-pushed the ig/enable-baseline-jit-win branch from b78e1fb to 580b6a2 Compare June 17, 2024 17:28
@iangrunert iangrunert force-pushed the ig/enable-baseline-jit-win branch from 580b6a2 to cc0fe94 Compare June 17, 2024 17:29
@iangrunert iangrunert force-pushed the ig/enable-baseline-jit-win branch from cc0fe94 to ce82f3c Compare June 17, 2024 18:32
@iangrunert iangrunert changed the title Enable Baseline JS JIT on Windows Use SystemV ABI for Baseline JS JIT on Windows Jun 17, 2024
@iangrunert iangrunert self-assigned this Jun 17, 2024
@iangrunert iangrunert marked this pull request as ready for review June 17, 2024 18:49
@iangrunert iangrunert requested a review from a team as a code owner June 17, 2024 18:49
Copy link
Copy Markdown
Member

@Constellation Constellation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me

Comment thread Source/WTF/wtf/PlatformEnable.h Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. We must enable it only for ARM64 and x64 and that's the most critical condition.

@iangrunert iangrunert removed the merging-blocked Applied to prevent a change from being merged label Jun 19, 2024
@Constellation
Copy link
Copy Markdown
Member

Keep in mind that Apple JSC team does not maintain Windows JIT. So this is community effort to keep it working.

@fujii fujii added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 20, 2024
https://bugs.webkit.org/show_bug.cgi?id=275213

Reviewed by Yusuke Suzuki.

Using SystemV ABI for C++ entrypoints and JIT operations.

Removed ExceptionOperationResultBase from OperationResult. Due to more
conservative empty base class optimization on Windows, this was causing
register spills which (surprisingly) added an extra register parameter
to the compiled function, which broke when called from JIT generated
assembly.

USE_BUILTIN_FRAME_ADDRESS is still disabled, this requires work in clang
to enable __builtin_stack_address().

Disabled CSS Selector JIT, requires further work
Disable DFG JIT at runtime, requires further work
Disable YARR JIT at runtime, requires further work

* Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:
(JSC::AbstractMacroAssembler::TrustedImmPtr::TrustedImmPtr):
* Source/JavaScriptCore/assembler/MacroAssembler.cpp:
(JSC::stdFunctionCallback):
* Source/JavaScriptCore/assembler/MacroAssembler.h:
* Source/JavaScriptCore/assembler/MacroAssemblerPrinter.cpp:
(JSC::Printer::printCallback):
* Source/JavaScriptCore/assembler/MacroAssemblerPrinter.h:
* Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:
(JSC::MacroAssemblerX86_64::call):
(JSC::MacroAssemblerX86_64::callWithUGPRPair): Deleted.
* Source/JavaScriptCore/assembler/ProbeContext.h:
* Source/JavaScriptCore/bytecode/CodeBlock.h:
* Source/JavaScriptCore/bytecode/InlineCacheCompiler.cpp:
(JSC::InlineCacheCompiler::emitDataICPrologue):
* Source/JavaScriptCore/dfg/DFGArithMode.h:
* Source/JavaScriptCore/dfg/DFGJITCompiler.h:
(JSC::DFG::JITCompiler::appendCallWithUGPRPair): Deleted.
* Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:
(JSC::DFG::SpeculativeJIT::appendCallSetResult):
(JSC::DFG::SpeculativeJIT::appendCallWithUGPRPair): Deleted.
* Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::emitCall):
* Source/JavaScriptCore/interpreter/CallFrame.h:
* Source/JavaScriptCore/jit/CCallHelpers.h:
(JSC::CCallHelpers::ArgCollection::argCount):
(JSC::CCallHelpers::calculatePokeOffset):
(JSC::CCallHelpers::marshallArgumentRegister):
(JSC::CCallHelpers::setupArgumentsImpl):
* Source/JavaScriptCore/jit/JIT.h:
* Source/JavaScriptCore/jit/JITCall.cpp:
(JSC::JIT::emit_op_iterator_next):
* Source/JavaScriptCore/jit/JITOpcodes.cpp:
(JSC::JIT::op_enter_handlerGenerator):
* Source/JavaScriptCore/jit/JITOperations.cpp:
* Source/JavaScriptCore/jit/JITOperations.h:
* Source/JavaScriptCore/jit/JITPropertyAccess.cpp:
* Source/JavaScriptCore/jit/OperationResult.h:
* Source/JavaScriptCore/jit/RegisterSet.cpp:
(JSC::RegisterSetBuilder::wasmPinnedRegisters):
* Source/JavaScriptCore/jit/SlowPathCall.cpp:
(JSC::JITSlowPathCall::generateThunk):
* Source/JavaScriptCore/jit/ThunkGenerators.cpp:
(JSC::nativeForGenerator):
(JSC::arityFixupGenerator):
* Source/JavaScriptCore/llint/LLIntData.cpp:
* Source/JavaScriptCore/llint/LLIntData.h:
* Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:
(JSC::LLInt::logWasmPrologue):
(JSC::LLInt::llint_write_barrier_slow):
* Source/JavaScriptCore/llint/LLIntThunks.h:
* Source/JavaScriptCore/runtime/Options.cpp:
(JSC::Options::notifyOptionsChanged):
* Source/JavaScriptCore/runtime/SlowPathFunction.h:
* Source/JavaScriptCore/tools/JSDollarVM.cpp:
* Source/WTF/wtf/FunctionTraits.h:
(WTF::SYSV_ABI):
* Source/WTF/wtf/PlatformEnable.h:
* Source/WTF/wtf/PlatformUse.h:

Canonical link: https://commits.webkit.org/280216@main
@webkit-commit-queue webkit-commit-queue force-pushed the ig/enable-baseline-jit-win branch from ce82f3c to 0b1e421 Compare June 20, 2024 20:41
@webkit-commit-queue
Copy link
Copy Markdown
Collaborator

Committed 280216@main (0b1e421): https://commits.webkit.org/280216@main

Reviewed commits have been landed. Closing PR #29582 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 0b1e421 into WebKit:main Jun 20, 2024
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants