Allow skip-if in jit-tests

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P2
normal
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(23 attachments, 1 obsolete attachment)

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
Assignee

Description

9 months ago
No description provided.
Assignee

Comment 1

9 months ago
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #9014251 - Flags: feedback?(jorendorff)
Assignee

Updated

9 months ago
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+
Assignee

Comment 3

9 months ago
Attachment #9014251 - Attachment is obsolete: true
Attachment #9014528 - Flags: review?(jorendorff)
Assignee

Comment 5

9 months ago
Attachment #9014531 - Flags: review?(jorendorff)
Assignee

Comment 6

9 months ago
Attachment #9014533 - Flags: review?(jorendorff)
Assignee

Comment 7

9 months ago
Attachment #9014534 - Flags: review?(jorendorff)
Assignee

Comment 8

9 months ago
Attachment #9014535 - Flags: review?(jorendorff)
Assignee

Comment 9

9 months ago
Attachment #9014536 - Flags: review?(jorendorff)
Assignee

Comment 12

9 months ago
Attachment #9014541 - Flags: review?(jorendorff)
Assignee

Comment 13

9 months ago
Attachment #9014542 - Flags: review?(jorendorff)
Assignee

Comment 14

9 months ago
Attachment #9014543 - Flags: review?(jorendorff)
Assignee

Comment 16

9 months ago
Attachment #9014545 - Flags: review?(jorendorff)
Assignee

Comment 19

9 months ago
Attachment #9014548 - Flags: review?(jorendorff)
Assignee

Updated

9 months ago
Attachment #9014549 - Flags: review?(jorendorff)
Assignee

Comment 23

9 months ago
Attachment #9014552 - Flags: review?(jorendorff)
Assignee

Comment 25

9 months ago
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
Assignee

Comment 32

9 months ago
(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

9 months ago
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.
Assignee

Comment 36

9 months ago
That seems well worth looking into.
Flags: needinfo?(efaustbmo)
Assignee

Updated

9 months ago
Depends on: 1497978
Flags: needinfo?(efaustbmo)

Comment 37

9 months ago
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?

Updated

9 months ago
See Also: → arm64-baseline-tests
You need to log in before you can comment on or make changes to this bug.