wasm: binary format 0xb

RESOLVED FIXED in Firefox 49

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sunfish, Assigned: luke)

Tracking

(Blocks: 1 bug)

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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
(Reporter)

Updated

2 years ago
Depends on: 1263205
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.
(Reporter)

Comment 2

2 years ago
https://github.com/WebAssembly/design/issues/658 is now tracking the br_table issue.
Created attachment 8743576 [details] [diff] [review]
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.
Attachment #8743576 - Flags: review?(bbouvier)
Comment on attachment 8743576 [details] [diff] [review]
wasm-experimental-version

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

Looks good!
Attachment #8743576 - Flags: review?(bbouvier) → review+
(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)
(Reporter)

Updated

2 years ago
Depends on: 1266449
Depends on: 1254142
Created attachment 8745068 [details] [diff] [review]
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
Assignee: nobody → luke
Status: NEW → ASSIGNED
Attachment #8745068 - Flags: review?(bbouvier)
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.
Attachment #8745068 - Flags: review?(bbouvier) → review+
(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 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2b4b2b927f55
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Duplicate of this bug: 1253572
You need to log in before you can comment on or make changes to this bug.