Closed Bug 1496297 Opened 3 years ago Closed 3 years ago

Allow skip-if in jit-tests

Categories

(Core :: JavaScript Engine, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: efaust, Assigned: efaust)

References

Details

Attachments

(23 files, 1 obsolete file)

5.95 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
40.03 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
18.49 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
23.27 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
32.59 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
63.33 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
46.43 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
2.89 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
6.16 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
14.99 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.49 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
853 bytes, patch
jorendorff
: review+
Details | Diff | Splinter Review
5.82 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
838 bytes, patch
jorendorff
: review+
Details | Diff | Splinter Review
2.02 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
8.34 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
5.28 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
2.12 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
5.87 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
13.93 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.39 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
4.41 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.63 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #9014251 - Flags: feedback?(jorendorff)
Summary: allow skip-if in jit-tests → Allow skip-if in jit-tests
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.
Attachment #9014251 - Flags: feedback?(jorendorff) → feedback+
Attachment #9014251 - Attachment is obsolete: true
Attachment #9014528 - Flags: review?(jorendorff)
Attachment #9014531 - Flags: review?(jorendorff)
Attachment #9014533 - Flags: review?(jorendorff)
Attachment #9014534 - Flags: review?(jorendorff)
Attachment #9014535 - Flags: review?(jorendorff)
Attachment #9014536 - Flags: review?(jorendorff)
Attachment #9014541 - Flags: review?(jorendorff)
Attachment #9014542 - Flags: review?(jorendorff)
Attachment #9014543 - Flags: review?(jorendorff)
Attachment #9014544 - Flags: review?(jorendorff)
Attachment #9014545 - Flags: review?(jorendorff)
Attachment #9014548 - Flags: review?(jorendorff)
Attachment #9014549 - Flags: review?(jorendorff)
Attachment #9014550 - Flags: review?(jorendorff)
Attachment #9014552 - Flags: review?(jorendorff)
Attachment #9014554 - Flags: review?(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.
Attachment #9014528 - Flags: review?(jorendorff) → review+
Attachment #9014529 - Flags: review?(jorendorff) → review+
Attachment #9014531 - Flags: review?(jorendorff) → review+
Attachment #9014533 - Flags: review?(jorendorff) → review+
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.
Attachment #9014534 - Flags: review?(jorendorff) → review+
Attachment #9014535 - Flags: review?(jorendorff) → review+
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.
Attachment #9014536 - Flags: review?(jorendorff) → review+
Attachment #9014538 - Flags: review?(jorendorff) → review+
Attachment #9014540 - Flags: review?(jorendorff) → review+
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.
Attachment #9014541 - Flags: review?(jorendorff) → review+
Attachment #9014542 - Flags: review?(jorendorff) → review+
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?
Attachment #9014543 - Flags: review?(jorendorff) → review+
Attachment #9014544 - Flags: review?(jorendorff) → review+
Attachment #9014545 - Flags: review?(jorendorff) → review+
Attachment #9014546 - Flags: review?(jorendorff) → review+
Attachment #9014547 - Flags: review?(jorendorff) → review+
Attachment #9014548 - Flags: review?(jorendorff) → review+
Attachment #9014549 - Flags: review?(jorendorff) → review+
Attachment #9014550 - Flags: review?(jorendorff) → review+
Attachment #9014551 - Flags: review?(jorendorff) → review+
Attachment #9014552 - Flags: review?(jorendorff) → review+
Attachment #9014553 - Flags: review?(jorendorff) → review+
Attachment #9014554 - Flags: review?(jorendorff) → review+
Priority: -- → P2
(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.
Pushed by efaustbmo@gmail.com:
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)
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.
That seems well worth looking into.
Flags: needinfo?(efaustbmo)
Depends on: 1497978
Flags: needinfo?(efaustbmo)
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?
You need to log in before you can comment on or make changes to this bug.