Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JSC] Save more code size by aligning wasm operations to JS's convention #8523

Conversation

Constellation
Copy link
Member

@Constellation Constellation commented Jan 11, 2023

773f16e

[JSC] Save more code size by aligning wasm operations to JS's convention
https://bugs.webkit.org/show_bug.cgi?id=250455
rdar://problem/104121534

Reviewed by Saam Barati.

This patch fixes bunch of problems in wasm operations so that we can save code size, and we can make implementation clean by removing many FIXMEs.

1. We get CallFrame* via __builtin_frame_address(1) as the same way to JS's operation. This reduces code size in JIT code.
   Each wasm operation call now calls prepareWasmCallOperation, which set up frame pointer in Instance* if __builtin_frame_address(1) is not available.
   If __builtin_frame_address(1) is available, this is nop. Wasm operation retrieves CallFrame* from __builtin_frame_address(1) or passed Instance*.
2. We must not call Wasm operations from C++ world. This is because (1) strongly assumes that Wasm operations are called from JIT code. This patch
   extracts code to WasmOperationsInlines.h so that we no longer call Wasm operations from C++ world. This was met in JS operations before. Now wasm
   operations meet this too.
3. We appropriately pass Instance*. And we remove `CallFrame::deprecatedVM()` use in wasm operations completely. This is well aligned to how JS operations
   work: JS operations take JSGlobalObject* as its realm. Now wasm operations take Instance*.

* Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:
* Source/JavaScriptCore/interpreter/CallFrame.h:
* Source/JavaScriptCore/jit/AssemblyHelpers.cpp:
(JSC::AssemblyHelpers::prepareWasmCallOperation):
* Source/JavaScriptCore/jit/AssemblyHelpers.h:
* Source/JavaScriptCore/wasm/WasmAirIRGenerator32_64.cpp:
(JSC::Wasm::AirIRGenerator32::emitCatchImpl):
* Source/JavaScriptCore/wasm/WasmAirIRGenerator64.cpp:
(JSC::Wasm::AirIRGenerator64::emitCatchImpl):
* Source/JavaScriptCore/wasm/WasmAirIRGeneratorBase.h:
(JSC::Wasm::AirIRGeneratorBase::emitCCall):
(JSC::Wasm::ExpressionType>::addGrowMemory):
* Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:
(JSC::Wasm::B3IRGenerator::emitPrepareWasmOperation):
(JSC::Wasm::B3IRGenerator::addTableGet):
(JSC::Wasm::B3IRGenerator::addTableSet):
(JSC::Wasm::B3IRGenerator::addRefFunc):
(JSC::Wasm::B3IRGenerator::addTableInit):
(JSC::Wasm::B3IRGenerator::addElemDrop):
(JSC::Wasm::B3IRGenerator::addTableSize):
(JSC::Wasm::B3IRGenerator::addTableGrow):
(JSC::Wasm::B3IRGenerator::addTableFill):
(JSC::Wasm::B3IRGenerator::addTableCopy):
(JSC::Wasm::B3IRGenerator::addGrowMemory):
(JSC::Wasm::B3IRGenerator::addMemoryFill):
(JSC::Wasm::B3IRGenerator::addMemoryInit):
(JSC::Wasm::B3IRGenerator::addMemoryCopy):
(JSC::Wasm::B3IRGenerator::addDataDrop):
(JSC::Wasm::B3IRGenerator::setGlobal):
(JSC::Wasm::B3IRGenerator::emitWriteBarrier):
(JSC::Wasm::B3IRGenerator::atomicWait):
(JSC::Wasm::B3IRGenerator::atomicNotify):
(JSC::Wasm::B3IRGenerator::pushArrayNew):
(JSC::Wasm::B3IRGenerator::addArrayGet):
(JSC::Wasm::B3IRGenerator::addArraySet):
(JSC::Wasm::B3IRGenerator::addStructNew):
(JSC::Wasm::B3IRGenerator::addStructNewDefault):
(JSC::Wasm::B3IRGenerator::emitCatchImpl):
(JSC::Wasm::B3IRGenerator::addI32Popcnt):
(JSC::Wasm::B3IRGenerator::addI64Popcnt):
* Source/JavaScriptCore/wasm/WasmBinding.cpp:
(JSC::Wasm::wasmToWasm):
* Source/JavaScriptCore/wasm/WasmInstance.h:
(JSC::Wasm::Instance::offsetOfTemporaryCallFrame):
(JSC::Wasm::Instance::temporaryCallFrame const):
(JSC::Wasm::Instance::setTemporaryCallFrame):
* Source/JavaScriptCore/wasm/WasmOperations.cpp:
(JSC::Wasm::JSC_DEFINE_JIT_OPERATION):
(JSC::Wasm::WasmOperationsInternal::retrieveAndClearExceptionIfCatchableImpl):
(JSC::Wasm::setWasmTableElement): Deleted.
(JSC::Wasm::wait): Deleted.
* Source/JavaScriptCore/wasm/WasmOperations.h:
* Source/JavaScriptCore/wasm/WasmOperationsInlines.h: Added.
(JSC::Wasm::refFunc):
(JSC::Wasm::arrayNew):
(JSC::Wasm::arrayGet):
(JSC::Wasm::arraySet):
(JSC::Wasm::structNew):
(JSC::Wasm::structGet):
(JSC::Wasm::structSet):
(JSC::Wasm::tableGet):
(JSC::Wasm::tableSet):
(JSC::Wasm::tableInit):
(JSC::Wasm::tableFill):
(JSC::Wasm::tableGrow):
(JSC::Wasm::tableCopy):
(JSC::Wasm::tableSize):
(JSC::Wasm::growMemory):
(JSC::Wasm::memoryInit):
(JSC::Wasm::memoryFill):
(JSC::Wasm::memoryCopy):
(JSC::Wasm::dataDrop):
(JSC::Wasm::elemDrop):
(JSC::Wasm::waitImpl):
(JSC::Wasm::memoryAtomicWait32):
(JSC::Wasm::memoryAtomicWait64):
(JSC::Wasm::memoryAtomicNotify):
(JSC::Wasm::throwWasmToJSException):
* Source/JavaScriptCore/wasm/WasmSlowPaths.cpp:
(JSC::LLInt::WASM_SLOW_PATH_DECL):
(JSC::LLInt::slow_path_wasm_throw_exception):
* Source/JavaScriptCore/wasm/WasmThunks.cpp:
(JSC::Wasm::throwExceptionFromWasmThunkGenerator):
* Source/JavaScriptCore/wasm/js/JSToWasm.cpp:
(JSC::Wasm::marshallJSResult):
* Source/JavaScriptCore/wasm/js/WasmToJS.cpp:
(JSC::Wasm::materializeImportJSCell):
(JSC::Wasm::handleBadImportTypeUse):
(JSC::Wasm::wasmToJS):
(JSC::Wasm::emitThrowWasmToJSException):

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

abc0f0f

Misc iOS, tvOS & watchOS macOS Linux Windows
❌ πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe   πŸ›  πŸ§ͺ win
βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ›  gtk   πŸ›  wincairo
βœ… πŸ§ͺ webkitperl   πŸ§ͺ ios-wk2   πŸ§ͺ api-mac   πŸ§ͺ gtk-wk2
  πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk1   πŸ§ͺ api-gtk
  πŸ›  πŸ§ͺ jsc βœ… πŸ›  tv   πŸ§ͺ mac-wk2 βœ… πŸ›  jsc-armv7
βœ… πŸ›  πŸ§ͺ jsc-arm64 βœ… πŸ›  tv-sim   πŸ§ͺ mac-AS-debug-wk2 ❌ πŸ§ͺ jsc-armv7-tests
βœ… πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress   πŸ›  jsc-mips
βœ… πŸ›  watch-sim   πŸ§ͺ jsc-mips-tests
βœ… πŸ›  πŸ§ͺ unsafe-merge

