Closed Bug 1515623 Opened 3 years ago Closed 3 years ago

Several caching jit-test failures with --no-wasm-ion

Categories

(Core :: Javascript: WebAssembly, enhancement, P2)

x86_64
Linux
enhancement

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: lth, Assigned: luke)

References

Details

Attachments

(1 file, 2 obsolete files)

Silly me, I had figured the build bot would run both --no-wasm-ion and --no-wasm-baseline to ensure coverage.  No such luck.

The tests that fail are:

wasm/caching.js
wasm/bench/wasm_box2d.js
wasm/gc/anyref-global-object.js
wasm/gc/anyref.js
wasm/gc/tables-generalized.js

The GC tests I'm not too worried about but the other two are worrisome, especially if they've been around for a while and are now on beta.  I'll investigate more deeply and bisect.
The problem with the caching test is that cache.cached is set to true by compileStreaming even though wasmCachingIsSupported() is false.
Flags: needinfo?(luke)
wasm_box2d fails for the same reason as caching.js.
For the other three the problem lay between the keyboard and the chair.
Summary: Several wasm jit-test failures with --no-wasm-ion → Several caching jit-test failures with --no-wasm-ion
So the failure, in both caching.js and wasm_box2d.js, is that --test-wasm-await-tier2 overrides --no-wasm-ion in CompilerEnvironment::computeParameters(), forcing on ion, which means wasm::HasCachingSupport(cx) is pessimistic in saying that caching is not supported if --no-wasm-ion, because with --test-wasm-await-tier2, caching does in fact happen at compile-time.

Rather than adding a special case to HasCachingSupport(), I'm inclined to just remove the "|| argTestTiering" from the computation of both baselineEnabled and ionEnabled in CompilerEnvironment::computeParameters().  Doing that locally passes --tbpl.
Flags: needinfo?(luke)
Attached patch tweak-wasm-await-tier2 (obsolete) — Splinter Review
Attachment #9032702 - Flags: review?(lhansen)
Comment on attachment 9032702 [details] [diff] [review]
tweak-wasm-await-tier2

Patch is empty?

I agree this code is a mess, and the TestingFunctions support is not well synchronized with it.  Benjamin is trying to clean it up over in bug 1509441, it may be useful to recast the fix for the present problem in terms of the rewrite there.
Attachment #9032702 - Flags: review?(lhansen)
See Also: → 1509441
Assignee: lhansen → luke
Severity: major → normal
Priority: P1 → P2
Attached patch tweak-wasm-await-tier2 (obsolete) — Splinter Review
Attachment #9032956 - Flags: review?(lhansen)
Oops, the first patch didn't enough, the second patch had too much; this patch is just right.
Attachment #9032702 - Attachment is obsolete: true
Attachment #9032956 - Attachment is obsolete: true
Attachment #9032956 - Flags: review?(lhansen)
Attachment #9032957 - Flags: review?(lhansen)
Comment on attachment 9032957 [details] [diff] [review]
tweak-wasm-await-tier2

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

WFM though it'll be ugly for Benjamin to merge after this one :)
Attachment #9032957 - Flags: review?(lhansen) → review+
Seems like there's no rush, so it can wait.
Depends on: 1509441
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6d74968b274
Make wasm::HasCachingSupport(cx) accurately represent testing flags (r=lth)
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.