Closed Bug 1316554 Opened 8 years ago Closed 8 years ago

wasm: Finish passing 0xd spec tests

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- wontfix
firefox53 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(4 files, 1 obsolete file)

- wait for spec repo update to address https://github.com/WebAssembly/spec/issues/367 - 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 (https://github.com/WebAssembly/spec/pull/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 https://github.com/WebAssembly/spec/issues/372. 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 end call $fb end call $fa end 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] 1.rename-specjs-wastjs.patch Review of attachment 8812830 [details] [diff] [review]: ----------------------------------------------------------------- sgtm
Attachment #8812830 - Flags: review?(luke) → review+
Comment on attachment 8812833 [details] [diff] [review] 2.maybe-match-names.patch 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] 4.handsome-switch.patch 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.
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] switches.patch 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 bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f978b398f898 Rename spec interpreter to wast.js; r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/e588418674e1 wast parser: allow matching names at end of if/else/blocks; r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/67ff8b138c46 Reimport spec tests; r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/5f23873d0c91 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.

Attachment

General

Created:
Updated:
Size: