Closed Bug 1660944 Opened 3 months ago Closed 3 months ago

Flip flags to enable Cranelift for Arm64 wasm optimizing tier

Categories

(Core :: Javascript: WebAssembly, task, P1)

ARM64
All
task

Tracking

()

RESOLVED FIXED
82 Branch
Tracking Status
firefox82 --- fixed

People

(Reporter: lth, Assigned: bbouvier)

References

(Blocks 1 open bug)

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?

Flags: needinfo?(bbouvier)

Yes, agreed with your assessment. Will check it works as intended too.

Flags: needinfo?(bbouvier)
Attached patch flip.patchSplinter Review

WIP

Depends on: 1661016
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Depends on: 1661723
Attachment #9172353 - Attachment description: Bug 1660944: Use Cranelift by default for wasm compilation on aarch64; r?lth → Bug 1660944: Use Cranelift by default for wasm compilation on aarch64; r=lth

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.

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.

(Sorry, i commented in the wrong bug, will continue discussion in bug 1661016.)

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.

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?

Flags: needinfo?(bbouvier)

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

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).

Flags: needinfo?(bbouvier)
Attachment #9173332 - Attachment description: Bug 1660944: Raise the timeout limit for jittests on Android hardware; r?jmaher,bc → Bug 1660944: Raise the timeout limit for jittests on Android hardware; r=bc
Blocks: 1662499
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
Regressions: 1662668
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch
Blocks: 1663898
Regressions: 1665028
You need to log in before you can comment on or make changes to this bug.