Flip flags to enable Cranelift for Arm64 wasm optimizing tier
Categories
(Core :: JavaScript: WebAssembly, task, P1)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox82 | --- | fixed |
People
(Reporter: lth, Assigned: bbouvier)
References
Details
Attachments
(3 files)
Effectively, we want --wasm-compiler=baseline+cranelift on this platform for both shell and browser. I'm not sure if anything else is needed; if we've done our jobs previously it should just work.
Benjamin, are you able to take a look at this?
|
Assignee | |
Comment 1•3 years ago
|
||
Yes, agreed with your assessment. Will check it works as intended too.
|
|
Assignee | |
Comment 2•3 years ago
|
||
WIP
|
|
Assignee | |
Comment 3•3 years ago
|
||
|
||
Updated•3 years ago
|
|
|
||
Updated•3 years ago
|
|
|
Assignee | |
Comment 4•3 years ago
•
|
||
A piece of news here: JS engine initialization creates JIT code for atomic ops (including flushing the icache for this JITted code). That may happen on a non-main thread, so because of the current patch, we think the icache flushing needs to be broadcast to all the threads, while we never checked for the membarrier availability before, and in my case that happens to fail (kernel too old).
The proper fix might be to add another parameter to EnsureIAndDCacheCoherency so it knows whether the icache invalidation must be broadcast or not. Then wasm compilation can set this parameter to "please broadcast the invalidation" since the code may be run on other threads, while other use cases don't have to do the heavyweight membarrier for their own usage.
|
Reporter | |
Comment 5•3 years ago
|
||
Not completely clear to me why icache invalidation is required there. The memory used should not previously have been used for code anywhere in the process. Of course, if we can do it then that's safer, but I'm unconvinced that heroics are necessary.
|
|
Assignee | |
Comment 6•3 years ago
|
||
(Sorry, i commented in the wrong bug, will continue discussion in bug 1661016.)
|
|
Assignee | |
Comment 7•3 years ago
|
||
The jittests now take too long to run on aarch64 hardware, so we'll need to bump the timeout limit there. I will upload a patch that does just that, and we can decide whether it's fine in the long run, or if we want to open a bug to lower the timeout limit back, in the future, once Cranelift compile times are a bit lower than what they are right now.
|
|
Reporter | |
Comment 8•3 years ago
|
||
BTW, what are we doing for the jittest flags? We have test-also=--wasm-compiler=ion test-also=--wasm-compiler=baseline, are we upgrading that? Should this be test-also=--wasm-compiler=optimizing test-also=--wasm-compiler=baseline instead?
|
|
Assignee | |
Comment 9•3 years ago
|
||
The new Cranelift compiler for WebAssembly makes jittest run a bit slower, so
raise the timeout value for those tests, until performance enhancements make it
run faster again.
Depends on D88396
|
|
Assignee | |
Comment 10•3 years ago
|
||
BTW, what are we doing for the jittest flags? We have test-also=--wasm-compiler=ion test-also=--wasm-compiler=baseline, are we upgrading that? Should this be test-also=--wasm-compiler=optimizing test-also=--wasm-compiler=baseline instead?
Good point, we should probably do this. Since the Android on automation doesn't have the membarrier, running the jit tests suite is equivalent to running with --wasm-compiler=cranelift. I'll file a follow up bug to change --wasm-compiler=ion to optimized (to remain consistent with Tier modes).
|
|
||
Updated•3 years ago
|
|
||
Comment 11•3 years ago
|
||
Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ba1939f5873a Use Cranelift by default for wasm compilation on aarch64; r=lth https://hg.mozilla.org/integration/autoland/rev/b612de011524 Raise the timeout limit for jittests on Android hardware; r=bc
|
||
Comment 12•3 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/ba1939f5873a
https://hg.mozilla.org/mozilla-central/rev/b612de011524
Description
•