1496297 - Allow skip-if in jit-tests

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Comment 1

[Eric Faust [:efaust]]

Attachment: [Part 1] Allow skip-if in jit-tests

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 9014251 [details] [diff] [review]
[Part 1] Allow skip-if in jit-tests

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

This is basically exactly what I had.

Comment 3

[Eric Faust [:efaust]]

Attachment: Implement |jit-test| skip-if: cond

Comment 4

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/TypedObject

Comment 5

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/basic

Comment 6

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/ion

Comment 7

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/wasm

Comment 8

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/gc

Comment 9

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/debug

Comment 10

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/baseline

Comment 11

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/auto-regress

Comment 12

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/asm.js

Comment 13

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/regexp

Comment 14

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/latin1

Comment 15

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/atomics

Comment 16

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/realms

Comment 17

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/saved-stacks

Comment 18

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/sharedbuf

Comment 19

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/parser

Comment 20

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/profiler

Comment 21

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/modules

Comment 22

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/regexp_parse

Comment 23

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/xdr

Comment 24

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/heap-analysis

Comment 25

[Eric Faust [:efaust]]

Attachment: Use |jit-test| skip-if in jit-test/tests/

Comment 26

[Eric Faust [:efaust]]

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=203372235&revision=e43c3a2e2ccad71471f004e736d920421c243ea6 for posterity.

Comment 27

[Jason Orendorff [:jorendorff]]

Comment on attachment 9014528 [details] [diff] [review]
Implement |jit-test| skip-if: cond

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

\o/

::: js/src/tests/lib/jittests.py
@@ +199,5 @@
>          # For each list of jit flags, make a copy of the test.
>          return [self.copy_and_extend_jitflags(v) for v in variants]
>  
>      COOKIE = '|jit-test|'
> +    SKIPPED_RETURN = 59

SKIPPED_EXIT_STATUS please.

Comment 28

[Jason Orendorff [:jorendorff]]

Comment on attachment 9014534 [details] [diff] [review]
Use |jit-test| skip-if in jit-test/tests/wasm

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

::: js/src/jit-test/tests/wasm/timeout/1.js
@@ +2,3 @@
>  
>  // Don't include wasm.js in timeout tests: when wasm isn't supported, it will
>  // quit(0) which will cause the test to fail.

Is this comment still correct? Same story in 2.js, debug-interrupt-1.js, etc.

Comment 29

[Jason Orendorff [:jorendorff]]

Comment on attachment 9014536 [details] [diff] [review]
Use |jit-test| skip-if in jit-test/tests/debug

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

::: js/src/jit-test/tests/debug/wasm-12.js
@@ +5,1 @@
>  

Extreme style nit: remove the extra blank line here, please.

Comment 30

[Jason Orendorff [:jorendorff]]

Comment on attachment 9014541 [details] [diff] [review]
Use |jit-test| skip-if in jit-test/tests/asm.js

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

::: js/src/jit-test/tests/asm.js/testZOOB.js
@@ +1,1 @@
> +// |jit-test|; skip-if: !isAsmJSCompilationAvailable()

Nit: Stray semicolon here.

Comment 31

[Jason Orendorff [:jorendorff]]

Comment on attachment 9014543 [details] [diff] [review]
Use |jit-test| skip-if in jit-test/tests/latin1

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

r=me with the confusion addressed one way or the other...

::: js/src/jit-test/tests/latin1/asm.js
@@ +7,1 @@
>      quit();

Little confused about this one. Is this right? Should it throw if caching wasn't enabled?

Comment 32

[Eric Faust [:efaust]]

