1592926 - Adapt wasmTextToBinary for multi-value

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P3)

Description

[Lars T Hansen [:lth]]

wasmTextToBinary needs to handle multi-value situations properly. This is probably a substantial job, see discussions on bug 1527871 comment 3 et seq, bug 1584097 for starters. In the former thread, wingo writes:

"If you were to fix wasmTextToBinary, I think you would start by removing AstExprType and AstExpr from WasmAST.h. Because we want to test cases in which values flow in through stack parameters rather than restricting to nested parameters, this abstraction doesn't work. Of course this means lots of adaptation both in WasmTextToBinary but also the AsmJS compiler and the wasm encoder. You would get to keep most concrete classes (e.g. AstCall) but they would inherit from AstNode and not store a type and not reference subexpressions. Then I think I would move to implement the unfolding algorithm from the spec. It would be a big couple patches and I would estimate it to take about 10 working days or so."

My only comment to that is that I don't think asm.js is actually affected, but it does look like a substantial piece of work in any case.

Comment 1

[Lars T Hansen [:lth]]

Attachment: Bug 1592926 - Remove AstExprType, introduce AstBlockType. r=wingo

Comment 2

[Lars T Hansen [:lth]]

First patch gets rid of AstExprType and allows a FuncType reference to be encoded as part of the new AstBlockType, used for BLOCK, LOOP, and IF. We need access to the AstModule during parsing so this patch puts it into the parsing context and gets rid of the module argument, that can be factored out. The patch is otherwise not quite clean, probably some code can be merged / refactored.

Anyway this is enough to allow us to parse eg (block (result i32 i32) E1 E2) i32.add, but other issues around expression parsing still disallow the parsing of the parenthesized form (i32.add (block (result i32 i32) E1 E2)). I know that syntax is considered well-formed but frankly I'm uncertain that that's actually a feature.

But problems elsewhere - notably, function types are not allowed by the validator to have more than one result - prevent us from doing much more than parsing at the moment.

Comment 3

[Lars T Hansen [:lth]]

Attachment: Bug 1592926 - Store the module in the wasm parse context, don't pass it. r=wingo

Comment 4

[Lars T Hansen [:lth]]

Attachment: Bug 1592926 - Adjustments to existing multi-value code. DO NOT LAND

Depends on D51316

Comment 5

[Lars T Hansen [:lth]]

This patch set has a bug, it uses an overly elaborate (and incorrect) encoding for the function index. Fix coming.

Comment 6

[Andy Wingo [:wingo]]

Clever hack! Thank you so much for this!!!

Comment 7

[Lars T Hansen [:lth]]

Attachment: Bug 1592926 - Wingo's test cases, sans wassemble. DO NOT LAND.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ff758d2b677
Store the module in the wasm parse context, don't pass it. r=wingo
https://hg.mozilla.org/integration/autoland/rev/0de1220699ef
Remove AstExprType, introduce AstBlockType. r=wingo

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1605a3463b56
Fix bustages in WasmTextToBinary.cpp and WasmAST.h a=bustage-fix CLOSED TREE

Comment 10

[Lars T Hansen [:lth]]

This probably needs to be amended to change the encoding of the function index, this should be a 33-bit sleb, not a 32-bit uleb as now. We won't notice any problems until we start testing wasm content from other producers.

Comment 11

[Andy Wingo [:wingo]]

Note that the maximum number of types is 1e7, if that makes encoding the s33 any easier: http://webassembly.github.io/spec/js-api/index.html#limits

Comment 12

[Lars T Hansen [:lth]]

Yeah, it means we can use writeVarS32 at least to write it, though I'm not sure yet about decoding whether readVarS32() has the exact semantics we want. It probably does, because it must read up to ceil(32/7) bytes and it's required to read up to ceil(33/7) bytes, and those two values are the same.

The problems I encountered before with readValType were probably spurious: in an older draft, I thought I needed to encode this type with a TypeCode::Func prefix, and that would have led the decoder into readValType, where it should not be.

Comment 13

[Lars T Hansen [:lth]]

Attachment: Bug 1592926 - Encode function index in block type as sleb. r=wingo

Function indices must be encoded as sleb33 to disambiguate from
valtypes, which are slebs. The enclosed test demonstrates a case
where the uleb encoding results in an error.

Drive-by fix: change nomenclature in wasmTextToBinary to match
nomenclature used elsewhere.

Comment 14

[Lars T Hansen [:lth]]

Attachment: Bug 1592926 - Addendum to Wingo's tests. DO NOT LAND.

Wingo will incorporate this into his tests when he lands them.

Depends on D51780

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57442cf2bb9d
Encode function index in block type as sleb. r=wingo

Comment 16

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/1ff758d2b677
https://hg.mozilla.org/mozilla-central/rev/0de1220699ef
https://hg.mozilla.org/mozilla-central/rev/1605a3463b56
https://hg.mozilla.org/mozilla-central/rev/57442cf2bb9d