Skip to content

Commit b9a2c29

Browse files
sumleosumleo
authored
Fix Precompute visitArrayGet missing memory ordering check (#8327)
- `visitStructGet` in the precompute pass correctly checks `MemoryOrder` before allowing precomputation of immutable fields—blocking `SeqCst` entirely and blocking shared `AcqRel`—but `visitArrayGet` skipped this check. - This causes `array.atomic.get` with `SeqCst` or `AcqRel` ordering on immutable array elements to be incorrectly constant-folded, removing synchronization semantics. - Added the same `MemoryOrder` switch to `visitArrayGet` matching the existing pattern in `visitStructGet`.
1 parent 204c3f8 commit b9a2c29

2 files changed

Lines changed: 122 additions & 6 deletions

File tree

‎src/passes/Precompute.cpp‎

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -185,12 +185,29 @@ class PrecomputingExpressionRunner
185185
}
186186
Flow visitArraySet(ArraySet* curr) { return Flow(NONCONSTANT_FLOW); }
187187
Flow visitArrayGet(ArrayGet* curr) {
188-
if (curr->ref->type != Type::unreachable && !curr->ref->type.isNull()) {
189-
// See above with struct.get
190-
auto element = curr->ref->type.getHeapType().getArray().element;
191-
if (element.mutable_ == Immutable) {
192-
return Super::visitArrayGet(curr);
193-
}
188+
if (curr->ref->type == Type::unreachable || curr->ref->type.isNull()) {
189+
return Flow(NONCONSTANT_FLOW);
190+
}
191+
switch (curr->order) {
192+
case MemoryOrder::Unordered:
193+
// This can always be precomputed.
194+
break;
195+
case MemoryOrder::SeqCst:
196+
// This can never be precomputed away because it synchronizes with other
197+
// threads.
198+
return Flow(NONCONSTANT_FLOW);
199+
case MemoryOrder::AcqRel:
200+
// This synchronizes only with writes to the same data, so it can still
201+
// be precomputed if the data is not shared with other threads.
202+
if (curr->ref->type.getHeapType().isShared()) {
203+
return Flow(NONCONSTANT_FLOW);
204+
}
205+
break;
206+
}
207+
// See above with struct.get
208+
auto element = curr->ref->type.getHeapType().getArray().element;
209+
if (element.mutable_ == Immutable) {
210+
return Super::visitArrayGet(curr);
194211
}
195212

196213
// Otherwise, we've failed to precompute.

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

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,13 @@
55
(module
66
;; CHECK: (type $shared (shared (struct (field i32))))
77
(type $shared (shared (struct (field i32))))
8+
;; CHECK: (type $shared-array (shared (array i32)))
9+
810
;; CHECK: (type $unshared (struct (field i32)))
911
(type $unshared (struct (field i32)))
1012

13+
;; CHECK: (type $unshared-array (array i32))
14+
1115
;; CHECK: (func $get-unordered-unshared (type $0) (result i32)
1216
;; CHECK-NEXT: (i32.const 0)
1317
;; CHECK-NEXT: )
@@ -69,4 +73,99 @@
6973
(struct.new_default $shared)
7074
)
7175
)
76+
77+
;; Array versions of the same tests. These should behave the same as the
78+
;; struct versions above.
79+
80+
(type $shared-array (shared (array i32)))
81+
(type $unshared-array (array i32))
82+
83+
;; CHECK: (func $array-get-unordered-unshared (type $0) (result i32)
84+
;; CHECK-NEXT: (i32.const 0)
85+
;; CHECK-NEXT: )
86+
(func $array-get-unordered-unshared (result i32)
87+
(array.get $unshared-array
88+
(array.new_default $unshared-array
89+
(i32.const 1)
90+
)
91+
(i32.const 0)
92+
)
93+
)
94+
95+
;; CHECK: (func $array-get-unordered-shared (type $0) (result i32)
96+
;; CHECK-NEXT: (i32.const 0)
97+
;; CHECK-NEXT: )
98+
(func $array-get-unordered-shared (result i32)
99+
(array.get $shared-array
100+
(array.new_default $shared-array
101+
(i32.const 1)
102+
)
103+
(i32.const 0)
104+
)
105+
)
106+
107+
;; CHECK: (func $array-get-seqcst-unshared (type $0) (result i32)
108+
;; CHECK-NEXT: (array.atomic.get $unshared-array
109+
;; CHECK-NEXT: (array.new_default $unshared-array
110+
;; CHECK-NEXT: (i32.const 1)
111+
;; CHECK-NEXT: )
112+
;; CHECK-NEXT: (i32.const 0)
113+
;; CHECK-NEXT: )
114+
;; CHECK-NEXT: )
115+
(func $array-get-seqcst-unshared (result i32)
116+
(array.atomic.get seqcst $unshared-array
117+
(array.new_default $unshared-array
118+
(i32.const 1)
119+
)
120+
(i32.const 0)
121+
)
122+
)
123+
124+
;; CHECK: (func $array-get-seqcst-shared (type $0) (result i32)
125+
;; CHECK-NEXT: (array.atomic.get $shared-array
126+
;; CHECK-NEXT: (array.new_default $shared-array
127+
;; CHECK-NEXT: (i32.const 1)
128+
;; CHECK-NEXT: )
129+
;; CHECK-NEXT: (i32.const 0)
130+
;; CHECK-NEXT: )
131+
;; CHECK-NEXT: )
132+
(func $array-get-seqcst-shared (result i32)
133+
(array.atomic.get seqcst $shared-array
134+
(array.new_default $shared-array
135+
(i32.const 1)
136+
)
137+
(i32.const 0)
138+
)
139+
)
140+
141+
;; CHECK: (func $array-get-acqrel-unshared (type $0) (result i32)
142+
;; CHECK-NEXT: (i32.const 0)
143+
;; CHECK-NEXT: )
144+
(func $array-get-acqrel-unshared (result i32)
145+
;; We can optimize this because acquire-release on unshared data does not
146+
;; synchronize with anything.
147+
(array.atomic.get acqrel $unshared-array
148+
(array.new_default $unshared-array
149+
(i32.const 1)
150+
)
151+
(i32.const 0)
152+
)
153+
)
154+
155+
;; CHECK: (func $array-get-acqrel-shared (type $0) (result i32)
156+
;; CHECK-NEXT: (array.atomic.get acqrel $shared-array
157+
;; CHECK-NEXT: (array.new_default $shared-array
158+
;; CHECK-NEXT: (i32.const 1)
159+
;; CHECK-NEXT: )
160+
;; CHECK-NEXT: (i32.const 0)
161+
;; CHECK-NEXT: )
162+
;; CHECK-NEXT: )
163+
(func $array-get-acqrel-shared (result i32)
164+
(array.atomic.get acqrel $shared-array
165+
(array.new_default $shared-array
166+
(i32.const 1)
167+
)
168+
(i32.const 0)
169+
)
170+
)
72171
)

0 commit comments

Comments
 (0)