1263202 - wasm: binary format 0xb

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Dan Gohman [:sunfish]]

The 0xb version of the binary format is currently proposed to include a switch to postorder, and probably a few other tweaks as well:

Design repo 0xb branch pull requests:

https://github.com/WebAssembly/design/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Aopen+base%3Abinary_0xb

Design repo 0xb branch:

https://github.com/WebAssembly/design/tree/binary_0xb

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

It's unclear to me after looking at the updated design repo: in this version, can br_table yield a sub-expression to the enclosing label? Current design repo says so, but it wasn't officially in the binary format, the updated design repo says so too, but there might be a discrepancy here.

Comment 2

[Dan Gohman [:sunfish]]

https://github.com/WebAssembly/design/issues/658 is now tracking the br_table issue.

Comment 3

[Luke Wagner [:luke]]

Attachment: wasm-experimental-version

This adds a Wasm.experimentalVersion property which will be used by the demo to pick the right binary bits to serve.  This property will be removed once the binary format is stable; definitely before release.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8743576 [details] [diff] [review]
wasm-experimental-version

Review of attachment 8743576 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

(Thinking about it, it should probably be marked as JSPROP_READONLY and JSPROP_PERMANENT, but as this is meant to be temporary, this is fine as si)

Comment 6

[Luke Wagner [:luke]]

Attachment: arities

This patch adds arities to br/br_if/br_table/call/call_indirect/call_import/return as described in https://github.com/WebAssembly/design/pull/595

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8745068 [details] [diff] [review]
arities

Review of attachment 8745068 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thank you for the patch!

::: js/src/asmjs/AsmJS.cpp
@@ +2698,5 @@
>      bool writeBr(uint32_t absolute, Expr expr = Expr::Br) {
>          MOZ_ASSERT(expr == Expr::Br || expr == Expr::BrIf);
>          MOZ_ASSERT(absolute < blockDepth_);
>          return encoder().writeExpr(expr) &&
> +               encoder().writeVarU32(0) &&

Can you specify somewhere that it's the arity of the break?

@@ +4398,5 @@
>      if (!f.writeCall(callNode, Expr::Call))
>          return false;
>  
> +    // Call arity
> +    if (!f.encoder().writeVarU32(arity))

Can inline arity here directly, instead of using a variable?

@@ +4487,5 @@
>      if (!f.writeCall(callNode, Expr::CallIndirect))
>          return false;
>  
> +    // Call arity
> +    if (!f.encoder().writeVarU32(arity))

ditto

@@ +4533,5 @@
>      if (!f.writeCall(callNode, Expr::CallImport))
>          return false;
>  
> +    // Call arity
> +    if (!f.encoder().writeVarU32(arity))

ditto

@@ +6434,5 @@
>      // Start the br_table block.
>      if (!f.encoder().writeExpr(Expr::BrTable))
>          return false;
>  
> +    // asm.js switches cannot express a branch value

Can you add the word "arity" maybe at the start of this comment?

::: js/src/asmjs/WasmBinaryIterator.h
@@ +1384,5 @@
>          return fail("unable to read br depth");
>  
>      TypeAndValue<Value> tv;
> +    if (arity) {
> +        if (!pop(&tv))

See question below (I think this should be either a `top` here, or two `pop` below)

@@ +1422,3 @@
>      TypeAndValue<Value> tv;
> +    if (arity) {
> +        if (!top(&tv))

Why are we using `top` here (and in readBrTable) while readBr does a `pop` at the same location? Is that a bug, or is that br_if/br_table's value is their result but br has no result? The current AstSemantics.md doc says that br *and* br_if don't yield values, doesn't mention br_table, but it is probably outdated.

Comment 8

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> Can inline arity here directly, instead of using a variable?
> ditto
> ditto

Unfortunately no, since the 'args' and 'sig' have already been Move()d.

> Why are we using `top` here (and in readBrTable) while readBr does a `pop`
> at the same location? Is that a bug, or is that br_if/br_table's value is
> their result but br has no result? The current AstSemantics.md doc says that
> br *and* br_if don't yield values, doesn't mention br_table, but it is
> probably outdated.

'br_if' and 'br_table' propagate their value operand to the caller if they don't branch and so the top() is propagating.  'br' always branches and so does not propagate anything to the caller and so replaces the top value with AnyType.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b4b2b927f55

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2b4b2b927f55