Skip to content

Conversation

@takikawa
Copy link
Contributor

@takikawa takikawa commented Nov 14, 2023

ea71f92

[Wasm-GC] Support bulk array instructions
https://bugs.webkit.org/show_bug.cgi?id=264057

Reviewed by Justin Michaud.

Add support for bulk array operations from the Wasm GC proposal. These
allow bulk operations into GC arrays.

Adds spec tests from commit ff856bef3b717609e9b6854a1c78da80068322e9
of the GC proposal repo.

Also fixes a number of other related issues:
  * Fix incorrect element segment copying ranges for array.new_elem.
  * Fix handling of dropped segments in array.new_elem.
  * Minor fix to exception cases in B3 backend.
  * Fix registration of declared functions in the constant expression
    parser.
  * Adjust error messages
  * Fixed global import for i31refs.

* JSTests/wasm/gc-spec-tests/array_copy.wast.js: Added.
* JSTests/wasm/gc-spec-tests/array_fill.wast.js: Added.
* JSTests/wasm/gc-spec-tests/array_init_data.wast.js: Added.
* JSTests/wasm/gc-spec-tests/array_init_elem.wast.js: Added.
* JSTests/wasm/gc/array_new_elem.js:
(testAllElementSegmentKinds):
* JSTests/wasm/gc/arrays.js:
* JSTests/wasm/gc/bulk-array.js: Added.
(testArrayFill.doTest):
(testArrayFill):
(testArrayCopy.doTest):
(testArrayCopy):
(testArrayInitElem):
(testArrayInitData):
* JSTests/wasm/wasm.json:
* Source/JavaScriptCore/bytecode/BytecodeList.rb:
* Source/JavaScriptCore/llint/WebAssembly.asm:
* Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:
(JSC::Wasm::B3IRGenerator::addArrayNewFixed):
(JSC::Wasm::B3IRGenerator::emitArrayNullCheck):
(JSC::Wasm::B3IRGenerator::addArraySet):
(JSC::Wasm::B3IRGenerator::addArrayFill):
(JSC::Wasm::B3IRGenerator::addArrayCopy):
(JSC::Wasm::B3IRGenerator::addArrayInitElem):
(JSC::Wasm::B3IRGenerator::addArrayInitData):
* Source/JavaScriptCore/wasm/WasmBBQJIT.cpp:
(JSC::Wasm::BBQJIT::addArrayFill):
(JSC::Wasm::BBQJIT::addArrayCopy):
(JSC::Wasm::BBQJIT::addArrayInitElem):
(JSC::Wasm::BBQJIT::addArrayInitData):
* Source/JavaScriptCore/wasm/WasmConstExprGenerator.cpp:
(JSC::Wasm::ConstExprGenerator::declaredFunctions const):
(JSC::Wasm::ConstExprGenerator::addRefFunc):
(JSC::Wasm::parseExtendedConstExpr):
* Source/JavaScriptCore/wasm/WasmConstExprGenerator.h:
* Source/JavaScriptCore/wasm/WasmExceptionType.h:
(JSC::Wasm::isTypeErrorExceptionType):
* Source/JavaScriptCore/wasm/WasmFunctionParser.h:
(JSC::Wasm::FunctionParser<Context>::parseExpression):
(JSC::Wasm::FunctionParser<Context>::parseUnreachableExpression):
* Source/JavaScriptCore/wasm/WasmIPIntGenerator.cpp:
(JSC::Wasm::IPIntGenerator::addArrayFill):
(JSC::Wasm::IPIntGenerator::addArrayCopy):
(JSC::Wasm::IPIntGenerator::addArrayInitElem):
(JSC::Wasm::IPIntGenerator::addArrayInitData):
* Source/JavaScriptCore/wasm/WasmInstance.cpp:
(JSC::Wasm::Instance::copyDataSegment):
(JSC::Wasm::Instance::copyElementSegment):
* Source/JavaScriptCore/wasm/WasmInstance.h:
* Source/JavaScriptCore/wasm/WasmLLIntBuiltin.h:
* Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:
(JSC::Wasm::LLIntGenerator::addArrayFill):
(JSC::Wasm::LLIntGenerator::addArrayCopy):
(JSC::Wasm::LLIntGenerator::addArrayInitElem):
(JSC::Wasm::LLIntGenerator::addArrayInitData):
* Source/JavaScriptCore/wasm/WasmOperations.cpp:
(JSC::Wasm::JSC_DEFINE_JIT_OPERATION):
* Source/JavaScriptCore/wasm/WasmOperations.h:
* Source/JavaScriptCore/wasm/WasmOperationsInlines.h:
(JSC::Wasm::fillArray):
(JSC::Wasm::createArrayFromDataSegment):
(JSC::Wasm::createArrayFromElementSegment):
(JSC::Wasm::arrayNewElem):
(JSC::Wasm::arrayFill):
(JSC::Wasm::arrayCopy):
(JSC::Wasm::arrayInitElem):
(JSC::Wasm::arrayInitData):
* Source/JavaScriptCore/wasm/WasmSlowPaths.cpp:
(JSC::LLInt::WASM_SLOW_PATH_DECL):
* Source/JavaScriptCore/wasm/WasmSlowPaths.h:
* Source/JavaScriptCore/wasm/WasmTypeDefinition.cpp:
(JSC::Wasm::TypeInformation::signatureForLLIntBuiltin):
(JSC::Wasm::TypeInformation::TypeInformation):
* Source/JavaScriptCore/wasm/WasmTypeDefinition.h:
* Source/JavaScriptCore/wasm/js/JSWebAssemblyArray.cpp:
(JSC::JSWebAssemblyArray::fill):
(JSC::JSWebAssemblyArray::copy):
* Source/JavaScriptCore/wasm/js/JSWebAssemblyArray.h:
* Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:
(JSC::WebAssemblyModuleRecord::initializeImports):
* Source/JavaScriptCore/wasm/wasm.json:

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

