Closed
Bug 1316554
Opened 8 years ago
Closed 8 years ago
wasm: Finish passing 0xd spec tests
Categories
(Core :: JavaScript Engine: JIT, defect, P1)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: bbouvier, Assigned: bbouvier)
References
Details
Attachments
(4 files, 1 obsolete file)
21.85 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
3.75 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
10.49 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
6.42 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
- wait for spec repo update to address https://github.com/WebAssembly/spec/issues/367
- reimport tests (there have been a few changes)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bbouvier
Priority: P2 → P1
Assignee | ||
Comment 1•8 years ago
|
||
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)
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
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 5•8 years ago
|
||
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 6•8 years ago
|
||
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+
![]() |
||
Updated•8 years ago
|
Attachment #8812834 -
Flags: review?(luke) → review+
![]() |
||
Comment 7•8 years ago
|
||
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.
Assignee | ||
Comment 9•8 years ago
|
||
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 10•8 years ago
|
||
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+
Comment 11•8 years ago
|
||
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
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f978b398f898
https://hg.mozilla.org/mozilla-central/rev/e588418674e1
https://hg.mozilla.org/mozilla-central/rev/67ff8b138c46
https://hg.mozilla.org/mozilla-central/rev/5f23873d0c91
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 13•8 years ago
|
||
I think it is not the intention to backport this to aurora right? Can we mark status-firefox52 to wontfix?
Flags: needinfo?(bbouvier)
Assignee | ||
Comment 14•8 years ago
|
||
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.
Description
•