Closed
Bug 1259694
Opened 8 years ago
Closed 6 years ago
wasm: recognize (i32.atomic.rmw.add (i32.const 0) (i32.const 0)) as a simple fence
Categories
(Core :: JavaScript Engine: JIT, defect, P5)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: lth, Unassigned)
References
(Blocks 1 open bug)
Details
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.
Reporter | ||
Comment 1•8 years ago
|
||
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.
Updated•8 years ago
|
Flags: needinfo?(lhansen)
Updated•8 years ago
|
Priority: -- → P5
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(lhansen)
Reporter | ||
Comment 2•8 years ago
|
||
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•8 years ago
|
||
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.
Reporter | ||
Updated•8 years ago
|
No longer blocks: shared-array-buffer
Comment 4•6 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INACTIVE
Updated•6 years ago
|
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---
Reporter | ||
Comment 5•6 years ago
|
||
asm.js atomics are gone and if wasm gets some kind of fence semantics it will be through well-defined instructions for that purpose.
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → WONTFIX
Summary: asm.js: Recognize Atomics.add(heap, 0, 0) as a simple fence → wasm: recognize (i32.atomic.rmw.add (i32.const 0) (i32.const 0)) as a simple fence
You need to log in
before you can comment on or make changes to this bug.
Description
•