ebb40f1

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

@takikawa takikawa requested a review from a team as a code owner November 14, 2023 07:14
@takikawa takikawa self-assigned this Nov 14, 2023
@takikawa takikawa added the WebAssembly For bugs in JavaScript WebAssembly label Nov 14, 2023
Copy link
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.

Can you add InPlace interpreter implementation too?

@takikawa
Copy link
Contributor Author

Can you add InPlace interpreter implementation too?

Sure I can look into this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment is about working around what might be a bug in BBQJIT. I'm planning to file a proper bug report and put the bug link in here.

The basic issue seems to be that for a call to a WasmOperation, if the call arguments need to be allocated on the stack, it breaks in debug build mode on Linux. This wasn't an issue before because I don't think other operations exceed 6 arguments (x86-64 unix calling convention has 6 arg regs).

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 14, 2023
@takikawa takikawa removed the merging-blocked Applied to prevent a change from being merged label Nov 16, 2023
@takikawa takikawa force-pushed the eng/Wasm-GC-Support-bulk-array-instructions branch from 272f39a to 3f790bd Compare November 16, 2023 05:11
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for 3f790bd. Live statuses available at the PR page, #20465

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 16, 2023
@takikawa
Copy link
Contributor Author

takikawa commented Nov 16, 2023

Can you add InPlace interpreter implementation too?

I looked into this and added the metadata generation at least for the new instructions. It seemed like there wasn't infrastructure yet for GC in the actual interpreter code (e.g., for dispatching on GC instructions and such), so I didn't add the runtime part, but let me know if I'm missing something. @Constellation

I fixed the ARMv7 failures so I think this is good otherwise though.

@Constellation
Copy link
Member

Please fix commit message format, which does not have bugzilla URL on the second line.

@takikawa takikawa removed the merging-blocked Applied to prevent a change from being merged label Dec 8, 2023
@takikawa takikawa changed the title Support bulk array instructions https://bugs.webkit.org/show_bug.cgi?id=264057 [Wasm-GC] Support bulk array instructions Dec 8, 2023
@takikawa takikawa force-pushed the eng/Wasm-GC-Support-bulk-array-instructions branch from 3f790bd to 849b4fb Compare December 8, 2023 15:53
@takikawa
Copy link
Contributor Author

takikawa commented Dec 8, 2023

Please fix commit message format, which does not have bugzilla URL on the second line.

Done, thanks!

@takikawa
Copy link
Contributor Author

takikawa commented Dec 9, 2023

Looks like tests got a bit stale due to other PRs landing (type printing behavior has changed). Will fix those and see if there are other test failures.

