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

[Wasm-GC] Add extern.internalize/externalize #11666

Conversation

takikawa
Copy link
Contributor

@takikawa takikawa commented Mar 17, 2023

e537d9e

[Wasm-GC] Add extern.internalize/externalize
https://bugs.webkit.org/show_bug.cgi?id=251039

Reviewed by Justin Michaud.

Adds extern conversion instructions to convert between anyref (internal
references) and externref. The any to extern direction is currently a
no-op.

The extern to any direction requires checking for Numbers in the i31
range and converting them to i31ref representation.

The any to extern is a no-op because JSC's value representation allows
i31s to just be JSC 32-bit exact integers, and structs and arrays are
represented as objects. This would need to change if the representation
were to change, so that, e.g., structs and arrays have an optimized
non-object wasm representation and a wrapper object is used to interop
with JS.

The addition of internalize means non-Wasm values (JS objects, strings,
etc) can be converted into an internal reference as "host references"
that are opaque to Wasm. These need to be distinguishable from true Wasm
values, including in JIT code, which requires adding a WasmGCObjectType
JSType.

* JSTests/wasm/gc/extern.js: Added.
(testInternalize):
(testRoundtrip):
(testTable):
* JSTests/wasm/wasm.json:
* Source/JavaScriptCore/bytecode/BytecodeList.rb:
* Source/JavaScriptCore/llint/WebAssembly32_64.asm:
* Source/JavaScriptCore/llint/WebAssembly64.asm:
* Source/JavaScriptCore/runtime/JSType.cpp:
(WTF::printInternal):
* Source/JavaScriptCore/runtime/JSType.h:
* Source/JavaScriptCore/wasm/WasmAirIRGeneratorBase.h:
(JSC::Wasm::ExpressionType>::emitRefTestOrCast):
(JSC::Wasm::ExpressionType>::makeBranchNotWasmGCObject):
(JSC::Wasm::ExpressionType>::addExternInternalize):
(JSC::Wasm::ExpressionType>::addExternExternalize):
* Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:
(JSC::Wasm::B3IRGenerator::emitRefTestOrCast):
(JSC::Wasm::B3IRGenerator::addExternInternalize):
(JSC::Wasm::B3IRGenerator::addExternExternalize):
* Source/JavaScriptCore/wasm/WasmBBQJIT.cpp:
(JSC::Wasm::BBQJIT::addExternInternalize):
(JSC::Wasm::BBQJIT::addExternExternalize):
* Source/JavaScriptCore/wasm/WasmFormat.h:
(JSC::Wasm::externrefType):
(JSC::Wasm::anyrefType):
* Source/JavaScriptCore/wasm/WasmFunctionParser.h:
(JSC::Wasm::FunctionParser<Context>::parseExpression):
* Source/JavaScriptCore/wasm/WasmLLIntBuiltin.h:
* Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:
(JSC::Wasm::LLIntGenerator::addExternInternalize):
(JSC::Wasm::LLIntGenerator::addExternExternalize):
* Source/JavaScriptCore/wasm/WasmOperations.cpp:
(JSC::Wasm::JSC_DEFINE_JIT_OPERATION):
* Source/JavaScriptCore/wasm/WasmOperations.h:
* Source/JavaScriptCore/wasm/WasmOperationsInlines.h:
(JSC::Wasm::refCast):
(JSC::Wasm::externInternalize):
* Source/JavaScriptCore/wasm/WasmSlowPaths.cpp:
(JSC::LLInt::WASM_SLOW_PATH_DECL):
* Source/JavaScriptCore/wasm/WasmTypeDefinition.cpp:
(JSC::Wasm::TypeInformation::signatureForLLIntBuiltin):
(JSC::Wasm::TypeInformation::TypeInformation):
* Source/JavaScriptCore/wasm/WasmTypeDefinition.h:
* Source/JavaScriptCore/wasm/js/JSWebAssemblyArray.h:
* Source/JavaScriptCore/wasm/js/JSWebAssemblyStruct.h:
* Source/JavaScriptCore/wasm/wasm.json:

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

3ac125e

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

