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

RESOLVED FIXED in Firefox 52

Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: lth, Assigned: lth)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

Assignee

Description

3 years ago
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
Assignee

Comment 1

3 years ago
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.
Attachment #8804822 - Flags: review?(bbouvier)
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()?
Attachment #8804822 - Flags: review?(bbouvier) → review+
Assignee

Comment 3

3 years ago
(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.
Flags: needinfo?(choller)
Assignee

Comment 4

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

Updated

3 years ago
Keywords: leave-open
(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.
Flags: needinfo?(choller)
Assignee

Comment 7

3 years ago
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.
Depends on: 1277770
Assignee

Comment 8

3 years ago
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.
Attachment #8808577 - Flags: review?(bbouvier)
Assignee

Comment 9

3 years ago
Move directives from asm.js test cases to directives.txt.
Attachment #8808578 - Flags: review?(bbouvier)
Assignee

Comment 10

3 years ago
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?
Attachment #8808580 - Flags: feedback?(bbouvier)
Attachment #8808577 - Flags: review?(bbouvier) → review+
Comment on attachment 8808578 [details] [diff] [review]
bug1313114-asmjs-tests.patch

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

sweet
Attachment #8808578 - Flags: review?(bbouvier) → review+
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!
Attachment #8808580 - Flags: feedback?(bbouvier) → feedback+
Assignee

Comment 13

3 years ago
One-liner to fix the import script.
Attachment #8808686 - Flags: review?(bbouvier)
Assignee

Comment 14

3 years ago
Bulk change to introduce directives.txt files and remove redundant directives from the tests.
Attachment #8808580 - Attachment is obsolete: true
Attachment #8808687 - Flags: review?(bbouvier)
Attachment #8808686 - Flags: review?(bbouvier) → review+
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!
Attachment #8808687 - Flags: review?(bbouvier) → review+
Assignee

Comment 17

3 years ago
(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
(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.
Assignee

Comment 20

3 years ago
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
Assignee

Updated

3 years ago
Keywords: leave-open
Assignee

Comment 22

3 years ago
(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.
You need to log in before you can comment on or make changes to this bug.