Skip to content

Commit 290147d

Browse files
kripkenkripken
authored
[GC] Fix Array optimization issues (#3438)
Precompute still tried to precompute a reference because the check was not in the topmost place. Also we truncated i8/i16 values, but did not extend them properly. That was also an issue with structs. The new test replaces the old one by moving from -O1 to -Oz (which runs more opts, and would have noticed this earlier), and adds array operations too, including sign extends.
1 parent e16cf58 commit 290147d

6 files changed

+217
-102
lines changed

‎src/passes/Precompute.cpp

+6-5
Original file line numberDiff line numberDiff line change
@@ -168,11 +168,6 @@ struct Precompute
168168
if (curr->type.isVector()) {
169169
return;
170170
}
171-
// Don't try to precompute a reference. We can't replace it with a constant
172-
// expression, as that would make a copy of it by value.
173-
if (curr->type.isRef()) {
174-
return;
175-
}
176171
// try to evaluate this into a const
177172
Flow flow = precomputeExpression(curr);
178173
if (flow.getType().hasVector()) {
@@ -228,6 +223,12 @@ struct Precompute
228223
// Precompute an expression, returning a flow, which may be a constant
229224
// (that we can replace the expression with if replaceExpression is set).
230225
Flow precomputeExpression(Expression* curr, bool replaceExpression = true) {
226+
// Don't try to precompute a reference. We can't replace it with a constant
227+
// expression, as that would make a copy of it by value.
228+
// TODO: do so when safe
229+
if (curr->type.isRef()) {
230+
return Flow(NONCONSTANT_FLOW);
231+
}
231232
try {
232233
return PrecomputingExpressionRunner(
233234
getModule(), getValues, replaceExpression)

‎src/wasm-interpreter.h

+32-8
Original file line numberDiff line numberDiff line change
@@ -1427,7 +1427,8 @@ class ExpressionRunner : public OverriddenVisitor<SubType, Flow> {
14271427
if (!data) {
14281428
trap("null ref");
14291429
}
1430-
return (*data)[curr->index];
1430+
auto field = curr->ref->type.getHeapType().getStruct().fields[curr->index];
1431+
return extendForPacking((*data)[curr->index], field, curr->signed_);
14311432
}
14321433
Flow visitStructSet(StructSet* curr) {
14331434
NOTE_ENTER("StructSet");
@@ -1444,7 +1445,7 @@ class ExpressionRunner : public OverriddenVisitor<SubType, Flow> {
14441445
trap("null ref");
14451446
}
14461447
auto field = curr->ref->type.getHeapType().getStruct().fields[curr->index];
1447-
(*data)[curr->index] = getMaybePackedValue(value.getSingleValue(), field);
1448+
(*data)[curr->index] = truncateForPacking(value.getSingleValue(), field);
14481449
return Flow();
14491450
}
14501451
Flow visitArrayNew(ArrayNew* curr) {
@@ -1494,7 +1495,8 @@ class ExpressionRunner : public OverriddenVisitor<SubType, Flow> {
14941495
if (i >= data->size()) {
14951496
trap("array oob");
14961497
}
1497-
return (*data)[i];
1498+
auto field = curr->ref->type.getHeapType().getArray().element;
1499+
return extendForPacking((*data)[i], field, curr->signed_);
14981500
}
14991501
Flow visitArraySet(ArraySet* curr) {
15001502
NOTE_ENTER("ArraySet");
@@ -1519,7 +1521,7 @@ class ExpressionRunner : public OverriddenVisitor<SubType, Flow> {
15191521
trap("array oob");
15201522
}
15211523
auto field = curr->ref->type.getHeapType().getArray().element;
1522-
(*data)[i] = getMaybePackedValue(value.getSingleValue(), field);
1524+
(*data)[i] = truncateForPacking(value.getSingleValue(), field);
15231525
return Flow();
15241526
}
15251527
Flow visitArrayLen(ArrayLen* curr) {
@@ -1541,13 +1543,35 @@ class ExpressionRunner : public OverriddenVisitor<SubType, Flow> {
15411543

15421544
private:
15431545
// Truncate the value if we need to. The storage is just a list of Literals,
1544-
// so we can't just write the value like we would to a C struct field.
1545-
Literal getMaybePackedValue(Literal value, const Field& field) {
1546+
// so we can't just write the value like we would to a C struct field and
1547+
// expect it to truncate for us. Instead, we truncate so the stored value is
1548+
// proper for the type.
1549+
Literal truncateForPacking(Literal value, const Field& field) {
1550+
if (field.type == Type::i32) {
1551+
int32_t c = value.geti32();
1552+
if (field.packedType == Field::i8) {
1553+
value = Literal(c & 0xff);
1554+
} else if (field.packedType == Field::i16) {
1555+
value = Literal(c & 0xffff);
1556+
}
1557+
}
1558+
return value;
1559+
}
1560+
1561+
Literal extendForPacking(Literal value, const Field& field, bool signed_) {
15461562
if (field.type == Type::i32) {
1563+
int32_t c = value.geti32();
15471564
if (field.packedType == Field::i8) {
1548-
value = value.and_(Literal(int32_t(0xff)));
1565+
// The stored value should already be truncated.
1566+
assert(c == (c & 0xff));
1567+
if (signed_) {
1568+
value = Literal((c << 24) >> 24);
1569+
}
15491570
} else if (field.packedType == Field::i16) {
1550-
value = value.and_(Literal(int32_t(0xffff)));
1571+
assert(c == (c & 0xffff));
1572+
if (signed_) {
1573+
value = Literal((c << 16) >> 16);
1574+
}
15511575
}
15521576
}
15531577
return value;

‎test/passes/O1_fuzz-exec_all-features.txt

-52
This file was deleted.

‎test/passes/O1_fuzz-exec_all-features.wast

-37
This file was deleted.

‎test/passes/Oz_fuzz-exec_all-features.txt

+109
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
[fuzz-exec] calling structs
2+
[LoggingExternalInterface logging 0]
3+
[LoggingExternalInterface logging 42]
4+
[LoggingExternalInterface logging 100]
5+
[LoggingExternalInterface logging 100]
6+
[fuzz-exec] calling arrays
7+
[LoggingExternalInterface logging 50]
8+
[LoggingExternalInterface logging 42]
9+
[LoggingExternalInterface logging 128]
10+
[LoggingExternalInterface logging -128]
11+
[LoggingExternalInterface logging 42]
12+
(module
13+
(type ${i32} (struct (field i32)))
14+
(type $none_=>_none (func))
15+
(type $[mut:i8] (array (mut i8)))
16+
(type $i32_=>_none (func (param i32)))
17+
(import "fuzzing-support" "log-i32" (func $log (param i32)))
18+
(export "structs" (func $0))
19+
(export "arrays" (func $1))
20+
(func $0 (; has Stack IR ;)
21+
(local $0 (ref null ${i32}))
22+
(call $log
23+
(struct.get ${i32} 0
24+
(local.tee $0
25+
(struct.new_default_with_rtt ${i32}
26+
(rtt.canon ${i32})
27+
)
28+
)
29+
)
30+
)
31+
(struct.set ${i32} 0
32+
(local.get $0)
33+
(i32.const 42)
34+
)
35+
(call $log
36+
(struct.get ${i32} 0
37+
(local.get $0)
38+
)
39+
)
40+
(struct.set ${i32} 0
41+
(local.get $0)
42+
(i32.const 100)
43+
)
44+
(call $log
45+
(struct.get ${i32} 0
46+
(local.get $0)
47+
)
48+
)
49+
(call $log
50+
(struct.get ${i32} 0
51+
(local.get $0)
52+
)
53+
)
54+
)
55+
(func $1 (; has Stack IR ;)
56+
(local $0 (ref null $[mut:i8]))
57+
(call $log
58+
(array.len $[mut:i8]
59+
(local.tee $0
60+
(array.new_with_rtt $[mut:i8]
61+
(rtt.canon $[mut:i8])
62+
(i32.const 50)
63+
(i32.const 42)
64+
)
65+
)
66+
)
67+
)
68+
(call $log
69+
(array.get_u $[mut:i8]
70+
(local.get $0)
71+
(i32.const 10)
72+
)
73+
)
74+
(array.set $[mut:i8]
75+
(local.get $0)
76+
(i32.const 10)
77+
(i32.const 65408)
78+
)
79+
(call $log
80+
(array.get_u $[mut:i8]
81+
(local.get $0)
82+
(i32.const 10)
83+
)
84+
)
85+
(call $log
86+
(array.get_s $[mut:i8]
87+
(local.get $0)
88+
(i32.const 10)
89+
)
90+
)
91+
(call $log
92+
(array.get_s $[mut:i8]
93+
(local.get $0)
94+
(i32.const 20)
95+
)
96+
)
97+
)
98+
)
99+
[fuzz-exec] calling structs
100+
[LoggingExternalInterface logging 0]
101+
[LoggingExternalInterface logging 42]
102+
[LoggingExternalInterface logging 100]
103+
[LoggingExternalInterface logging 100]
104+
[fuzz-exec] calling arrays
105+
[LoggingExternalInterface logging 50]
106+
[LoggingExternalInterface logging 42]
107+
[LoggingExternalInterface logging 128]
108+
[LoggingExternalInterface logging -128]
109+
[LoggingExternalInterface logging 42]

‎test/passes/Oz_fuzz-exec_all-features.wast

+70
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
(module
2+
(type $struct (struct i32))
3+
(type $bytes (array (mut i8)))
4+
(import "fuzzing-support" "log-i32" (func $log (param i32)))
5+
(func "structs"
6+
(local $x (ref null $struct))
7+
(local $y (ref null $struct))
8+
(local.set $x
9+
(struct.new_default_with_rtt $struct
10+
(rtt.canon $struct)
11+
)
12+
)
13+
;; The value is initialized to 0
14+
;; Note: -Oz will optimize all these to constants thanks to Precompute
15+
(call $log
16+
(struct.get $struct 0 (local.get $x))
17+
)
18+
;; Assigning a value works
19+
(struct.set $struct 0
20+
(local.get $x)
21+
(i32.const 42)
22+
)
23+
(call $log
24+
(struct.get $struct 0 (local.get $x))
25+
)
26+
;; References are references, so writing to one's value affects the other's
27+
(local.set $y (local.get $x))
28+
(struct.set $struct 0
29+
(local.get $y)
30+
(i32.const 100)
31+
)
32+
(call $log
33+
(struct.get $struct 0 (local.get $x))
34+
)
35+
(call $log
36+
(struct.get $struct 0 (local.get $y))
37+
)
38+
)
39+
(func "arrays"
40+
(local $x (ref null $bytes))
41+
(local.set $x
42+
(array.new_with_rtt $bytes
43+
(rtt.canon $bytes)
44+
(i32.const 50) ;; size
45+
(i32.const 42) ;; value to splat into the array
46+
)
47+
)
48+
;; The length should be 50
49+
(call $log
50+
(array.len $bytes (local.get $x))
51+
)
52+
;; The value should be 42
53+
(call $log
54+
(array.get_u $bytes (local.get $x) (i32.const 10))
55+
)
56+
;; Write a value that will be truncated into an i8
57+
(array.set $bytes (local.get $x) (i32.const 10) (i32.const 0xff80))
58+
;; The value should be 0x80 (-128 or 128 depending on signed/unsigned)
59+
(call $log
60+
(array.get_u $bytes (local.get $x) (i32.const 10))
61+
)
62+
(call $log
63+
(array.get_s $bytes (local.get $x) (i32.const 10))
64+
)
65+
;; Other items than the one at index 10 are unaffected.
66+
(call $log
67+
(array.get_s $bytes (local.get $x) (i32.const 20))
68+
)
69+
)
70+
)

0 commit comments

Comments
 (0)