Closed Bug 1592926 Opened 8 months ago Closed 8 months ago

Adapt wasmTextToBinary for multi-value

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: lth, Assigned: lth)

References

Details

Attachments

(3 files, 3 obsolete files)

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.

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.

Attachment #9105629 - Attachment description: Bug 1592926 - Remove AstExprType, introduce AstBlockType. WIP → Bug 1592926 - Remove AstExprType, introduce AstBlockType.

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

Attachment #9106450 - Attachment description: Bug 1592926 - Store the module in the wasm parse context, don't pass it → Bug 1592926 - Store the module in the wasm parse context, don't pass it. r?wingo
Attachment #9105629 - Attachment description: Bug 1592926 - Remove AstExprType, introduce AstBlockType. → Bug 1592926 - Remove AstExprType, introduce AstBlockType. r?wingo
Attachment #9106451 - Attachment description: Bug 1592926 - Adjustments to existing multi-value code → Bug 1592926 - Adjustments to existing multi-value code. DO NOT LAND

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

Attachment #9106450 - Attachment description: Bug 1592926 - Store the module in the wasm parse context, don't pass it. r?wingo → Bug 1592926 - Store the module in the wasm parse context, don't pass it. r=wingo
Attachment #9105629 - Attachment description: Bug 1592926 - Remove AstExprType, introduce AstBlockType. r?wingo → Bug 1592926 - Remove AstExprType, introduce AstBlockType. r=wingo
Pushed by lhansen@mozilla.com:
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
Pushed by malexandru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1605a3463b56
Fix bustages in WasmTextToBinary.cpp and WasmAST.h a=bustage-fix CLOSED TREE

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.

Keywords: leave-open

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

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.

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.

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

Depends on D51780

Attachment #9106834 - Attachment description: Bug 1592926 - Encode function index in block type as sleb. r?wingo → Bug 1592926 - Encode function index in block type as sleb. r=wingo
Keywords: leave-open
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/57442cf2bb9d
Encode function index in block type as sleb. r=wingo
Attachment #9106843 - Attachment is obsolete: true
Attachment #9106451 - Attachment is obsolete: true
Attachment #9106813 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.