We're currently at 0xC.
Created attachment 295701 [details] patch
Created attachment 295702 [details] Helper script to synchronize BinaryFormat.md and wasm.json
This patch modifies one of the wasm.json files. Please ensure that any changes in one have been mirrored to the other. You can find the wasm.json files at "Source/JavaScriptCore/wasm/wasm.json" and "JSTests/wasm/wasm.json".
Comment on attachment 295701 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=295701&action=review > Source/JavaScriptCore/wasm/wasm.json:10 > + "i32": { "type": "varint7", "value": -1, "b3type": "B3::Int32" }, What's "varint7"?
(In reply to comment #4) > Comment on attachment 295701 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=295701&action=review > > > Source/JavaScriptCore/wasm/wasm.json:10 > > + "i32": { "type": "varint7", "value": -1, "b3type": "B3::Int32" }, > > What's "varint7"? It's a signed LEB128 variable length integer, encoded in at most a byte (so 7 useful bits of encoding): https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#varintn
Comment on attachment 295701 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=295701&action=review r=me > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:468 > + case Wasm::Void: > + case Wasm::Func: > + case Wasm::Anyfunc: > + break; Would this be a FIXME? If not, maybe just put RELEASE_ASSERT_NOT_REACHED here? > Source/JavaScriptCore/wasm/WasmFormat.h:59 > + // FIXME auto-generate this. Please link a bugzilla here > Source/JavaScriptCore/wasm/wasm.json:93 > + "i32.add": { "category": "arithmetic", "value": 106, "return": ["i32"], "parameter": ["i32", "i32"], "immediate": [], "b3op": "Add" }, Where do these numbers come from?
Comment on attachment 295701 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=295701&action=review >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:468 >> + break; > > Would this be a FIXME? If not, maybe just put RELEASE_ASSERT_NOT_REACHED here? That would just be a bug in the calling code. I thought having the assert outside a switch was the idiomatic thing, because (IIRC) MSVC can't figure out that all paths return if the switch covers all cases and has no return after: int foo(enum Type t) { switch (t) { case Foo: // ... cover all of Type's values } // No return here makes some MSVCs sad. } >> Source/JavaScriptCore/wasm/wasm.json:93 >> + "i32.add": { "category": "arithmetic", "value": 106, "return": ["i32"], "parameter": ["i32", "i32"], "immediate": [], "b3op": "Add" }, > > Where do these numbers come from? Discussed offline, comes from https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md
Created attachment 295790 [details] patch Add FIXME URL.
Created attachment 295820 [details] patch Minor rebase+fix with my previous patch, and Yusuke's patch.
Comment on attachment 295820 [details] patch Clearing flags on attachment: 295820 Committed r209175: <http://trac.webkit.org/changeset/209175>
All reviewed patches have been landed. Closing bug.
Comment on attachment 295820 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=295820&action=review > JSTests/wasm/Builder.js:54 > + assert.truthy(WASM.isValidBlockType(ret), `Type return ${ret} must be valid block type`); I think isValidBlockType is called isValidReturnType in the C++ code. We should probably use the same name for consistency. I'm indifferent to which one we choose though.
Trying to revert here due to odd breakage: https://bugs.webkit.org/show_bug.cgi?id=165308