(In reply to Jason Orendorff [:jorendorff] from comment #31)
> Comment on attachment 9014543 [details] [diff] [review]
> Use |jit-test| skip-if in jit-test/tests/latin1
> 
> Review of attachment 9014543 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the confusion addressed one way or the other...
> 
> ::: js/src/jit-test/tests/latin1/asm.js
> @@ +7,1 @@
> >      quit();
> 
> Little confused about this one. Is this right? Should it throw if caching
> wasn't enabled?

Seems right. setCacheEnabled(true) sets the "I want to have this enabled" bool, but isCachingEnabled() checks whether there's a cachepath as well. Seems like we need both, and that skipping peacefully is fine.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a7a0b6b2bbf
Implement skip-if in jit-tests. (r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/012e94d3d96c
Use |jit-test| skip-if as appropriate in jit-test/tests/TypedObject/ (r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdf9b230e270
Use |jit-test| skip-if as appropriate in jit-test/tests/basic/ (r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b97fa179df85
Use |jit-test| skip-if as appropriate in jit-test/tests/ion/ (r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b29d20f0309
Use |jit-test| skip-if as appropriate to jit-test/tests/wasm/ (r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/4157315ccd54
Use |jit-test| skip-if as appropriate in jit-test/tests/gc/ (r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb0c1944852b
Use |jit-test| skip-if as appropriate in jit-test/tests/debug/ (r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb27fccdb19c
Use |jit-test| skip-if as appropriate in jit-test/tests/baseline/ (r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5140037f3a17
Use |jit-test| skip-if as appropriate in jit-test/tests/auto-regress/ (r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b95948e9a126
Use |jit-test| skip-if as appropriate in jite-test/tests/asm.js/ (r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b8297687615
Use |jit-test| skip-if as appropriate in jit-test/tests/regexp/ (r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/9da32b3fe705
Use |jit-test| skip-if as appropriate in jit-test/tests/latin1/ (r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6112cec4211f
Use |jit-test| skip-if as appropriate in jit-test/tests/atomics/ (r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6a61c3b3dbb
Use |jit-test| skip-if as appropriate in jit-test/tests/realms/ (r=jornedorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/085483ce64fe
Use |jit-test| skip-if as appropriate in jit-test/tests/saved-stacks/ (r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca4e8e9425ae
Use |jit-test| skip-if as appropriate in jit-test/tests/sharedbuf/ (r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fb70553886a
Use |jit-test| skip-if as appropriate in jit-test/tests/parser/ (r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/178e8e65a1a3
Use |jit-test| skip-if as appropriate in jit-test/tests/profiler/ (r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/4da5afa3f427
Use |jit-test| skip-if as appropriate in jit-test/tests/modules/ (r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/222b9bd838ec
Use |jit-test| skip-if as appropriate in jit-test/tests/regexp_parse/ (r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/de0ab3ea1796
Use |jit-test| skip-if as appropriate in jit-test/tests/xdr/ (r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/179b81691182
Use |jit-test| skip-if as appropriate in jit-test/tests/heap-analysis/ (r=jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8b420c49253
Use |jit-test| skip-if as appropriate in jit-test/tests/ (r=jorendorff)

Comment 34

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/2a7a0b6b2bbf
https://hg.mozilla.org/mozilla-central/rev/012e94d3d96c
https://hg.mozilla.org/mozilla-central/rev/fdf9b230e270
https://hg.mozilla.org/mozilla-central/rev/b97fa179df85
https://hg.mozilla.org/mozilla-central/rev/1b29d20f0309
https://hg.mozilla.org/mozilla-central/rev/4157315ccd54
https://hg.mozilla.org/mozilla-central/rev/cb0c1944852b
https://hg.mozilla.org/mozilla-central/rev/bb27fccdb19c
https://hg.mozilla.org/mozilla-central/rev/5140037f3a17
https://hg.mozilla.org/mozilla-central/rev/b95948e9a126
https://hg.mozilla.org/mozilla-central/rev/0b8297687615
https://hg.mozilla.org/mozilla-central/rev/9da32b3fe705
https://hg.mozilla.org/mozilla-central/rev/6112cec4211f
https://hg.mozilla.org/mozilla-central/rev/f6a61c3b3dbb
https://hg.mozilla.org/mozilla-central/rev/085483ce64fe
https://hg.mozilla.org/mozilla-central/rev/ca4e8e9425ae
https://hg.mozilla.org/mozilla-central/rev/0fb70553886a
https://hg.mozilla.org/mozilla-central/rev/178e8e65a1a3
https://hg.mozilla.org/mozilla-central/rev/4da5afa3f427
https://hg.mozilla.org/mozilla-central/rev/222b9bd838ec
https://hg.mozilla.org/mozilla-central/rev/de0ab3ea1796
https://hg.mozilla.org/mozilla-central/rev/179b81691182
https://hg.mozilla.org/mozilla-central/rev/c8b420c49253

Comment 35

[Jon Coppeard (:jonco)]

I'm getting a warning locally running jit-tests:

/home/jon/clone/modules/js/src/jit-test/tests/gc/bug-1155455.js: warning: unrecognized |jit-test| attribute skip-if !('gczeal' in this)

I don't know why this is happening for this test in particular.

Comment 36

[Eric Faust [:efaust]]

That seems well worth looking into.

Comment 37

[Bob Clary [:bc]]

This turned all of the tier3 jit tests orange on the pixel2 aarch64 tests:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&tier=1,2,3&searchStr=android-hw&group_state=expanded&revision=c8b420c49253532689f462813d7c9673ba798165

Was that expected? We previously had perma orange for jit{5,6,10} which prevented the tests from moving up to tier 2. Now, it is not apparent these tests are worth running in this state. Should I file a follow up bug for these new errors or to just disable the jit tests on pixel2 aarch64 altogether?