Skip to content

Commit f77f1f1

Browse files
tlivelytlively
authored
Fix atomic ordering (#8384)
EffectAnalyzer previously considered atomic operations to be ordered only with operations that read or write Wasm memories. But they should be ordered with all operations that read or write the machine's memory, which includes things like globals, tables, and the GC heap. Fix the problem. To avoid regressions on MVP programs that want to be able to freely reorder instructions like global.get and memory.grow (which is atomic, preventing such reordering), update the effects of all atomic instructions to be atomic only when operating on shared memory, including shared heap types. This is valid because the ordering of atomic operations on unshared locations can never synchronize with or be observed by operations on other threads.
1 parent ec79a6b commit f77f1f1

6 files changed

Lines changed: 140 additions & 74 deletions

File tree

‎src/ir/effects.h‎

Lines changed: 27 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,9 @@ class EffectAnalyzer {
219219
return mutableGlobalsRead.size() || readsMemory || readsTable ||
220220
readsMutableStruct || readsArray || isAtomic || calls;
221221
}
222+
bool accessesGlobalState() const {
223+
return readsMutableGlobalState() || writesGlobalState();
224+
}
222225

223226
bool hasNonTrapSideEffects() const {
224227
return localsWritten.size() > 0 || danglingPop || writesGlobalState() ||
@@ -305,10 +308,10 @@ class EffectAnalyzer {
305308
(danglingPop || other.danglingPop)) {
306309
return true;
307310
}
308-
// All atomics are sequentially consistent for now, and ordered wrt other
309-
// memory references.
310-
if ((isAtomic && other.accessesMemory()) ||
311-
(other.isAtomic && accessesMemory())) {
311+
// We model all atomics as sequentially consistent for now. They are ordered
312+
// wrt other loads and stores from anything, not just memories.
313+
if ((isAtomic && other.accessesGlobalState()) ||
314+
(other.isAtomic && accessesGlobalState())) {
312315
return true;
313316
}
314317
for (auto local : localsWritten) {
@@ -611,24 +614,26 @@ class EffectAnalyzer {
611614
}
612615
void visitLoad(Load* curr) {
613616
parent.readsMemory = true;
614-
parent.isAtomic |= curr->isAtomic();
617+
parent.isAtomic |=
618+
curr->isAtomic() && parent.module.getMemory(curr->memory)->shared;
615619
parent.implicitTrap = true;
616620
}
617621
void visitStore(Store* curr) {
618622
parent.writesMemory = true;
619-
parent.isAtomic |= curr->isAtomic();
623+
parent.isAtomic |=
624+
curr->isAtomic() && parent.module.getMemory(curr->memory)->shared;
620625
parent.implicitTrap = true;
621626
}
622627
void visitAtomicRMW(AtomicRMW* curr) {
623628
parent.readsMemory = true;
624629
parent.writesMemory = true;
625-
parent.isAtomic = true;
630+
parent.isAtomic |= parent.module.getMemory(curr->memory)->shared;
626631
parent.implicitTrap = true;
627632
}
628633
void visitAtomicCmpxchg(AtomicCmpxchg* curr) {
629634
parent.readsMemory = true;
630635
parent.writesMemory = true;
631-
parent.isAtomic = true;
636+
parent.isAtomic |= parent.module.getMemory(curr->memory)->shared;
632637
parent.implicitTrap = true;
633638
}
634639
void visitAtomicWait(AtomicWait* curr) {
@@ -754,8 +759,9 @@ class EffectAnalyzer {
754759
// memory.size accesses the size of the memory, and thus can be modeled as
755760
// reading memory
756761
parent.readsMemory = true;
757-
// Atomics are sequentially consistent with memory.size.
758-
parent.isAtomic = true;
762+
// Synchronizes when memory.grow on other threads, but only when operating
763+
// on shared memories.
764+
parent.isAtomic |= parent.module.getMemory(curr->memory)->shared;
759765
}
760766
void visitMemoryGrow(MemoryGrow* curr) {
761767
// TODO: find out if calls is necessary here
@@ -765,8 +771,9 @@ class EffectAnalyzer {
765771
// addresses, and just a read operation in the failure case
766772
parent.readsMemory = true;
767773
parent.writesMemory = true;
768-
// Atomics are also sequentially consistent with memory.grow.
769-
parent.isAtomic = true;
774+
// Synchronizes with memory.size on other threads, but only when operating
775+
// on shared memories.
776+
parent.isAtomic |= parent.module.getMemory(curr->memory)->shared;
770777
}
771778
void visitRefNull(RefNull* curr) {}
772779
void visitRefIsNull(RefIsNull* curr) {}
@@ -910,18 +917,8 @@ class EffectAnalyzer {
910917
if (curr->ref->type.isNullable()) {
911918
parent.implicitTrap = true;
912919
}
913-
switch (curr->order) {
914-
case MemoryOrder::Unordered:
915-
break;
916-
case MemoryOrder::SeqCst:
917-
// Synchronizes with other threads.
918-
parent.isAtomic = true;
919-
break;
920-
case MemoryOrder::AcqRel:
921-
// Only synchronizes if other threads can read the field.
922-
parent.isAtomic = curr->ref->type.getHeapType().isShared();
923-
break;
924-
}
920+
parent.isAtomic |=
921+
curr->isAtomic() && curr->ref->type.getHeapType().isShared();
925922
}
926923
void visitStructSet(StructSet* curr) {
927924
if (curr->ref->type.isNull()) {
@@ -933,9 +930,8 @@ class EffectAnalyzer {
933930
if (curr->ref->type.isNullable()) {
934931
parent.implicitTrap = true;
935932
}
936-
if (curr->order != MemoryOrder::Unordered) {
937-
parent.isAtomic = true;
938-
}
933+
parent.isAtomic |=
934+
curr->isAtomic() && curr->ref->type.getHeapType().isShared();
939935
}
940936
void visitStructRMW(StructRMW* curr) {
941937
if (curr->ref->type.isNull()) {
@@ -1037,6 +1033,8 @@ class EffectAnalyzer {
10371033
parent.readsArray = true;
10381034
// traps when the arg is null or the index out of bounds
10391035
parent.implicitTrap = true;
1036+
parent.isAtomic |=
1037+
curr->isAtomic() && curr->ref->type.getHeapType().isShared();
10401038
}
10411039
void visitArraySet(ArraySet* curr) {
10421040
if (curr->ref->type.isNull()) {
@@ -1046,6 +1044,8 @@ class EffectAnalyzer {
10461044
parent.writesArray = true;
10471045
// traps when the arg is null or the index out of bounds
10481046
parent.implicitTrap = true;
1047+
parent.isAtomic |=
1048+
curr->isAtomic() && curr->ref->type.getHeapType().isShared();
10491049
}
10501050
void visitArrayLen(ArrayLen* curr) {
10511051
if (curr->ref->type.isNull()) {

‎src/wasm.h‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1715,6 +1715,8 @@ class StructGet : public SpecificExpression<Expression::StructGetId> {
17151715
bool signed_ = false;
17161716
MemoryOrder order = MemoryOrder::Unordered;
17171717

1718+
bool isAtomic() const { return order != MemoryOrder::Unordered; }
1719+
17181720
void finalize();
17191721
};
17201722

@@ -1728,6 +1730,8 @@ class StructSet : public SpecificExpression<Expression::StructSetId> {
17281730
Expression* value;
17291731
MemoryOrder order = MemoryOrder::Unordered;
17301732

1733+
bool isAtomic() const { return order != MemoryOrder::Unordered; }
1734+
17311735
void finalize();
17321736
};
17331737

@@ -1844,6 +1848,8 @@ class ArrayGet : public SpecificExpression<Expression::ArrayGetId> {
18441848
bool signed_ = false;
18451849
MemoryOrder order = MemoryOrder::Unordered;
18461850

1851+
bool isAtomic() const { return order != MemoryOrder::Unordered; }
1852+
18471853
void finalize();
18481854
};
18491855

@@ -1857,6 +1863,8 @@ class ArraySet : public SpecificExpression<Expression::ArraySetId> {
18571863
Expression* value;
18581864
MemoryOrder order = MemoryOrder::Unordered;
18591865

1866+
bool isAtomic() const { return order != MemoryOrder::Unordered; }
1867+
18601868
void finalize();
18611869
};
18621870

‎test/lit/passes/simplify-locals-atomic-effects.wast‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
;; NOTE: Assertions have been generated by update_lit_checks.py and should not be edited.
2+
;; RUN: wasm-opt %s -all --simplify-locals -S -o - | filecheck %s
3+
4+
(module
5+
;; CHECK: (type $shared-struct (shared (struct (field (mut i32)))))
6+
(type $shared-struct (shared (struct (field (mut i32)))))
7+
;; CHECK: (type $unshared-struct (struct (field (mut i32))))
8+
(type $unshared-struct (struct (field (mut i32))))
9+
10+
;; CHECK: (memory $shared 1 1 shared)
11+
(memory $shared 1 1 shared)
12+
;; CHECK: (memory $unshared 1 1)
13+
(memory $unshared 1 1)
14+
15+
;; CHECK: (func $memory.size-shared-shared (type $2) (param $shared (ref null $shared-struct)) (result i32)
16+
;; CHECK-NEXT: (local $x i32)
17+
;; CHECK-NEXT: (local.set $x
18+
;; CHECK-NEXT: (memory.size $shared)
19+
;; CHECK-NEXT: )
20+
;; CHECK-NEXT: (struct.atomic.set $shared-struct 0
21+
;; CHECK-NEXT: (local.get $shared)
22+
;; CHECK-NEXT: (i32.const 0)
23+
;; CHECK-NEXT: )
24+
;; CHECK-NEXT: (local.get $x)
25+
;; CHECK-NEXT: )
26+
(func $memory.size-shared-shared (param $shared (ref null $shared-struct)) (result i32)
27+
(local $x i32)
28+
;; memory.size can synchronize with memory.grow on another thread when the
29+
;; memory is shared. It cannot be moved past a write to a shared struct.
30+
(local.set $x
31+
(memory.size $shared)
32+
)
33+
(struct.atomic.set $shared-struct 0 (local.get $shared) (i32.const 0))
34+
(local.get $x)
35+
)
36+
37+
;; CHECK: (func $memory.size-unshared-shared (type $2) (param $shared (ref null $shared-struct)) (result i32)
38+
;; CHECK-NEXT: (local $x i32)
39+
;; CHECK-NEXT: (local.set $x
40+
;; CHECK-NEXT: (memory.size $unshared)
41+
;; CHECK-NEXT: )
42+
;; CHECK-NEXT: (struct.atomic.set $shared-struct 0
43+
;; CHECK-NEXT: (local.get $shared)
44+
;; CHECK-NEXT: (i32.const 0)
45+
;; CHECK-NEXT: )
46+
;; CHECK-NEXT: (local.get $x)
47+
;; CHECK-NEXT: )
48+
(func $memory.size-unshared-shared (param $shared (ref null $shared-struct)) (result i32)
49+
(local $x i32)
50+
;; Now the memory is unshared. In principle we could reorder because no
51+
;; other thread could observe the reordering, but we do not distinguish
52+
;; between accesses to shared and unshared state. TODO.
53+
(local.set $x
54+
(memory.size $unshared)
55+
)
56+
(struct.atomic.set $shared-struct 0 (local.get $shared) (i32.const 0))
57+
(local.get $x)
58+
)
59+
60+
;; CHECK: (func $memory.size-shared-unshared (type $3) (param $unshared (ref null $unshared-struct)) (result i32)
61+
;; CHECK-NEXT: (local $x i32)
62+
;; CHECK-NEXT: (local.set $x
63+
;; CHECK-NEXT: (memory.size $shared)
64+
;; CHECK-NEXT: )
65+
;; CHECK-NEXT: (struct.atomic.set $unshared-struct 0
66+
;; CHECK-NEXT: (local.get $unshared)
67+
;; CHECK-NEXT: (i32.const 0)
68+
;; CHECK-NEXT: )
69+
;; CHECK-NEXT: (local.get $x)
70+
;; CHECK-NEXT: )
71+
(func $memory.size-shared-unshared (param $unshared (ref null $unshared-struct)) (result i32)
72+
(local $x i32)
73+
;; Now the memory is shared but the struct is unshared. Again, we could
74+
;; reorder in principle, but we do not yet. TODO.
75+
(local.set $x
76+
(memory.size $shared)
77+
)
78+
(struct.atomic.set $unshared-struct 0 (local.get $unshared) (i32.const 0))
79+
(local.get $x)
80+
)
81+
82+
;; CHECK: (func $memory.size-unshared-unshared (type $3) (param $unshared (ref null $unshared-struct)) (result i32)
83+
;; CHECK-NEXT: (local $x i32)
84+
;; CHECK-NEXT: (nop)
85+
;; CHECK-NEXT: (struct.atomic.set $unshared-struct 0
86+
;; CHECK-NEXT: (local.get $unshared)
87+
;; CHECK-NEXT: (i32.const 0)
88+
;; CHECK-NEXT: )
89+
;; CHECK-NEXT: (memory.size $unshared)
90+
;; CHECK-NEXT: )
91+
(func $memory.size-unshared-unshared (param $unshared (ref null $unshared-struct)) (result i32)
92+
(local $x i32)
93+
;; Now both the memory and struct are unshared, so neither can synchronize
94+
;; with anything and we reorder freely.
95+
(local.set $x
96+
(memory.size $unshared)
97+
)
98+
(struct.atomic.set $unshared-struct 0 (local.get $unshared) (i32.const 0))
99+
(local.get $x)
100+
)
101+
)

‎test/lit/passes/vacuum-gc-atomics.wast‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
(module
99
;; CHECK: (type $shared (shared (struct (field i32))))
1010
(type $shared (shared (struct (field i32))))
11-
;; CHECK: (type $unshared (struct (field i32)))
1211
(type $unshared (struct (field i32)))
1312

1413
;; CHECK: (func $get-unordered-unshared (type $0)
@@ -34,11 +33,7 @@
3433
)
3534

3635
;; CHECK: (func $get-seqcst-unshared (type $0)
37-
;; CHECK-NEXT: (drop
38-
;; CHECK-NEXT: (struct.atomic.get $unshared 0
39-
;; CHECK-NEXT: (struct.new_default $unshared)
40-
;; CHECK-NEXT: )
41-
;; CHECK-NEXT: )
36+
;; CHECK-NEXT: (nop)
4237
;; CHECK-NEXT: )
4338
(func $get-seqcst-unshared
4439
(drop

‎test/lit/passes/vacuum-tnh.wast‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
;; NO_TNH: (tag $tag (type $2) (param i32))
1515
(tag $tag (param i32))
1616

17-
(memory 1 1)
17+
(memory 1 1 shared)
1818

1919
(type $struct (struct (field (mut i32))))
2020

‎test/wasm2js/bulk-memory.2asm.js.opt‎

Lines changed: 2 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -276,15 +276,6 @@ export var init = retasmFunc.init;
276276
export var load8_u = retasmFunc.load8_u;
277277

278278
function asmFunc(imports) {
279-
var buffer = new ArrayBuffer(65536);
280-
var HEAP8 = new Int8Array(buffer);
281-
var HEAP16 = new Int16Array(buffer);
282-
var HEAP32 = new Int32Array(buffer);
283-
var HEAPU8 = new Uint8Array(buffer);
284-
var HEAPU16 = new Uint16Array(buffer);
285-
var HEAPU32 = new Uint32Array(buffer);
286-
var HEAPF32 = new Float32Array(buffer);
287-
var HEAPF64 = new Float64Array(buffer);
288279
var Math_imul = Math.imul;
289280
var Math_fround = Math.fround;
290281
var Math_abs = Math.abs;
@@ -299,40 +290,11 @@ function asmFunc(imports) {
299290

300291
}
301292

302-
function $1() {
303-
__wasm_memory_size();
304-
}
305-
306-
function __wasm_memory_size() {
307-
return buffer.byteLength / 65536 | 0;
308-
}
309-
310-
function __wasm_memory_grow(pagesToAdd) {
311-
pagesToAdd = pagesToAdd | 0;
312-
var oldPages = __wasm_memory_size() | 0;
313-
var newPages = oldPages + pagesToAdd | 0;
314-
if ((oldPages < newPages) && (newPages < 65536)) {
315-
var newBuffer = new ArrayBuffer(Math_imul(newPages, 65536));
316-
var newHEAP8 = new Int8Array(newBuffer);
317-
newHEAP8.set(HEAP8);
318-
HEAP8 = new Int8Array(newBuffer);
319-
HEAP16 = new Int16Array(newBuffer);
320-
HEAP32 = new Int32Array(newBuffer);
321-
HEAPU8 = new Uint8Array(newBuffer);
322-
HEAPU16 = new Uint16Array(newBuffer);
323-
HEAPU32 = new Uint32Array(newBuffer);
324-
HEAPF32 = new Float32Array(newBuffer);
325-
HEAPF64 = new Float64Array(newBuffer);
326-
buffer = newBuffer;
327-
}
328-
return oldPages;
329-
}
330-
331293
return {
332294
"drop_passive": $0,
333-
"init_passive": $1,
295+
"init_passive": $0,
334296
"drop_active": $0,
335-
"init_active": $1
297+
"init_active": $0
336298
};
337299
}
338300

0 commit comments

Comments
 (0)