Closed Bug 1263202 Opened 4 years ago Closed 3 years ago

wasm: binary format 0xb

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: sunfish, Assigned: luke)

References

Details

Attachments

(2 files)

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
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.
https://github.com/WebAssembly/design/issues/658 is now tracking the br_table issue.
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)
Depends on: 1266449
Depends on: 1254142
Attached patch aritiesSplinter Review
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.
https://hg.mozilla.org/mozilla-central/rev/2b4b2b927f55
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
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.