Skip to content

Commit 6ddacde

Browse files
tlivelytlively
authored
[NFC] Make MemoryOrder parameters non-optional (#7171)
Update Builder and IRBuilder makeStructGet and makeStructSet functions to require the memory order to be explicitly supplied. This is slightly more verbose, but will reduce the chances that we forget to properly consider synchronization when implementing new features in the future.
1 parent 4d8a933 commit 6ddacde

11 files changed

Lines changed: 36 additions & 31 deletions

File tree

‎src/binaryen-c.cpp‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1752,15 +1752,17 @@ BinaryenExpressionRef BinaryenStructGet(BinaryenModuleRef module,
17521752
bool signed_) {
17531753
return static_cast<Expression*>(
17541754
Builder(*(Module*)module)
1755-
.makeStructGet(index, (Expression*)ref, Type(type), signed_));
1755+
.makeStructGet(
1756+
index, (Expression*)ref, MemoryOrder::Unordered, Type(type), signed_));
17561757
}
17571758
BinaryenExpressionRef BinaryenStructSet(BinaryenModuleRef module,
17581759
BinaryenIndex index,
17591760
BinaryenExpressionRef ref,
17601761
BinaryenExpressionRef value) {
17611762
return static_cast<Expression*>(
17621763
Builder(*(Module*)module)
1763-
.makeStructSet(index, (Expression*)ref, (Expression*)value));
1764+
.makeStructSet(
1765+
index, (Expression*)ref, (Expression*)value, MemoryOrder::Unordered));
17641766
}
17651767
BinaryenExpressionRef BinaryenArrayNew(BinaryenModuleRef module,
17661768
BinaryenHeapType type,

‎src/parser/contexts.h‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -740,15 +740,12 @@ struct NullInstrParserCtx {
740740
HeapTypeT,
741741
FieldIdxT,
742742
bool,
743-
MemoryOrder = MemoryOrder::Unordered) {
743+
MemoryOrder) {
744744
return Ok{};
745745
}
746746
template<typename HeapTypeT>
747-
Result<> makeStructSet(Index,
748-
const std::vector<Annotation>&,
749-
HeapTypeT,
750-
FieldIdxT,
751-
MemoryOrder = MemoryOrder::Unordered) {
747+
Result<> makeStructSet(
748+
Index, const std::vector<Annotation>&, HeapTypeT, FieldIdxT, MemoryOrder) {
752749
return Ok{};
753750
}
754751
template<typename HeapTypeT>

‎src/parser/parsers.h‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2240,7 +2240,8 @@ Result<> makeStructGet(Ctx& ctx,
22402240
CHECK_ERR(type);
22412241
auto field = fieldidx(ctx, *type);
22422242
CHECK_ERR(field);
2243-
return ctx.makeStructGet(pos, annotations, *type, *field, signed_);
2243+
return ctx.makeStructGet(
2244+
pos, annotations, *type, *field, signed_, MemoryOrder::Unordered);
22442245
}
22452246

22462247
template<typename Ctx>
@@ -2264,7 +2265,8 @@ makeStructSet(Ctx& ctx, Index pos, const std::vector<Annotation>& annotations) {
22642265
CHECK_ERR(type);
22652266
auto field = fieldidx(ctx, *type);
22662267
CHECK_ERR(field);
2267-
return ctx.makeStructSet(pos, annotations, *type, *field);
2268+
return ctx.makeStructSet(
2269+
pos, annotations, *type, *field, MemoryOrder::Unordered);
22682270
}
22692271

22702272
template<typename Ctx>

‎src/passes/Heap2Local.cpp‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1066,7 +1066,9 @@ struct Array2Struct : PostWalker<Array2Struct> {
10661066
}
10671067

10681068
// Convert the ArraySet into a StructSet.
1069-
replaceCurrent(builder.makeStructSet(index, curr->ref, curr->value));
1069+
// TODO: Handle atomic array accesses.
1070+
replaceCurrent(builder.makeStructSet(
1071+
index, curr->ref, curr->value, MemoryOrder::Unordered));
10701072
}
10711073

10721074
void visitArrayGet(ArrayGet* curr) {
@@ -1085,8 +1087,9 @@ struct Array2Struct : PostWalker<Array2Struct> {
10851087
}
10861088

10871089
// Convert the ArrayGet into a StructGet.
1088-
replaceCurrent(
1089-
builder.makeStructGet(index, curr->ref, curr->type, curr->signed_));
1090+
// TODO: Handle atomic array accesses.
1091+
replaceCurrent(builder.makeStructGet(
1092+
index, curr->ref, MemoryOrder::Unordered, curr->type, curr->signed_));
10901093
}
10911094

10921095
// Some additional operations need special handling

‎src/passes/J2CLItableMerging.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -280,6 +280,7 @@ struct J2CLItableMerging : public Pass {
280280
replaceCurrent(builder.makeStructGet(
281281
0,
282282
curr->ref,
283+
MemoryOrder::Unordered,
283284
parent.structInfoByITableType[curr->type.getHeapType()]
284285
->javaClass.getStruct()
285286
.fields[0]
@@ -341,4 +342,4 @@ struct J2CLItableMerging : public Pass {
341342
} // anonymous namespace
342343

343344
Pass* createJ2CLItableMergingPass() { return new J2CLItableMerging(); }
344-
} // namespace wasm
345+
} // namespace wasm

‎src/tools/fuzzing/fuzzing.cpp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4493,7 +4493,8 @@ Expression* TranslateToFuzzReader::makeStructGet(Type type) {
44934493
auto [structType, fieldIndex] = pick(structFields);
44944494
auto* ref = makeTrappingRefUse(structType);
44954495
auto signed_ = maybeSignedGet(structType.getStruct().fields[fieldIndex]);
4496-
return builder.makeStructGet(fieldIndex, ref, type, signed_);
4496+
return builder.makeStructGet(
4497+
fieldIndex, ref, MemoryOrder::Unordered, type, signed_);
44974498
}
44984499

44994500
Expression* TranslateToFuzzReader::makeStructSet(Type type) {
@@ -4505,7 +4506,7 @@ Expression* TranslateToFuzzReader::makeStructSet(Type type) {
45054506
auto fieldType = structType.getStruct().fields[fieldIndex].type;
45064507
auto* ref = makeTrappingRefUse(structType);
45074508
auto* value = make(fieldType);
4508-
return builder.makeStructSet(fieldIndex, ref, value);
4509+
return builder.makeStructSet(fieldIndex, ref, value, MemoryOrder::Unordered);
45094510
}
45104511

45114512
// Make a bounds check for an array operation, given a ref + index. An optional

‎src/tools/wasm-ctor-eval.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -957,7 +957,8 @@ struct CtorEvalExternalInterface : EvallingModuleRunner::ExternalInterface {
957957

958958
Expression* set;
959959
if (global.type.isStruct()) {
960-
set = builder.makeStructSet(index, getGlobal, value);
960+
set =
961+
builder.makeStructSet(index, getGlobal, value, MemoryOrder::Unordered);
961962
} else {
962963
set = builder.makeArraySet(
963964
getGlobal, builder.makeConst(int32_t(index)), value);

‎src/wasm-builder.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -938,9 +938,9 @@ class Builder {
938938
}
939939
StructGet* makeStructGet(Index index,
940940
Expression* ref,
941+
MemoryOrder order,
941942
Type type,
942-
bool signed_ = false,
943-
MemoryOrder order = MemoryOrder::Unordered) {
943+
bool signed_ = false) {
944944
auto* ret = wasm.allocator.alloc<StructGet>();
945945
ret->index = index;
946946
ret->ref = ref;
@@ -953,7 +953,7 @@ class Builder {
953953
StructSet* makeStructSet(Index index,
954954
Expression* ref,
955955
Expression* value,
956-
MemoryOrder order = MemoryOrder::Unordered) {
956+
MemoryOrder order) {
957957
auto* ret = wasm.allocator.alloc<StructSet>();
958958
ret->index = index;
959959
ret->ref = ref;

‎src/wasm-ir-builder.h‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -204,13 +204,9 @@ class IRBuilder : public UnifiedExpressionVisitor<IRBuilder, Result<>> {
204204
makeBrOn(Index label, BrOnOp op, Type in = Type::none, Type out = Type::none);
205205
Result<> makeStructNew(HeapType type);
206206
Result<> makeStructNewDefault(HeapType type);
207-
Result<> makeStructGet(HeapType type,
208-
Index field,
209-
bool signed_,
210-
MemoryOrder order = MemoryOrder::Unordered);
211-
Result<> makeStructSet(HeapType type,
212-
Index field,
213-
MemoryOrder order = MemoryOrder::Unordered);
207+
Result<>
208+
makeStructGet(HeapType type, Index field, bool signed_, MemoryOrder order);
209+
Result<> makeStructSet(HeapType type, Index field, MemoryOrder order);
214210
Result<> makeArrayNew(HeapType type);
215211
Result<> makeArrayNewDefault(HeapType type);
216212
Result<> makeArrayNewData(HeapType type, Name data);

‎src/wasm/wasm-binary.cpp‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4172,13 +4172,15 @@ Result<> WasmBinaryReader::readInst() {
41724172
case BinaryConsts::StructGetU: {
41734173
auto type = getIndexedHeapType();
41744174
auto field = getU32LEB();
4175-
return builder.makeStructGet(
4176-
type, field, op == BinaryConsts::StructGetS);
4175+
return builder.makeStructGet(type,
4176+
field,
4177+
op == BinaryConsts::StructGetS,
4178+
MemoryOrder::Unordered);
41774179
}
41784180
case BinaryConsts::StructSet: {
41794181
auto type = getIndexedHeapType();
41804182
auto field = getU32LEB();
4181-
return builder.makeStructSet(type, field);
4183+
return builder.makeStructSet(type, field, MemoryOrder::Unordered);
41824184
}
41834185
case BinaryConsts::ArrayNew:
41844186
return builder.makeArrayNew(getIndexedHeapType());

0 commit comments

Comments
 (0)