1259694 - wasm: recognize (i32.atomic.rmw.add (i32.const 0) (i32.const 0)) as a simple fence

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine: JIT, defect, P5)

Description

[Lars T Hansen [:lth]]

We will remove Atomics.fence but some asm.js code still uses Atomics.add(heap32,0,0) in ported C++ code to implement a fence.  That may or may not be semantically correct but it might still be useful to recognize that on x86 that can be reduced to just an MFENCE instruction and on ARM it is just a single DMB, not a DMB pair with a LL/SC loop within it.  No heap reference or range checking should be needed, and of course no actual addition.

For asm.js code, the size of heap32 is never zero so no range checking is needed, I think, and it's never necessary to check the type of the heap memory.

It's probably platform-dependent: on x64 it's likely the pattern already boils down to just a "LOCK; ADD [heapreg+0], 0" which is a fine fence as it is.

Comment 1

[Lars T Hansen [:lth]]

Apropos x64, in bug 1077027 we observe that XCHG is significantly faster than MOV; MFENCE for an atomic store on two very different x64 implementations, so we want to do some careful benchmarking.

Comment 2

[Lars T Hansen [:lth]]

According to the new(est) memory model this phrase is not a correct fence: there is no notion of fence in that model, only a model of event relationships, and no way to "express" a fence.

For the time being, in every implementation on all interesting hardware, the phrase will continue to work as a fence however.  That is because every implementation will implement atomics in the straightforward way with seq_cst fences as necessary.  It is more open to discussion what it will mean once implementations start optimizing atomics in earnest, but I think that won't happen until after WebAssembly has shared memory and atomics, and probably a memory model that does include fences, and it will probably share that model with JS.

Comment 3

[Jukka Jylänki]

Thanks, good update. We recently had a discussion about lockfree atomics performance and how to come up with meaningful benchmarks for that, and we're working on setting those up so that we'd be able to understand the impact of these the best possible way.

Comment 4

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 5

[Lars T Hansen [:lth]]

asm.js atomics are gone and if wasm gets some kind of fence semantics it will be through well-defined instructions for that purpose.