1313114 - Always run all asm.js and wasm tests with the baseline compiler

Status: RESOLVED FIXED

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

Description

[Lars T Hansen [:lth]]

Right now we run *almost* all tests with the baseline compiler, but there's at least one that is not run (x64 debug build, with my patch for bug 1313043 applied):

jit-test$ ./jit_test.py ../build-debug/dist/bin/js --args="--wasm-always-baseline" wasm asm.js
...
Assertion failure: func_.callSiteLineNums().length() == lastReadCallSite_, at /Users/lhansen/moz/mozilla-inbound/js/src/asmjs/WasmBaselineCompile.cpp:7433
Exit code: -11
FAIL - asm.js/testBug1291887.js

We should:

(a) fix the bug above, and
(b) change the test runner so that it always runs the wasm baseline compiler when it runs an asm.js test or a wasm test

Comment 1

[Lars T Hansen [:lth]]

Attachment: bug1313114-read-all-the-bits.patch

Another Belgian situation, I think...

We need to be sure to read everything that needs to be read before doing nothing if the code is dead.  So order things properly.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8804822 [details] [diff] [review]
bug1313114-read-all-the-bits.patch

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

Indeed; had to do the exact same change in the rendering phase recently. Is the deadCode_ flag always equal to f.iter().inUnreachableCode()?

Comment 3

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #2)

> Is the deadCode_ flag always equal to f.iter().inUnreachableCode()?

I don't know, I believe the latter method did not exist when the baseline compiler was written.  I'll try to investigate.

:decoder, it will be useful for the fuzzers to ensure that every possible instruction is always present in some dead-code position, so that we can ferret out these problems.

Comment 4

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/642bd2b0839fe64e22d5494a951a0d42a637b8da
Bug 1313114 - order bytecode consumption and deadCode_ skipping properly.  r=bbouvier

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/642bd2b0839f

Comment 6

[Christian Holler (:decoder)]

(In reply to Lars T Hansen [:lth] from comment #3)
> 
> :decoder, it will be useful for the fuzzers to ensure that every possible
> instruction is always present in some dead-code position, so that we can
> ferret out these problems.

With the current fuzzing, we have no influence on that. But the guided fuzzing will surely hit these cases from time to time.

Comment 7

[Lars T Hansen [:lth]]

If we had a per-directory directive file (bug 1277770) fixing part (b) above would be a no-brainer: we could remove the test-also-wasm-baseline directive from individual files in jit-test/tests/wasm and jit-test/tests/asm.js and put it in the directive file instead.

Comment 8

[Lars T Hansen [:lth]]

Attachment: bug1313114-remove-hack.patch

The fix in bug 1277770 will make it possible to just add testing directives to directories and we don't need this sniffing hack any more (in my opinion).  Also see subsequent patches on this bug.

Comment 9

[Lars T Hansen [:lth]]

Attachment: bug1313114-asmjs-tests.patch

Move directives from asm.js test cases to directives.txt.

Comment 10

[Lars T Hansen [:lth]]

Attachment: bug1313114-wasm-tests-sample.patch

This just moves the directives from basic.js to directives.txt, to illustrate how it would be done here.  I would have to go through all the other files too.

The reason I'm asking for feedback and not review is that I suspect there is some functionality in a test importer script somewhere that adds the directives to the spec tests when those are imported.  That should probably be updated too.  Where might one find this script?

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8808578 [details] [diff] [review]
bug1313114-asmjs-tests.patch

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

sweet

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8808580 [details] [diff] [review]
bug1313114-wasm-tests-sample.patch

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

One might find this script in jit-tests/wasm/spec/import_test.sh. Thanks for doing this!

Comment 13

[Lars T Hansen [:lth]]

Attachment: bug1313114-fix-import-script.patch

One-liner to fix the import script.

Comment 14

[Lars T Hansen [:lth]]

Attachment: bug1313114-bulk-directive-change.patch

Bulk change to introduce directives.txt files and remove redundant directives from the tests.

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8808687 [details] [diff] [review]
bug1313114-bulk-directive-change.patch

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

Don't you need a directives.txt file in the spec/ subdir too? r=me with that. Thanks!

Comment 16

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eceabad88234

Comment 17

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #15)
> Comment on attachment 8808687 [details] [diff] [review]
> bug1313114-bulk-directive-change.patch
> 
> Review of attachment 8808687 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Don't you need a directives.txt file in the spec/ subdir too?

The perils of forgetting 'hg add'.  Thanks!

A couple more adjustments are needed here:

- there's one asm.js test that should only run if Odin is enabled, we can point-fix that with a test
  inside the test without complicating things too much

- I need to strip the directives from files in tests/wasm/regress, and to add directives.txt there

Comment 18

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Lars T Hansen [:lth] from comment #17)
> A couple more adjustments are needed here:
> 
> - there's one asm.js test that should only run if Odin is enabled, we can
> point-fix that with a test
>   inside the test without complicating things too much
> 
> - I need to strip the directives from files in tests/wasm/regress, and to
> add directives.txt there

This all sounds great, thanks.

Comment 19

[Lars T Hansen [:lth]]

Try run with this and other patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=193cdd4713be7528e5de69c0439c3213cb05116d

Comment 20

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2fc79743f58e1b73655879551448e0152a122d19
Bug 1313114 - remove hack for running wasm baseline tests when sniffing asm.js content from 'noasmjs'. r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/6de2be470ac6f7a4d24113b54b99b0c299e9397f
Bug 1313114 - move test-also-wasm-baseline and test-also-noasmjs directives to directives.txt. r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/f29bb9b8ad4ec902cdd56fbef54fa6306072dc88
Bug 1313114 - Make the wasm spec import script omit the baseline directive. r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/0675eb15b778409550d77a77324695a7350d6586
Bug 1313114 - bulk change to remove test-also-wasm-baseline directives; add directives.txt files. r=bbouvier

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2fc79743f58e
https://hg.mozilla.org/mozilla-central/rev/6de2be470ac6
https://hg.mozilla.org/mozilla-central/rev/f29bb9b8ad4e
https://hg.mozilla.org/mozilla-central/rev/0675eb15b778

Comment 22

[Lars T Hansen [:lth]]

(In reply to Lars T Hansen [:lth] from comment #3)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> 
> > Is the deadCode_ flag always equal to f.iter().inUnreachableCode()?
> 
> I don't know, I believe the latter method did not exist when the baseline
> compiler was written.  I'll try to investigate.

It seems to have gone away again.