Closed Bug 1316554 Opened 4 years ago Closed 4 years ago

wasm: Finish passing 0xd spec tests


(Core :: JavaScript Engine: JIT, defect, P1)




Tracking Status
firefox52 --- wontfix
firefox53 --- fixed


(Reporter: bbouvier, Assigned: bbouvier)




(4 files, 1 obsolete file)

- wait for spec repo update to address
- reimport tests (there have been a few changes)
Assignee: nobody → bbouvier
Priority: P2 → P1
This is something I've been wanting for the last few weeks: this patch renames spec.js to wast.js, since it's really a wast interpreter written in JS.
Attachment #8812830 - Flags: review?(luke)
This allows names from block/loop/if-else ("start") position to be placed at the then/else ("end") positions, in a flexible way: you can either put a name at the start position, or at the end position, but if it's there at both positions, the two names must match.

Also allows > in names, so as to form names with arrows: "i64->f64" is now a valid name (and used in one spec test).
Attachment #8812833 - Flags: review?(luke)
This re-imports the spec tests, including the PR #383 ( which has not been merged yet. They're pretty stable now. Likely minor updates will be needed, but at lower frequencies, so I'm in favor of closing this bug (and meta-bug) once this is in, and add future spec tests along with the features (or use new bugs).
Attachment #8812834 - Flags: review?(luke)
Attached patch 4.handsome-switch.patch (obsolete) — Splinter Review
This makes switches look pretty, as suggested in It's full of heuristics, but it seemed to render well small test cases and for AngryBots too.

Instead of:

block $a
  block $b
    block $c
      get_local 0
      br_table $a $b $c
    call $fb
  call $fa

we now have:

block block block
  get_local 0
  br_table $a $b $c
end $c
  call $fb
end $b
  call $fa
end $a
Attachment #8812836 - Flags: review?(luke)
Comment on attachment 8812830 [details] [diff] [review]

Review of attachment 8812830 [details] [diff] [review]:

Attachment #8812830 - Flags: review?(luke) → review+
Comment on attachment 8812833 [details] [diff] [review]

Review of attachment 8812833 [details] [diff] [review]:

::: js/src/wasm/WasmTextToBinary.cpp
@@ +1564,5 @@
>      return true;
>  }
> +static bool
> +MaybeMatchName(WasmParseContext& c, AstName& name)

Since this parameter is mutated, can you pass by *?
Attachment #8812833 - Flags: review?(luke) → review+
Attachment #8812834 - Flags: review?(luke) → review+
Comment on attachment 8812836 [details] [diff] [review]

Review of attachment 8812836 [details] [diff] [review]:

Thanks for doing this, I think this will be a *huge* improvement.  The consensus reached on #372 was actually that the labels *must* go on both the top and bottom.  I'm fine accepting either-or (to make it convenient to write tests), but for output, we should emit both.  I was thinking in devtools we could apply a style that would make the first label a light grey for blocks.
Duplicate of this bug: 1316609
Attached patch switches.patchSplinter Review
Ha right, I wasn't sure what "option 3" was referring to, it's a bit hidden in the deep in the comments. Updated the patch to the following:

- when a "block" or "loop" has a name, it's always displayed at the start and "end", independently of the stacking taking effect or not. It will happen also for typed blocks, which is a bit unfortunate and ugly (`block $a block i32 $b block $c`); but looking at AngryBots.wasm, there are no non-void blocks (at all).
- simplified the patch to not need the global boolean in the context
Attachment #8812836 - Attachment is obsolete: true
Attachment #8812836 - Flags: review?(luke)
Attachment #8813163 - Flags: review?(luke)
Comment on attachment 8813163 [details] [diff] [review]

Review of attachment 8813163 [details] [diff] [review]:

Wow, that's a simpler solution than I was imagining; nice job!

::: js/src/jit-test/tests/wasm/full-cycle.js
@@ +120,5 @@
> +    wasmFullPass(`(module
> +        (func (export "run") (result i32) (param $p i32) (local $n i32)
> +            i32.const 0
> +            set_local $n
> +            block block block

On second though, could you *require* blocks to have labels at the top?  That could help us later on when sharing test cases.

::: js/src/jit-test/tests/wasm/to-text.js
@@ +265,5 @@
> +    (func (export "run") (param $p i32) (local $n i32)
> +        i32.const 0
> +        set_local $n
> +        loop $outer
> +            block block block loop $inner

Could you change the algo to only stack blocks inline, so that loops are always on their own line?  I think loops are inherently "interesting" and reflect real program structure, unlike blocks.
Attachment #8813163 - Flags: review?(luke) → review+
Pushed by
Rename spec interpreter to wast.js; r=luke
wast parser: allow matching names at end of if/else/blocks; r=luke
Reimport spec tests; r=luke
Render series of blocks as inline blocks; r=luke
I think it is not the intention to backport this to aurora right? Can we mark status-firefox52 to wontfix?
Flags: needinfo?(bbouvier)
Indeed: patches 1 2 and 3 are for testing purposes, and patch 4 is a binary-to-text enhancement that can be deferred to the next version.
Flags: needinfo?(bbouvier)
You need to log in before you can comment on or make changes to this bug.