@takikawa takikawa force-pushed the eng/Wasm-GC-Support-bulk-array-instructions branch from 849b4fb to 513d920 Compare December 9, 2023 01:11
@takikawa
Copy link
Contributor Author

takikawa commented Dec 9, 2023

Fixed the tests, there is a trivial merge conflict with #21184 (which I'll fix) but otherwise should be good

@takikawa takikawa force-pushed the eng/Wasm-GC-Support-bulk-array-instructions branch from 513d920 to adaf507 Compare December 11, 2023 15:38
@takikawa takikawa force-pushed the eng/Wasm-GC-Support-bulk-array-instructions branch from adaf507 to 44e2c0d Compare December 14, 2023 01:06
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 14, 2023
@bashor
Copy link

bashor commented Dec 18, 2023

Are any updates required in this PR? 🥺

Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing a write barrier, right? You should be able to get it to crash with collectContinuously=1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be ok actually, because reftypes are handled separately. Specifically on line 106 if the element type is a reftype then it will actually call the set method on each index instead of doing std::fill, which will use the write barrier path.

Same with the copy method below which also has a slow path for reftypes.

There might be a write barrier issue with array.init_elem though, which I'll look into before merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For array.init_elem, I added a vm.writeBarrier() call to ensure the write barrier is used after the write into the array (though I wasn't able to trigger any failures with test cases for this). That should address everything that's left on this.

Copy link
Contributor

@justinmichaud justinmichaud 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 with the write barrier comment resolved. I think all three functions may be missing one, unless I am missing something

@takikawa takikawa removed the merging-blocked Applied to prevent a change from being merged label Jan 5, 2024
@takikawa takikawa force-pushed the eng/Wasm-GC-Support-bulk-array-instructions branch from 44e2c0d to ebb40f1 Compare January 5, 2024 01:44
@takikawa takikawa added the safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks label Jan 5, 2024
@webkit-ews-buildbot webkit-ews-buildbot added merging-blocked Applied to prevent a change from being merged merge-queue Applied to send a pull request to merge-queue and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Jan 5, 2024
@webkit-ews-buildbot
Copy link
Collaborator

Safe-Merge-Queue: Build #8283.

@takikawa takikawa added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged merge-queue Applied to send a pull request to merge-queue labels Jan 5, 2024
https://bugs.webkit.org/show_bug.cgi?id=264057

Reviewed by Justin Michaud.

Add support for bulk array operations from the Wasm GC proposal. These
allow bulk operations into GC arrays.

Adds spec tests from commit ff856bef3b717609e9b6854a1c78da80068322e9
of the GC proposal repo.

Also fixes a number of other related issues:
  * Fix incorrect element segment copying ranges for array.new_elem.
  * Fix handling of dropped segments in array.new_elem.
  * Minor fix to exception cases in B3 backend.
  * Fix registration of declared functions in the constant expression
    parser.
  * Adjust error messages
  * Fixed global import for i31refs.

