[Wasm-GC] Introduce i31ref types#2202
[Wasm-GC] Introduce i31ref types#2202webkit-early-warning-system merged 1 commit intoWebKit:mainfrom
Conversation
|
I was originally thinking of leaving the 32-bit support out in this initial patch, but it looks like EWS won't be happy that way so I will add that into the patch and revise this PR. |
There was a problem hiding this comment.
The spec says to trap if null, but I am wondering: if i31 refs aren't nullable, then shouldn't downcasts and imports trap before getting here? The only way to get here is by passing in null directly? This seems somewhat unfortunate unless I am missing something
There was a problem hiding this comment.
if i31 refs aren't nullable, then shouldn't downcasts and imports trap before getting here?
In general it should be possible for a non-null i31ref to get passed in here, they share a null with other reference types. For example the type i31ref is shorthand for (ref null i31) and includes null.
I think it might be possible in the B3/Air tiers to elide the null check though by inspecting the argument type and seeing if it allows null.
JSTests/wasm/gc/i31.js
Outdated
There was a problem hiding this comment.
We should make a script or IDE plugin to compile/decompile these automatically
There was a problem hiding this comment.
What do you think about using the Wasm GC spec's reference interpreter (bundling the compiled to JS code) to write these tests? Then the tests can all be written in text format, except those that test malformed binary format code.
Currently the spec interpreter's compile-to-JS mode appears to be broken, but I'm looking into a possible fix.
28417a3 to
2c37b45
Compare
justinmichaud
left a comment
There was a problem hiding this comment.
LGTM as long as we gate everything by the Wasm GC option, not just typed function references. r=me
There was a problem hiding this comment.
So is this supposed to be a number, or an instance of the JS type Webassembly.i31ref? I'm not sure what the current status of the spec is, it seems like the JS api document is out of date?
There was a problem hiding this comment.
The JS API isn't fully decided yet, but I think it's likely that the semantics will be that I31refs that flow to JS will be interpreted as JS Numbers. For the other direction, JS numbers that are in the i31ref range would be normalized/converted to an i31ref (based on discussion in WebAssembly/gc#76).
For now this PR should let Wasm i31refs get treated as JS values, but the other direction currently should fail and error as the conversion logic hasn't been implemented yet.
Source/JavaScriptCore/wasm/wasm.json
Outdated
There was a problem hiding this comment.
Should the B3 type be an Int64?
There was a problem hiding this comment.
I had left this as Void here to be consistent with, e.g., structs. In particular TypeKind::I31ref shouldn't show up as a type on its own (same with structs), because the parser should always convert an i31ref type to TypeKind::Ref (or RefNull) with an I31 heap type.
2c37b45 to
99ef4f8
Compare
|
I think I've addressed essentially all the comments now (gated by GC flag, uses single exception for The relevant diffs are:
Re: tests, I'd be happy to follow up with a separate PR to convert all of the GC proposal tests we have so far to a nicer text format embedding a JS-compiled version of the Wasm reference interpreter, so that the comment text and binary cannot go out-of-sync. |
|
This looks good to me, I don't know if you want to get feedback from others on JSC or from 32-bit folks. It seems unlikely to break anything though, so LGTM. |
99ef4f8 to
3db6bed
Compare
|
Thanks for the review! I think I'm happy with the PR now, I tested the 32-bit support as well on an arm7 machine and it looks like the CI is green now. I don't have the committer flag so will need someone else to apply the merge-queue label though. |
3db6bed to
debcb9f
Compare
debcb9f to
f550016
Compare
https://bugs.webkit.org/show_bug.cgi?id=242261 Reviewed by Justin Michaud. Adds support for i31ref types and i31 operations (i31.new, i31.get_s, i32.get_u) when experimental GC support is turned on. As Wasm reference types are represented as encoded JS pointer values, i31refs are represented with encoded JS integer values with the standard NumberTag for 32-bit integers. * JSTests/wasm/gc/i31.js: Added. (module): (testI31Type): (testI31New): (testI31Get): (testI31JS): * JSTests/wasm/wasm.json: * Source/JavaScriptCore/bytecode/BytecodeList.rb: * Source/JavaScriptCore/llint/WebAssembly32_64.asm: * Source/JavaScriptCore/llint/WebAssembly64.asm: * Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp: (JSC::Wasm::AirIRGenerator::addI31New): (JSC::Wasm::AirIRGenerator::addI31GetS): (JSC::Wasm::AirIRGenerator::addI31GetU): * Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp: (JSC::Wasm::B3IRGenerator::addI31New): (JSC::Wasm::B3IRGenerator::addI31GetS): (JSC::Wasm::B3IRGenerator::addI31GetU): * Source/JavaScriptCore/wasm/WasmExceptionType.h: * Source/JavaScriptCore/wasm/WasmFormat.h: (JSC::Wasm::isI31ref): (JSC::Wasm::isRefWithTypeIndex): (JSC::Wasm::isValidHeapTypeKind): * Source/JavaScriptCore/wasm/WasmFunctionParser.h: (JSC::Wasm::FunctionParser<Context>::parseExpression): (JSC::Wasm::FunctionParser<Context>::parseUnreachableExpression): * Source/JavaScriptCore/wasm/WasmGlobal.cpp: (JSC::Wasm::Global::set): * Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp: (JSC::Wasm::LLIntGenerator::callInformationForCaller): (JSC::Wasm::LLIntGenerator::callInformationForCallee): (JSC::Wasm::LLIntGenerator::addArguments): (JSC::Wasm::LLIntGenerator::addI31New): (JSC::Wasm::LLIntGenerator::addI31GetS): (JSC::Wasm::LLIntGenerator::addI31GetU): * Source/JavaScriptCore/wasm/WasmParser.h: (JSC::Wasm::Parser<SuccessType>::parseValueType): * Source/JavaScriptCore/wasm/js/JSToWasm.cpp: (JSC::Wasm::marshallJSResult): * Source/JavaScriptCore/wasm/js/JSWebAssemblyHelpers.h: (JSC::fromJSValue): * Source/JavaScriptCore/wasm/js/WasmToJS.cpp: (JSC::Wasm::wasmToJS): * Source/JavaScriptCore/wasm/js/WebAssemblyGlobalConstructor.cpp: (JSC::JSC_DEFINE_HOST_FUNCTION): * Source/JavaScriptCore/wasm/wasm.json: Canonical link: https://commits.webkit.org/252688@main
f550016 to
2215792
Compare
|
Committed 252688@main (2215792): https://commits.webkit.org/252688@main Reviewed commits have been landed. Closing PR #2202 and removing active labels. |
2215792