@takikawa takikawa self-assigned this Mar 17, 2023
@takikawa takikawa added the WebAssembly For bugs in JavaScript WebAssembly label Mar 17, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 17, 2023
@takikawa takikawa removed the merging-blocked Applied to prevent a change from being merged label Mar 17, 2023
@takikawa takikawa force-pushed the eng/Wasm-GC-Add-extern-internalizeexternalize branch from 3010140 to 4c3a5fc Compare March 17, 2023 22:55
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 17, 2023
@takikawa takikawa removed the merging-blocked Applied to prevent a change from being merged label Mar 18, 2023
@takikawa takikawa force-pushed the eng/Wasm-GC-Add-extern-internalizeexternalize branch from 4c3a5fc to 6a4ea67 Compare March 18, 2023 00:03
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 18, 2023
@takikawa takikawa removed the merging-blocked Applied to prevent a change from being merged label Mar 21, 2023
@takikawa takikawa force-pushed the eng/Wasm-GC-Add-extern-internalizeexternalize branch from 6a4ea67 to e379d0d Compare March 21, 2023 23:11
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 22, 2023
@takikawa takikawa removed the merging-blocked Applied to prevent a change from being merged label Mar 25, 2023
@takikawa takikawa force-pushed the eng/Wasm-GC-Add-extern-internalizeexternalize branch from e379d0d to 60ff12d Compare March 25, 2023 00:09
@takikawa takikawa marked this pull request as ready for review March 25, 2023 17:26
@takikawa takikawa requested a review from a team as a code owner March 25, 2023 17:26
@takikawa
Copy link
Contributor Author

Should be ready for review now. I forgot to update the commit message, but I plan to add an explanation about eqrefs like the following:

  • This patch introduces the ability to create "hostrefs", which are internal Wasm references that are not subtypes of eqref. As a result, code paths checking for eqrefs at runtime now have to check that the value is either i31, struct, or array (before eqref was essentially equivalent to anyref).

@takikawa
Copy link
Contributor Author

takikawa commented Mar 25, 2023