* JSTests/wasm/gc-spec-tests/array_copy.wast.js: Added.
* JSTests/wasm/gc-spec-tests/array_fill.wast.js: Added.
* JSTests/wasm/gc-spec-tests/array_init_data.wast.js: Added.
* JSTests/wasm/gc-spec-tests/array_init_elem.wast.js: Added.
* JSTests/wasm/gc/array_new_elem.js:
(testAllElementSegmentKinds):
* JSTests/wasm/gc/arrays.js:
* JSTests/wasm/gc/bulk-array.js: Added.
(testArrayFill.doTest):
(testArrayFill):
(testArrayCopy.doTest):
(testArrayCopy):
(testArrayInitElem):
(testArrayInitData):
* JSTests/wasm/wasm.json:
* Source/JavaScriptCore/bytecode/BytecodeList.rb:
* Source/JavaScriptCore/llint/WebAssembly.asm:
* Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:
(JSC::Wasm::B3IRGenerator::addArrayNewFixed):
(JSC::Wasm::B3IRGenerator::emitArrayNullCheck):
(JSC::Wasm::B3IRGenerator::addArraySet):
(JSC::Wasm::B3IRGenerator::addArrayFill):
(JSC::Wasm::B3IRGenerator::addArrayCopy):
(JSC::Wasm::B3IRGenerator::addArrayInitElem):
(JSC::Wasm::B3IRGenerator::addArrayInitData):
* Source/JavaScriptCore/wasm/WasmBBQJIT.cpp:
(JSC::Wasm::BBQJIT::addArrayFill):
(JSC::Wasm::BBQJIT::addArrayCopy):
(JSC::Wasm::BBQJIT::addArrayInitElem):
(JSC::Wasm::BBQJIT::addArrayInitData):
* Source/JavaScriptCore/wasm/WasmConstExprGenerator.cpp:
(JSC::Wasm::ConstExprGenerator::declaredFunctions const):
(JSC::Wasm::ConstExprGenerator::addRefFunc):
(JSC::Wasm::parseExtendedConstExpr):
* Source/JavaScriptCore/wasm/WasmConstExprGenerator.h:
* Source/JavaScriptCore/wasm/WasmExceptionType.h:
(JSC::Wasm::isTypeErrorExceptionType):
* Source/JavaScriptCore/wasm/WasmFunctionParser.h:
(JSC::Wasm::FunctionParser<Context>::parseExpression):
(JSC::Wasm::FunctionParser<Context>::parseUnreachableExpression):
* Source/JavaScriptCore/wasm/WasmIPIntGenerator.cpp:
(JSC::Wasm::IPIntGenerator::addArrayFill):
(JSC::Wasm::IPIntGenerator::addArrayCopy):
(JSC::Wasm::IPIntGenerator::addArrayInitElem):
(JSC::Wasm::IPIntGenerator::addArrayInitData):
* Source/JavaScriptCore/wasm/WasmInstance.cpp:
(JSC::Wasm::Instance::copyDataSegment):
(JSC::Wasm::Instance::copyElementSegment):
* Source/JavaScriptCore/wasm/WasmInstance.h:
* Source/JavaScriptCore/wasm/WasmLLIntBuiltin.h:
* Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:
(JSC::Wasm::LLIntGenerator::addArrayFill):
(JSC::Wasm::LLIntGenerator::addArrayCopy):
(JSC::Wasm::LLIntGenerator::addArrayInitElem):
(JSC::Wasm::LLIntGenerator::addArrayInitData):
* Source/JavaScriptCore/wasm/WasmOperations.cpp:
(JSC::Wasm::JSC_DEFINE_JIT_OPERATION):
* Source/JavaScriptCore/wasm/WasmOperations.h:
* Source/JavaScriptCore/wasm/WasmOperationsInlines.h:
(JSC::Wasm::fillArray):
(JSC::Wasm::createArrayFromDataSegment):
(JSC::Wasm::createArrayFromElementSegment):
(JSC::Wasm::arrayNewElem):
(JSC::Wasm::arrayFill):
(JSC::Wasm::arrayCopy):
(JSC::Wasm::arrayInitElem):
(JSC::Wasm::arrayInitData):
* Source/JavaScriptCore/wasm/WasmSlowPaths.cpp:
(JSC::LLInt::WASM_SLOW_PATH_DECL):
* Source/JavaScriptCore/wasm/WasmSlowPaths.h:
* Source/JavaScriptCore/wasm/WasmTypeDefinition.cpp:
(JSC::Wasm::TypeInformation::signatureForLLIntBuiltin):
(JSC::Wasm::TypeInformation::TypeInformation):
* Source/JavaScriptCore/wasm/WasmTypeDefinition.h:
* Source/JavaScriptCore/wasm/js/JSWebAssemblyArray.cpp:
(JSC::JSWebAssemblyArray::fill):
(JSC::JSWebAssemblyArray::copy):
* Source/JavaScriptCore/wasm/js/JSWebAssemblyArray.h:
* Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:
(JSC::WebAssemblyModuleRecord::initializeImports):
* Source/JavaScriptCore/wasm/wasm.json:

Canonical link: https://commits.webkit.org/272677@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Wasm-GC-Support-bulk-array-instructions branch from ebb40f1 to ea71f92 Compare January 5, 2024 07:57
@webkit-commit-queue
Copy link
Collaborator

Committed 272677@main (ea71f92): https://commits.webkit.org/272677@main

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

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 5, 2024
@webkit-commit-queue webkit-commit-queue merged commit ea71f92 into WebKit:main Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WebAssembly For bugs in JavaScript WebAssembly

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants