Skip to content

Commit 7d05d2e

Browse files
tlivelytlively
committed
Update GTO and TypeRefining for Struct RMW operations
Both passes use StructUtils::StructScanner to analyze struct operations. Add support for RMW operations to this utility and update its users to provide the new `noteRMW` hook. Test that GTO and TypeRefining optimizations work as expected in the presence of RMW operations, but leave a proper implementation in ConstantFieldPropagation to a later PR. To allow TypeRefining to refine field types based only on the "replacement" operand and not the "expected" operand of cmpxchg operations, update validation to allow the "expected" field to be a supertype of the accessed field type as long as it is still equality comparable. WebAssembly/shared-everything-threads#92 clarifies this intended typing in the upstream proposal.
1 parent f05cf1d commit 7d05d2e

File tree

8 files changed

+341
-16
lines changed

8 files changed

+341
-16
lines changed

‎scripts/test/fuzzing.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,8 @@
8484
'shared-structs.wast',
8585
'heap2local-rmw.wast',
8686
'optimize-instructions-struct-rmw.wast',
87+
'gto-removals-rmw.wast',
88+
'type-refining-rmw.wast',
8789
# contains too many segments to run in a wasm VM
8890
'limit-segments_disable-bulk-memory.wast',
8991
# https://github.com/WebAssembly/binaryen/issues/7176

‎src/ir/struct-utils.h‎

Lines changed: 49 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,10 @@ struct FunctionStructValuesMap
118118
//
119119
// void noteDefault(Type fieldType, HeapType type, Index index, T& info);
120120
//
121+
// * Note a RMW operation on a field. TODO: Pass more useful information here.
122+
//
123+
// void noteRMW(Expression* expr, HeapType type, Index index, T& info);
124+
//
121125
// * Note a copied value (read from this field and written to the same, possibly
122126
// in another object). Note that we require that the two types (the one read
123127
// from, and written to) are identical; allowing subtyping is possible, but
@@ -140,6 +144,8 @@ struct StructScanner
140144

141145
bool modifiesBinaryenIR() override { return false; }
142146

147+
SubType& self() { return *static_cast<SubType*>(this); }
148+
143149
StructScanner(FunctionStructValuesMap<T>& functionNewInfos,
144150
FunctionStructValuesMap<T>& functionSetGetInfos)
145151
: functionNewInfos(functionNewInfos),
@@ -157,8 +163,7 @@ struct StructScanner
157163
auto& infos = functionNewInfos[this->getFunction()][heapType];
158164
for (Index i = 0; i < fields.size(); i++) {
159165
if (curr->isWithDefault()) {
160-
static_cast<SubType*>(this)->noteDefault(
161-
fields[i].type, heapType, i, infos[i]);
166+
self().noteDefault(fields[i].type, heapType, i, infos[i]);
162167
} else {
163168
noteExpressionOrCopy(curr->operands[i], heapType, i, infos[i]);
164169
}
@@ -187,10 +192,48 @@ struct StructScanner
187192

188193
auto heapType = type.getHeapType();
189194
auto index = curr->index;
190-
static_cast<SubType*>(this)->noteRead(
191-
heapType,
192-
index,
193-
functionSetGetInfos[this->getFunction()][heapType][index]);
195+
self().noteRead(heapType,
196+
index,
197+
functionSetGetInfos[this->getFunction()][heapType][index]);
198+
}
199+
200+
void visitStructRMW(StructRMW* curr) {
201+
auto type = curr->ref->type;
202+
if (type == Type::unreachable || type.isNull()) {
203+
return;
204+
}
205+
206+
auto heapType = type.getHeapType();
207+
auto index = curr->index;
208+
auto& info =
209+
functionSetGetInfos[this->getFunction()][type.getHeapType()][curr->index];
210+
211+
if (curr->op == RMWXchg) {
212+
// An xchg is really like a read and write combined.
213+
self().noteRead(heapType, index, info);
214+
noteExpressionOrCopy(curr->value, heapType, index, info);
215+
return;
216+
}
217+
218+
// Otherwise we don't have a simple expression to describe the written
219+
// value, so fall back to noting an opaque RMW.
220+
self().noteRMW(curr->value, heapType, index, info);
221+
}
222+
223+
void visitStructCmpxchg(StructCmpxchg* curr) {
224+
auto type = curr->ref->type;
225+
if (type == Type::unreachable || type.isNull()) {
226+
return;
227+
}
228+
229+
auto heapType = type.getHeapType();
230+
auto index = curr->index;
231+
auto& info =
232+
functionSetGetInfos[this->getFunction()][type.getHeapType()][curr->index];
233+
234+
// A cmpxchg is like a read and conditional write.
235+
self().noteRead(heapType, index, info);
236+
noteExpressionOrCopy(curr->replacement, heapType, index, info);
194237
}
195238

196239
void

‎src/passes/ConstantFieldPropagation.cpp‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -444,6 +444,13 @@ struct PCVScanner
444444
// Reads do not interest us.
445445
}
446446

447+
void noteRMW(Expression* expr,
448+
HeapType type,
449+
Index index,
450+
PossibleConstantValues& info) {
451+
WASM_UNREACHABLE("TODO");
452+
}
453+
447454
BoolFunctionStructValuesMap& functionCopyInfos;
448455
};
449456

‎src/passes/GlobalTypeOptimization.cpp‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,11 @@ struct FieldInfoScanner
9595
void noteRead(HeapType type, Index index, FieldInfo& info) {
9696
info.noteRead();
9797
}
98+
99+
void noteRMW(Expression* expr, HeapType type, Index index, FieldInfo& info) {
100+
info.noteRead();
101+
info.noteWrite();
102+
}
98103
};
99104

100105
struct GlobalTypeOptimization : public Pass {

‎src/passes/TypeRefining.cpp‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,11 @@ struct FieldInfoScanner
7676
// Nothing to do for a read, we just care about written values.
7777
}
7878

79+
void noteRMW(Expression* expr, HeapType type, Index index, FieldInfo& info) {
80+
// We must not refine past the RMW value type.
81+
info.note(expr->type);
82+
}
83+
7984
Properties::FallthroughBehavior getFallthroughBehavior() {
8085
// Looking at fallthrough values may be dangerous here, because it ignores
8186
// intermediate steps. Consider this:

‎src/wasm/wasm-validator.cpp‎

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3154,20 +3154,23 @@ void FunctionValidator::visitStructCmpxchg(StructCmpxchg* curr) {
31543154
field.mutable_, Mutable, curr, "struct.atomic.rmw field must be mutable");
31553155
shouldBeFalse(
31563156
field.isPacked(), curr, "struct.atomic.rmw field must not be packed");
3157-
bool isEq =
3158-
field.type.isRef() &&
3159-
Type::isSubType(
3160-
field.type,
3161-
Type(HeapTypes::eq.getBasic(field.type.getHeapType().getShared()),
3162-
Nullable));
3163-
if (!shouldBeTrue(field.type == Type::i32 || field.type == Type::i64 || isEq,
3164-
curr,
3165-
"struct.atomic.rmw field type invalid for operation")) {
3157+
3158+
Type expectedExpectedType;
3159+
if (field.type == Type::i32) {
3160+
expectedExpectedType = Type::i32;
3161+
} else if (field.type == Type::i64) {
3162+
expectedExpectedType = Type::i64;
3163+
} else if (field.type.isRef()) {
3164+
expectedExpectedType = Type(
3165+
HeapTypes::eq.getBasic(field.type.getHeapType().getShared()), Nullable);
3166+
} else {
3167+
shouldBeTrue(
3168+
false, curr, "struct.atomic.rmw field type invalid for operation");
31663169
return;
31673170
}
31683171
shouldBeSubType(
31693172
curr->expected->type,
3170-
field.type,
3173+
expectedExpectedType,
31713174
curr,
31723175
"struct.atomic.rmw.cmpxchg expected value must have the proper type");
31733176
shouldBeSubType(

‎test/lit/passes/gto-removals-rmw.wast‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.
2+
;; RUN: foreach %s %t wasm-opt --gto --closed-world -all -S -o - | filecheck %s
3+
4+
;; RMW ops are combined reads and writes, so they keep fields alive and mutable.
5+
(module
6+
;; CHECK: (type $A (shared (struct (field (mut i32)))))
7+
(type $A (shared (struct (mut i32))))
8+
9+
;; CHECK: (type $1 (func (param (ref $A)) (result i32)))
10+
11+
;; CHECK: (func $rmw (type $1) (param $0 (ref $A)) (result i32)
12+
;; CHECK-NEXT: (struct.atomic.rmw.add $A 0
13+
;; CHECK-NEXT: (local.get $0)
14+
;; CHECK-NEXT: (i32.const 0)
15+
;; CHECK-NEXT: )
16+
;; CHECK-NEXT: )
17+
(func $rmw (param (ref $A)) (result i32)
18+
(struct.atomic.rmw.add $A 0
19+
(local.get 0)
20+
(i32.const 0)
21+
)
22+
)
23+
)
24+
25+
;; Even when it is a copy from a field to itself, xchg keeps the field alive.
26+
;; Optimizing this is left to ConstantFieldPropagation.
27+
(module
28+
;; CHECK: (type $A (shared (struct (field (mut i32)))))
29+
(type $A (shared (struct (mut i32))))
30+
31+
;; CHECK: (type $1 (func (param (ref $A) (ref $A)) (result i32)))
32+
33+
;; CHECK: (func $xchg (type $1) (param $0 (ref $A)) (param $1 (ref $A)) (result i32)
34+
;; CHECK-NEXT: (struct.atomic.rmw.xchg $A 0
35+
;; CHECK-NEXT: (local.get $0)
36+
;; CHECK-NEXT: (struct.atomic.get $A 0
37+
;; CHECK-NEXT: (local.get $1)
38+
;; CHECK-NEXT: )
39+
;; CHECK-NEXT: )
40+
;; CHECK-NEXT: )
41+
(func $xchg (param (ref $A) (ref $A)) (result i32)
42+
(struct.atomic.rmw.xchg $A 0
43+
(local.get 0)
44+
(struct.atomic.get $A 0
45+
(local.get 1)
46+
)
47+
)
48+
)
49+
)
50+
51+
;; Same with cmpxchg.
52+
(module
53+
;; CHECK: (type $A (shared (struct (field (mut i32)))))
54+
(type $A (shared (struct (mut i32))))
55+
56+
;; CHECK: (type $1 (func (param (ref $A) i32 (ref $A)) (result i32)))
57+
58+
;; CHECK: (func $cmpxchg (type $1) (param $0 (ref $A)) (param $1 i32) (param $2 (ref $A)) (result i32)
59+
;; CHECK-NEXT: (struct.atomic.rmw.cmpxchg $A 0
60+
;; CHECK-NEXT: (local.get $0)
61+
;; CHECK-NEXT: (local.get $1)
62+
;; CHECK-NEXT: (struct.atomic.get $A 0
63+
;; CHECK-NEXT: (local.get $2)
64+
;; CHECK-NEXT: )
65+
;; CHECK-NEXT: )
66+
;; CHECK-NEXT: )
67+
(func $cmpxchg (param (ref $A) i32 (ref $A)) (result i32)
68+
(struct.atomic.rmw.cmpxchg $A 0
69+
(local.get 0)
70+
(local.get 1)
71+
(struct.atomic.get $A 0
72+
(local.get 2)
73+
)
74+
)
75+
)
76+
)

0 commit comments

Comments
 (0)