Notes for extern conversions:

  • any->extern is a no-op because JSC's value representation allows i31s to just be JSC 32-bit exact integers, and structs and arrays are represented as objects to begin with. This would need to change if the representation were to change, so that, e.g., structs and arrays have an optimized non-object wasm representation and a wrapper object is used to interop with JS.
  • extern->any requires coercing any Number in i31 range to an i31ref. This ensures that hostrefs and i31refs are dynamically distinguishable, otherwise casts would expose implementation details about i31s (see https://github.com/WebAssembly/gc/blob/main/proposals/gc/MVP-JS.md).

@takikawa
Copy link
Contributor Author

The wasm.json file needs to be synced with the one in the tests directory, I'll do that when I update the PR next.

@@ -3604,6 +3604,21 @@ class BBQJIT {
PartialResult WARN_UNUSED_RETURN addRefCast(ExpressionType, bool, int32_t, ExpressionType&) BBQ_STUB
PartialResult WARN_UNUSED_RETURN addRefTest(ExpressionType, bool, int32_t, ExpressionType&) BBQ_STUB

PartialResult WARN_UNUSED_RETURN addExternInternalize(ExpressionType reference, ExpressionType& result)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: due to Wasm GC disabling BBQJIT, this isn't really testable right now. If it's better to stub it out, I can change this to stub code.

@@ -904,6 +904,7 @@ class TypeInformation {
RefPtr<TypeDefinition> m_I32_RefI32I32;
RefPtr<TypeDefinition> m_Ref_RefI32I32;
RefPtr<TypeDefinition> m_Ref_I32I32I32I32;
RefPtr<TypeDefinition> m_Anyref_Externref;
Copy link
Contributor Author

@takikawa takikawa Mar 27, 2023

Choose a reason for hiding this comment

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

Note: The style CI failure is due to this member variable naming, which is named like this for consistency with the others.

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

@@ -127,6 +127,7 @@ enum JSType : uint8_t {
JSWeakSetType,
WebAssemblyModuleType,
WebAssemblyInstanceType,
WebAssemblyGCObjectType,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish we could avoid adding a new type here

Copy link
Contributor Author

@takikawa takikawa Mar 28, 2023

Choose a reason for hiding this comment

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

Yeah unfortunately I'm not aware of a way to avoid it. The C++ code can just use jsDynamicCast (https://github.com/WebKit/WebKit/blob/60ff12d314ad2127a1ff211c05636ff630f29582/Source/JavaScriptCore/wasm/WasmOperationsInlines.h#L300) but I don't think it's easy to use this generic dispatch from JIT since it involves calling JSCell::inherits IIUC.

@takikawa takikawa force-pushed the eng/Wasm-GC-Add-extern-internalizeexternalize branch from 60ff12d to 3ac125e Compare March 28, 2023 21:33
@takikawa takikawa added the merge-queue Applied to send a pull request to merge-queue label Mar 29, 2023
https://bugs.webkit.org/show_bug.cgi?id=251039

Reviewed by Justin Michaud.

Adds extern conversion instructions to convert between anyref (internal
references) and externref. The any to extern direction is currently a
no-op.

The extern to any direction requires checking for Numbers in the i31
range and converting them to i31ref representation.

The any to extern is a no-op because JSC's value representation allows
i31s to just be JSC 32-bit exact integers, and structs and arrays are
represented as objects. This would need to change if the representation
were to change, so that, e.g., structs and arrays have an optimized
non-object wasm representation and a wrapper object is used to interop
with JS.

The addition of internalize means non-Wasm values (JS objects, strings,
etc) can be converted into an internal reference as "host references"
that are opaque to Wasm. These need to be distinguishable from true Wasm
values, including in JIT code, which requires adding a WasmGCObjectType
JSType.

* JSTests/wasm/gc/extern.js: Added.
(testInternalize):
(testRoundtrip):
(testTable):
* JSTests/wasm/wasm.json:
* Source/JavaScriptCore/bytecode/BytecodeList.rb:
* Source/JavaScriptCore/llint/WebAssembly32_64.asm:
* Source/JavaScriptCore/llint/WebAssembly64.asm:
* Source/JavaScriptCore/runtime/JSType.cpp:
(WTF::printInternal):
* Source/JavaScriptCore/runtime/JSType.h:
* Source/JavaScriptCore/wasm/WasmAirIRGeneratorBase.h:
(JSC::Wasm::ExpressionType>::emitRefTestOrCast):
(JSC::Wasm::ExpressionType>::makeBranchNotWasmGCObject):
(JSC::Wasm::ExpressionType>::addExternInternalize):
(JSC::Wasm::ExpressionType>::addExternExternalize):
* Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:
(JSC::Wasm::B3IRGenerator::emitRefTestOrCast):
(JSC::Wasm::B3IRGenerator::addExternInternalize):
(JSC::Wasm::B3IRGenerator::addExternExternalize):
* Source/JavaScriptCore/wasm/WasmBBQJIT.cpp:
(JSC::Wasm::BBQJIT::addExternInternalize):
(JSC::Wasm::BBQJIT::addExternExternalize):
* Source/JavaScriptCore/wasm/WasmFormat.h:
(JSC::Wasm::externrefType):
(JSC::Wasm::anyrefType):
* Source/JavaScriptCore/wasm/WasmFunctionParser.h:
(JSC::Wasm::FunctionParser<Context>::parseExpression):
* Source/JavaScriptCore/wasm/WasmLLIntBuiltin.h:
* Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:
(JSC::Wasm::LLIntGenerator::addExternInternalize):
(JSC::Wasm::LLIntGenerator::addExternExternalize):
* Source/JavaScriptCore/wasm/WasmOperations.cpp:
(JSC::Wasm::JSC_DEFINE_JIT_OPERATION):
* Source/JavaScriptCore/wasm/WasmOperations.h:
* Source/JavaScriptCore/wasm/WasmOperationsInlines.h:
(JSC::Wasm::refCast):
(JSC::Wasm::externInternalize):
* Source/JavaScriptCore/wasm/WasmSlowPaths.cpp:
(JSC::LLInt::WASM_SLOW_PATH_DECL):
* Source/JavaScriptCore/wasm/WasmTypeDefinition.cpp:
(JSC::Wasm::TypeInformation::signatureForLLIntBuiltin):
(JSC::Wasm::TypeInformation::TypeInformation):
* Source/JavaScriptCore/wasm/WasmTypeDefinition.h:
* Source/JavaScriptCore/wasm/js/JSWebAssemblyArray.h:
* Source/JavaScriptCore/wasm/js/JSWebAssemblyStruct.h:
* Source/JavaScriptCore/wasm/wasm.json:

Canonical link: https://commits.webkit.org/262282@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Wasm-GC-Add-extern-internalizeexternalize branch from 3ac125e to e537d9e Compare March 29, 2023 18:24
@webkit-commit-queue
Copy link
Collaborator

Committed 262282@main (e537d9e): https://commits.webkit.org/262282@main

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

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 29, 2023
@webkit-commit-queue webkit-commit-queue merged commit e537d9e into WebKit:main Mar 29, 2023
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
5 participants