@Constellation Constellation requested a review from a team as a code owner January 11, 2023 15:06
@Constellation Constellation self-assigned this Jan 11, 2023
@Constellation Constellation added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Jan 11, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 11, 2023
@Constellation Constellation removed the merging-blocked Applied to prevent a change from being merged label Jan 11, 2023
@Constellation Constellation force-pushed the eng/JSC-Save-more-code-size-by-aligning-wasm-operations-to-JSs-convention branch from 15f802c to 4004642 Compare January 11, 2023 15:22
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 11, 2023
@Constellation Constellation removed the merging-blocked Applied to prevent a change from being merged label Jan 11, 2023
patchpoint->effects = B3::Effects::forCall();
patchpoint->setGenerator([=] (CCallHelpers& jit, const B3::StackmapGenerationParams& params) {
GPRReg instanceGPR = params[0].gpr();
jit.storePtr(GPRInfo::callFrameRegister, CCallHelpers::Address(instanceGPR, Instance::offsetOfTemporaryCallFrame()));
Copy link
Contributor

Choose a reason for hiding this comment

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

should we call prepareWasmCallOperation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Now, we encode it as normal B3 / Air IR.

patchpoint->append(instanceValue(), ValueRep::SomeRegister);
patchpoint->setGenerator([=] (CCallHelpers& jit, const StackmapGenerationParams& params) {
GPRReg instanceGPR = params[0].gpr();
jit.storePtr(GPRInfo::callFrameRegister, CCallHelpers::Address(instanceGPR, Instance::offsetOfTemporaryCallFrame()));
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, I think this entire patchpoint can just be normal B3 IR. Just a Store(FramePointer)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

#if !USE(BUILTIN_FRAME_ADDRESS) || ASSERT_ENABLED
// Prepare wasm operation calls.
PatchpointValue* patchpoint = block->appendNew<PatchpointValue>(m_proc, B3::Void, origin());
patchpoint->effects = B3::Effects::forCall();
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe just writes top?

@@ -1255,6 +1276,7 @@ auto B3IRGenerator::addElemDrop(unsigned elementIndex) -> PartialResult
auto B3IRGenerator::addTableSize(unsigned tableIndex, ExpressionType& result) -> PartialResult
{
// FIXME: Emit this inline <https://bugs.webkit.org/show_bug.cgi?id=198506>.
emitPrepareWasmOperation(m_currentBlock);
Copy link
Contributor

Choose a reason for hiding this comment

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

given these are sprinkled everywhere, I wonder if it's worth having a helper function like "callWasmOperation" that takes all the arguments for the CCallValue, and internally it calls emitPrepareWasmOperation. That way we don't need to remember to do this everywhere that we are doing a wasm operation call.

Copy link
Contributor

Choose a reason for hiding this comment

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

something like:

template <typename ...Args>
Value* callWasmOperation(Args&&... args) { ... }

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed.

{
auto* patchpoint = addPatchpoint(B3::Void);
patchpoint->effects = B3::Effects::forCall();
patchpoint->setGenerator([=] (CCallHelpers& jit, const B3::StackmapGenerationParams& params) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this patchpoint can just be done in normal Air IR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed

@Constellation Constellation force-pushed the eng/JSC-Save-more-code-size-by-aligning-wasm-operations-to-JSs-convention branch from 4004642 to abc0f0f Compare January 12, 2023 04:33
@Constellation Constellation added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jan 12, 2023
https://bugs.webkit.org/show_bug.cgi?id=250455
rdar://problem/104121534

Reviewed by Saam Barati.

This patch fixes bunch of problems in wasm operations so that we can save code size, and we can make implementation clean by removing many FIXMEs.

1. We get CallFrame* via __builtin_frame_address(1) as the same way to JS's operation. This reduces code size in JIT code.
   Each wasm operation call now calls prepareWasmCallOperation, which set up frame pointer in Instance* if __builtin_frame_address(1) is not available.
   If __builtin_frame_address(1) is available, this is nop. Wasm operation retrieves CallFrame* from __builtin_frame_address(1) or passed Instance*.
2. We must not call Wasm operations from C++ world. This is because (1) strongly assumes that Wasm operations are called from JIT code. This patch
   extracts code to WasmOperationsInlines.h so that we no longer call Wasm operations from C++ world. This was met in JS operations before. Now wasm
   operations meet this too.
3. We appropriately pass Instance*. And we remove `CallFrame::deprecatedVM()` use in wasm operations completely. This is well aligned to how JS operations
   work: JS operations take JSGlobalObject* as its realm. Now wasm operations take Instance*.

* Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:
* Source/JavaScriptCore/interpreter/CallFrame.h:
* Source/JavaScriptCore/jit/AssemblyHelpers.cpp:
(JSC::AssemblyHelpers::prepareWasmCallOperation):
* Source/JavaScriptCore/jit/AssemblyHelpers.h:
* Source/JavaScriptCore/wasm/WasmAirIRGenerator32_64.cpp:
(JSC::Wasm::AirIRGenerator32::emitCatchImpl):
* Source/JavaScriptCore/wasm/WasmAirIRGenerator64.cpp:
(JSC::Wasm::AirIRGenerator64::emitCatchImpl):
* Source/JavaScriptCore/wasm/WasmAirIRGeneratorBase.h:
(JSC::Wasm::AirIRGeneratorBase::emitCCall):
(JSC::Wasm::ExpressionType>::addGrowMemory):
* Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:
(JSC::Wasm::B3IRGenerator::emitPrepareWasmOperation):
(JSC::Wasm::B3IRGenerator::addTableGet):
(JSC::Wasm::B3IRGenerator::addTableSet):
(JSC::Wasm::B3IRGenerator::addRefFunc):
(JSC::Wasm::B3IRGenerator::addTableInit):
(JSC::Wasm::B3IRGenerator::addElemDrop):
(JSC::Wasm::B3IRGenerator::addTableSize):
(JSC::Wasm::B3IRGenerator::addTableGrow):
(JSC::Wasm::B3IRGenerator::addTableFill):
(JSC::Wasm::B3IRGenerator::addTableCopy):
(JSC::Wasm::B3IRGenerator::addGrowMemory):
(JSC::Wasm::B3IRGenerator::addMemoryFill):
(JSC::Wasm::B3IRGenerator::addMemoryInit):
(JSC::Wasm::B3IRGenerator::addMemoryCopy):
(JSC::Wasm::B3IRGenerator::addDataDrop):
(JSC::Wasm::B3IRGenerator::setGlobal):
(JSC::Wasm::B3IRGenerator::emitWriteBarrier):
(JSC::Wasm::B3IRGenerator::atomicWait):
(JSC::Wasm::B3IRGenerator::atomicNotify):
(JSC::Wasm::B3IRGenerator::pushArrayNew):
(JSC::Wasm::B3IRGenerator::addArrayGet):
(JSC::Wasm::B3IRGenerator::addArraySet):
(JSC::Wasm::B3IRGenerator::addStructNew):
(JSC::Wasm::B3IRGenerator::addStructNewDefault):
(JSC::Wasm::B3IRGenerator::emitCatchImpl):
(JSC::Wasm::B3IRGenerator::addI32Popcnt):
(JSC::Wasm::B3IRGenerator::addI64Popcnt):
* Source/JavaScriptCore/wasm/WasmBinding.cpp:
(JSC::Wasm::wasmToWasm):
* Source/JavaScriptCore/wasm/WasmInstance.h:
(JSC::Wasm::Instance::offsetOfTemporaryCallFrame):
(JSC::Wasm::Instance::temporaryCallFrame const):
(JSC::Wasm::Instance::setTemporaryCallFrame):
* Source/JavaScriptCore/wasm/WasmOperations.cpp:
(JSC::Wasm::JSC_DEFINE_JIT_OPERATION):
(JSC::Wasm::WasmOperationsInternal::retrieveAndClearExceptionIfCatchableImpl):
(JSC::Wasm::setWasmTableElement): Deleted.
(JSC::Wasm::wait): Deleted.
* Source/JavaScriptCore/wasm/WasmOperations.h:
* Source/JavaScriptCore/wasm/WasmOperationsInlines.h: Added.
(JSC::Wasm::refFunc):
(JSC::Wasm::arrayNew):
(JSC::Wasm::arrayGet):
(JSC::Wasm::arraySet):
(JSC::Wasm::structNew):
(JSC::Wasm::structGet):
(JSC::Wasm::structSet):
(JSC::Wasm::tableGet):
(JSC::Wasm::tableSet):
(JSC::Wasm::tableInit):
(JSC::Wasm::tableFill):
(JSC::Wasm::tableGrow):
(JSC::Wasm::tableCopy):
(JSC::Wasm::tableSize):
(JSC::Wasm::growMemory):
(JSC::Wasm::memoryInit):
(JSC::Wasm::memoryFill):
(JSC::Wasm::memoryCopy):
(JSC::Wasm::dataDrop):
(JSC::Wasm::elemDrop):
(JSC::Wasm::waitImpl):
(JSC::Wasm::memoryAtomicWait32):
(JSC::Wasm::memoryAtomicWait64):
(JSC::Wasm::memoryAtomicNotify):
(JSC::Wasm::throwWasmToJSException):
* Source/JavaScriptCore/wasm/WasmSlowPaths.cpp:
(JSC::LLInt::WASM_SLOW_PATH_DECL):
(JSC::LLInt::slow_path_wasm_throw_exception):
* Source/JavaScriptCore/wasm/WasmThunks.cpp:
(JSC::Wasm::throwExceptionFromWasmThunkGenerator):
* Source/JavaScriptCore/wasm/js/JSToWasm.cpp:
(JSC::Wasm::marshallJSResult):
* Source/JavaScriptCore/wasm/js/WasmToJS.cpp:
(JSC::Wasm::materializeImportJSCell):
(JSC::Wasm::handleBadImportTypeUse):
(JSC::Wasm::wasmToJS):
(JSC::Wasm::emitThrowWasmToJSException):

Canonical link: https://commits.webkit.org/258820@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/JSC-Save-more-code-size-by-aligning-wasm-operations-to-JSs-convention branch from abc0f0f to 773f16e Compare January 12, 2023 05:38
@webkit-commit-queue
Copy link
Collaborator

Committed 258820@main (773f16e): https://commits.webkit.org/258820@main

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

@webkit-commit-queue webkit-commit-queue merged commit 773f16e into WebKit:main Jan 12, 2023
@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 Jan 12, 2023
@Constellation Constellation deleted the eng/JSC-Save-more-code-size-by-aligning-wasm-operations-to-JSs-convention branch January 12, 2023 06:53
webkit-early-warning-system pushed a commit to jjgriego/WebKit that referenced this pull request Jan 20, 2023
…hrow`

https://bugs.webkit.org/show_bug.cgi?id=250588

Reviewed by Mark Lam.

WebKit#8523 changed the signature of some runtime
wasm operations, but incorrectly translated these changes for the arm32 calling
convention, breaking some tests.

Changes to other wasm operations were covered by the platform-specific
`emitCCall`, but calls to `operationWasmRethrow` are not emitted this way.

(The arm32 calling convention requires that 64-bit arguments get passed starting
in an even-numbered registers, potentially causing an odd-numbered register to
be unused in the call, in this case, `r1/argumentGPR1`)

* Source/JavaScriptCore/wasm/WasmAirIRGenerator32_64.cpp:
(JSC::Wasm::AirIRGenerator32::addRethrow):

Canonical link: https://commits.webkit.org/259152@main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.
Projects
None yet
5 participants