Skip to content

Commit 969bf76

Browse files
kripkenkripken
authored
[GC] Precompute: Fix ref.eq comparisons of structs with nested effects (WebAssembly#7857)
In the testcase here, the nested tee make the outer struct not precomputable when we care about effects (tee is an effect). But we could precompute it otherwise, and that ended up causing confusion, with no caching being done of the canonical allocation for it, meaning each time we scanned it we saw another struct, different in identity. To fix this, implement the TODO to cache in both modes. I thought that would be useful only for speed, but turns out it is necessary for correctness. Luckily doing so is quite simple.
1 parent 605ef3b commit 969bf76

File tree

2 files changed

+94
-29
lines changed

2 files changed

+94
-29
lines changed

‎src/passes/Precompute.cpp‎

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,22 @@ using GetValues = std::unordered_map<LocalGet*, Literals>;
7171
// representation, the merge will cause a local.get of $x to have more
7272
// possible input values than that struct.new, which means we will not infer
7373
// a value for it, and not attempt to say anything about comparisons of $x.
74-
using HeapValues = std::unordered_map<Expression*, std::shared_ptr<GCData>>;
74+
struct HeapValues {
75+
// Store two maps, one for effects and one without. The one with effects is
76+
// used when PRESERVE_SIDEEFFECTS is on, and the other when not. This is
77+
// necessary because when we preserve effects then nested effects in a GC
78+
// allocation can cause us to end up as nonconstant (nothing can be
79+
// precomputed), and we do not want to mix results between the two modes (if
80+
// we did, we might cache a result when we ignore effects that we later use
81+
// when not ignoring them, which would forget the effects).
82+
std::unordered_map<Expression*, std::shared_ptr<GCData>> withEffects,
83+
withoutEffects;
84+
85+
void clear() {
86+
withEffects.clear();
87+
withoutEffects.clear();
88+
}
89+
};
7590

7691
// Precomputes an expression. Errors if we hit anything that can't be
7792
// precomputed. Inherits most of its functionality from
@@ -187,11 +202,14 @@ class PrecomputingExpressionRunner
187202

188203
// Generates heap info for a heap-allocating expression.
189204
Flow getGCAllocation(Expression* curr, std::function<Flow()> visitFunc) {
205+
auto& heapValuesMap = (flags & FlagValues::PRESERVE_SIDEEFFECTS)
206+
? heapValues.withEffects
207+
: heapValues.withoutEffects;
190208
// We must return a literal that refers to the canonical location for this
191209
// source expression, so that each time we compute a specific *.new then
192210
// we get the same identity.
193-
auto iter = heapValues.find(curr);
194-
if (iter != heapValues.end()) {
211+
auto iter = heapValuesMap.find(curr);
212+
if (iter != heapValuesMap.end()) {
195213
// Refer to the same canonical GCData that we already created.
196214
return Literal(iter->second, curr->type.getHeapType());
197215
}
@@ -200,14 +218,7 @@ class PrecomputingExpressionRunner
200218
if (flow.breaking()) {
201219
return flow;
202220
}
203-
// If we preserve side effects then we can cache the results, but if we
204-
// ignore them then the result we compute does not contain them, and later
205-
// reads from the cache that do care about side effects would be wrong.
206-
// TODO: use a separate cache for the two modes, but the other mode is
207-
// only used in propagateLocals, which is far less used
208-
if (flags & FlagValues::PRESERVE_SIDEEFFECTS) {
209-
heapValues[curr] = flow.getSingleValue().getGCData();
210-
}
221+
heapValuesMap[curr] = flow.getSingleValue().getGCData();
211222
return flow;
212223
}
213224

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

Lines changed: 72 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,9 @@
1717

1818
;; CHECK: (type $array-i32 (array (mut i32)))
1919

20+
;; CHECK: (type $referrer (struct (field (mut (ref null $empty)))))
21+
(type $referrer (struct (field (mut (ref null $empty)))))
22+
2023
;; CHECK: (type $B (struct (field (mut f64))))
2124
(type $B (struct (field (mut f64))))
2225

@@ -175,7 +178,7 @@
175178
(struct.get $struct 0 (local.get $x))
176179
)
177180
)
178-
;; CHECK: (func $modify-gc-heap (type $6) (param $x (ref null $struct))
181+
;; CHECK: (func $modify-gc-heap (type $7) (param $x (ref null $struct))
179182
;; CHECK-NEXT: (struct.set $struct 0
180183
;; CHECK-NEXT: (local.get $x)
181184
;; CHECK-NEXT: (i32.add
@@ -229,7 +232,7 @@
229232
(struct.get $struct 0 (local.get $x))
230233
)
231234
)
232-
;; CHECK: (func $load-from-struct-bad-arrive (type $6) (param $x (ref null $struct))
235+
;; CHECK: (func $load-from-struct-bad-arrive (type $7) (param $x (ref null $struct))
233236
;; CHECK-NEXT: (call $log
234237
;; CHECK-NEXT: (struct.get $struct 0
235238
;; CHECK-NEXT: (local.get $x)
@@ -242,7 +245,7 @@
242245
(struct.get $struct 0 (local.get $x))
243246
)
244247
)
245-
;; CHECK: (func $ref-comparisons (type $12) (param $x (ref null $struct)) (param $y (ref null $struct))
248+
;; CHECK: (func $ref-comparisons (type $13) (param $x (ref null $struct)) (param $y (ref null $struct))
246249
;; CHECK-NEXT: (local $z (ref null $struct))
247250
;; CHECK-NEXT: (local $w (ref null $struct))
248251
;; CHECK-NEXT: (call $log
@@ -395,7 +398,7 @@
395398
(local.get $tempresult)
396399
)
397400

398-
;; CHECK: (func $propagate-uncertain-param (type $7) (param $input (ref $empty)) (result i32)
401+
;; CHECK: (func $propagate-uncertain-param (type $8) (param $input (ref $empty)) (result i32)
399402
;; CHECK-NEXT: (local $tempresult i32)
400403
;; CHECK-NEXT: (local $tempref (ref null $empty))
401404
;; CHECK-NEXT: (local.set $tempresult
@@ -420,7 +423,7 @@
420423
(local.get $tempresult)
421424
)
422425

423-
;; CHECK: (func $propagate-different-params (type $13) (param $input1 (ref $empty)) (param $input2 (ref $empty)) (result i32)
426+
;; CHECK: (func $propagate-different-params (type $14) (param $input1 (ref $empty)) (param $input2 (ref $empty)) (result i32)
424427
;; CHECK-NEXT: (local $tempresult i32)
425428
;; CHECK-NEXT: (local.set $tempresult
426429
;; CHECK-NEXT: (ref.eq
@@ -442,7 +445,7 @@
442445
(local.get $tempresult)
443446
)
444447

445-
;; CHECK: (func $propagate-same-param (type $7) (param $input (ref $empty)) (result i32)
448+
;; CHECK: (func $propagate-same-param (type $8) (param $input (ref $empty)) (result i32)
446449
;; CHECK-NEXT: (local $tempresult i32)
447450
;; CHECK-NEXT: (local.set $tempresult
448451
;; CHECK-NEXT: (ref.eq
@@ -744,7 +747,7 @@
744747
)
745748
)
746749

747-
;; CHECK: (func $helper (type $14) (param $0 i32) (result i32)
750+
;; CHECK: (func $helper (type $15) (param $0 i32) (result i32)
748751
;; CHECK-NEXT: (unreachable)
749752
;; CHECK-NEXT: )
750753
(func $helper (param i32) (result i32)
@@ -822,14 +825,14 @@
822825
)
823826
)
824827

825-
;; CHECK: (func $receive-f64 (type $15) (param $0 f64)
828+
;; CHECK: (func $receive-f64 (type $16) (param $0 f64)
826829
;; CHECK-NEXT: (unreachable)
827830
;; CHECK-NEXT: )
828831
(func $receive-f64 (param f64)
829832
(unreachable)
830833
)
831834

832-
;; CHECK: (func $odd-cast-and-get-non-null (type $16) (param $temp (ref $func-return-i32))
835+
;; CHECK: (func $odd-cast-and-get-non-null (type $17) (param $temp (ref $func-return-i32))
833836
;; CHECK-NEXT: (local.set $temp
834837
;; CHECK-NEXT: (ref.cast (ref nofunc)
835838
;; CHECK-NEXT: (ref.func $receive-f64)
@@ -857,7 +860,7 @@
857860
)
858861
)
859862

860-
;; CHECK: (func $new_block_unreachable (type $9) (result anyref)
863+
;; CHECK: (func $new_block_unreachable (type $10) (result anyref)
861864
;; CHECK-NEXT: (block ;; (replaces unreachable StructNew we can't emit)
862865
;; CHECK-NEXT: (drop
863866
;; CHECK-NEXT: (block
@@ -878,7 +881,7 @@
878881
)
879882
)
880883

881-
;; CHECK: (func $br_on_cast-on-creation (type $17) (result (ref $empty))
884+
;; CHECK: (func $br_on_cast-on-creation (type $18) (result (ref $empty))
882885
;; CHECK-NEXT: (block $label (result (ref (exact $empty)))
883886
;; CHECK-NEXT: (drop
884887
;; CHECK-NEXT: (br_on_cast $label (ref (exact $empty)) (ref (exact $empty))
@@ -977,7 +980,7 @@
977980
)
978981
)
979982

980-
;; CHECK: (func $remove-set (type $18) (result (ref func))
983+
;; CHECK: (func $remove-set (type $19) (result (ref func))
981984
;; CHECK-NEXT: (local $nn funcref)
982985
;; CHECK-NEXT: (local $i i32)
983986
;; CHECK-NEXT: (loop $loop
@@ -1020,7 +1023,7 @@
10201023
)
10211024
)
10221025

1023-
;; CHECK: (func $strings (type $19) (param $param (ref string))
1026+
;; CHECK: (func $strings (type $20) (param $param (ref string))
10241027
;; CHECK-NEXT: (local $s (ref string))
10251028
;; CHECK-NEXT: (local.set $s
10261029
;; CHECK-NEXT: (string.const "hello, world")
@@ -1059,7 +1062,7 @@
10591062
)
10601063
)
10611064

1062-
;; CHECK: (func $get-nonnullable-in-unreachable (type $9) (result anyref)
1065+
;; CHECK: (func $get-nonnullable-in-unreachable (type $10) (result anyref)
10631066
;; CHECK-NEXT: (local $x (ref any))
10641067
;; CHECK-NEXT: (local.tee $x
10651068
;; CHECK-NEXT: (unreachable)
@@ -1096,7 +1099,7 @@
10961099
(local.get $x)
10971100
)
10981101

1099-
;; CHECK: (func $get-nonnullable-in-unreachable-entry (type $10) (param $x i32) (param $y (ref any))
1102+
;; CHECK: (func $get-nonnullable-in-unreachable-entry (type $11) (param $x i32) (param $y (ref any))
11001103
;; CHECK-NEXT: (local $0 (ref any))
11011104
;; CHECK-NEXT: (unreachable)
11021105
;; CHECK-NEXT: (local.set $0
@@ -1130,7 +1133,7 @@
11301133
)
11311134
)
11321135

1133-
;; CHECK: (func $get-nonnullable-in-unreachable-later-loop (type $10) (param $x i32) (param $y (ref any))
1136+
;; CHECK: (func $get-nonnullable-in-unreachable-later-loop (type $11) (param $x i32) (param $y (ref any))
11341137
;; CHECK-NEXT: (local $0 (ref any))
11351138
;; CHECK-NEXT: (if
11361139
;; CHECK-NEXT: (local.get $x)
@@ -1175,7 +1178,7 @@
11751178
)
11761179
)
11771180

1178-
;; CHECK: (func $get-nonnullable-in-unreachable-tuple (type $20) (result anyref i32)
1181+
;; CHECK: (func $get-nonnullable-in-unreachable-tuple (type $21) (result anyref i32)
11791182
;; CHECK-NEXT: (local $x (tuple (ref any) i32))
11801183
;; CHECK-NEXT: (local.tee $x
11811184
;; CHECK-NEXT: (unreachable)
@@ -1281,5 +1284,56 @@
12811284
)
12821285
)
12831286
)
1284-
)
12851287

1288+
;; CHECK: (func $nested-struct-ref.eq (type $2)
1289+
;; CHECK-NEXT: (local $A (ref $referrer))
1290+
;; CHECK-NEXT: (local $B (ref $empty))
1291+
;; CHECK-NEXT: (local $A2 (ref $referrer))
1292+
;; CHECK-NEXT: (local $temp i32)
1293+
;; CHECK-NEXT: (local.set $A2
1294+
;; CHECK-NEXT: (local.tee $A
1295+
;; CHECK-NEXT: (struct.new $referrer
1296+
;; CHECK-NEXT: (local.tee $B
1297+
;; CHECK-NEXT: (struct.new_default $empty)
1298+
;; CHECK-NEXT: )
1299+
;; CHECK-NEXT: )
1300+
;; CHECK-NEXT: )
1301+
;; CHECK-NEXT: )
1302+
;; CHECK-NEXT: (local.set $temp
1303+
;; CHECK-NEXT: (i32.const 1)
1304+
;; CHECK-NEXT: )
1305+
;; CHECK-NEXT: (local.set $temp
1306+
;; CHECK-NEXT: (i32.const 0)
1307+
;; CHECK-NEXT: )
1308+
;; CHECK-NEXT: )
1309+
(func $nested-struct-ref.eq
1310+
(local $A (ref $referrer))
1311+
(local $B (ref $empty))
1312+
(local $A2 (ref $referrer))
1313+
(local $temp i32)
1314+
;; Create $A, and copy to $A2.
1315+
(local.set $A2
1316+
(local.tee $A
1317+
(struct.new $referrer
1318+
(local.tee $B
1319+
(struct.new_default $empty)
1320+
)
1321+
)
1322+
)
1323+
)
1324+
;; They should be equal, so this can be 1.
1325+
(local.set $temp
1326+
(ref.eq
1327+
(local.get $A)
1328+
(local.get $A2)
1329+
)
1330+
)
1331+
;; $A and $B are of course different.
1332+
(local.set $temp
1333+
(ref.eq
1334+
(local.get $A)
1335+
(local.get $B)
1336+
)
1337+
)
1338+
)
1339+
)

0 commit comments

Comments
 (0)