Skip to content

Commit 0b0c338

Browse files
kripkenkripken
authored
Fix ConstantFieldPropagation signed packed field handling and improve Heap2Local's (#6493)
CFP already had logic for truncating but not for sign-extending, which this fixes. Use the new helper function in Heap2Local as well. This changes the model there from "truncate on set, sign-extend on get" to "truncate or sign-extend on get". That is both simpler by reusing the same logic as CFP but also more optimal: the idea to truncate on sets made sense since sets are rarer, but if we must then sign-extend on gets then we can end up doing more work overall (as the truncations on sets are not needed if all gets are signed). Found by #6486
1 parent da8b071 commit 0b0c338

File tree

5 files changed

+135
-59
lines changed

5 files changed

+135
-59
lines changed

‎src/ir/bits.h‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,30 @@ inline Expression* makeSignExt(Expression* value, Index bytes, Module& wasm) {
107107
}
108108
}
109109

110+
// Given a value that is read from a field, as a replacement for a
111+
// StructGet/ArrayGet that we inferred the value of, and given the signedness of
112+
// the get and the field info, if we are doing a signed read of a packed field
113+
// then sign-extend it, or if it is unsigned then truncate. This fixes up cases
114+
// where we can replace the StructGet/ArrayGet with the value we know must be
115+
// there (without making any assumptions about |value|, that is, we do not
116+
// assume it has been truncated already).
117+
inline Expression* makePackedFieldGet(Expression* value,
118+
const Field& field,
119+
bool signed_,
120+
Module& wasm) {
121+
if (!field.isPacked()) {
122+
return value;
123+
}
124+
125+
if (signed_) {
126+
return makeSignExt(value, field.getByteSize(), wasm);
127+
}
128+
129+
Builder builder(wasm);
130+
auto mask = Bits::lowBitMask(field.getByteSize() * 8);
131+
return builder.makeBinary(AndInt32, value, builder.makeConst(int32_t(mask)));
132+
}
133+
110134
// getMaxBits() helper that has pessimistic results for the bits used in locals.
111135
struct DummyLocalInfoProvider {
112136
Index getMaxBitsForLocal(LocalGet* get) {

‎src/passes/ConstantFieldPropagation.cpp‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -125,13 +125,8 @@ struct FunctionOptimizer : public WalkerPass<PostWalker<FunctionOptimizer>> {
125125
Expression* value = info.makeExpression(*getModule());
126126
auto field = GCTypeUtils::getField(type, curr->index);
127127
assert(field);
128-
if (field->isPacked()) {
129-
// We cannot just pass through a value that is packed, as the input gets
130-
// truncated.
131-
auto mask = Bits::lowBitMask(field->getByteSize() * 8);
132-
value =
133-
builder.makeBinary(AndInt32, value, builder.makeConst(int32_t(mask)));
134-
}
128+
value =
129+
Bits::makePackedFieldGet(value, *field, curr->signed_, *getModule());
135130
replaceCurrent(builder.makeSequence(
136131
builder.makeDrop(builder.makeRefAs(RefAsNonNull, curr->ref)), value));
137132
changed = true;

‎src/passes/Heap2Local.cpp‎

Lines changed: 8 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -649,21 +649,6 @@ struct Struct2Local : PostWalker<Struct2Local> {
649649
curr->finalize();
650650
}
651651

652-
// Add a mask for packed fields. We add masks on sets rather than on gets
653-
// because gets tend to be more numerous both in code appearances and in
654-
// runtime execution. As a result of masking on sets, the value in the local
655-
// is always the masked unsigned value (which is also nice for debugging,
656-
// incidentally).
657-
Expression* addMask(Expression* value, const Field& field) {
658-
if (!field.isPacked()) {
659-
return value;
660-
}
661-
662-
auto mask = Bits::lowBitMask(field.getByteSize() * 8);
663-
return builder.makeBinary(
664-
AndInt32, value, builder.makeConst(int32_t(mask)));
665-
}
666-
667652
void visitStructNew(StructNew* curr) {
668653
if (curr != allocation) {
669654
return;
@@ -710,9 +695,7 @@ struct Struct2Local : PostWalker<Struct2Local> {
710695
// Copy them to the normal ones.
711696
for (Index i = 0; i < tempIndexes.size(); i++) {
712697
auto* value = builder.makeLocalGet(tempIndexes[i], fields[i].type);
713-
// Add a mask on the values we write.
714-
contents.push_back(
715-
builder.makeLocalSet(localIndexes[i], addMask(value, fields[i])));
698+
contents.push_back(builder.makeLocalSet(localIndexes[i], value));
716699
}
717700

718701
// TODO Check if the nondefault case does not increase code size in some
@@ -724,8 +707,6 @@ struct Struct2Local : PostWalker<Struct2Local> {
724707
//
725708
// Note that we must assign the defaults because we might be in a loop,
726709
// that is, there might be a previous value.
727-
//
728-
// Note there is no need to mask as these are zeros anyhow.
729710
for (Index i = 0; i < localIndexes.size(); i++) {
730711
contents.push_back(builder.makeLocalSet(
731712
localIndexes[i],
@@ -786,8 +767,7 @@ struct Struct2Local : PostWalker<Struct2Local> {
786767
// write the data to the local instead of the heap allocation.
787768
replaceCurrent(builder.makeSequence(
788769
builder.makeDrop(curr->ref),
789-
builder.makeLocalSet(localIndexes[curr->index],
790-
addMask(curr->value, fields[curr->index]))));
770+
builder.makeLocalSet(localIndexes[curr->index], curr->value)));
791771
}
792772

793773
void visitStructGet(StructGet* curr) {
@@ -813,11 +793,12 @@ struct Struct2Local : PostWalker<Struct2Local> {
813793
refinalize = true;
814794
}
815795
Expression* value = builder.makeLocalGet(localIndexes[curr->index], type);
816-
if (field.isPacked() && curr->signed_) {
817-
// The value in the local is the masked unsigned value, which we must
818-
// sign-extend.
819-
value = Bits::makeSignExt(value, field.getByteSize(), wasm);
820-
}
796+
// Note that in theory we could try to do better here than to fix up the
797+
// packing and signedness on gets: we could truncate on sets. That would be
798+
// more efficient if all gets are unsigned, as gets outnumber sets in
799+
// general. However, signed gets make that more complicated, so leave this
800+
// for other opts to handle.
801+
value = Bits::makePackedFieldGet(value, field, curr->signed_, wasm);
821802
replaceCurrent(builder.makeSequence(builder.makeDrop(curr->ref), value));
822803
}
823804
};

‎test/lit/passes/cfp.wast‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2228,6 +2228,87 @@
22282228
)
22292229
)
22302230
)
2231+
2232+
;; CHECK: (func $test_signed (type $0)
2233+
;; CHECK-NEXT: (drop
2234+
;; CHECK-NEXT: (block (result i32)
2235+
;; CHECK-NEXT: (drop
2236+
;; CHECK-NEXT: (ref.as_non_null
2237+
;; CHECK-NEXT: (struct.new $A_8
2238+
;; CHECK-NEXT: (i32.const 305419896)
2239+
;; CHECK-NEXT: )
2240+
;; CHECK-NEXT: )
2241+
;; CHECK-NEXT: )
2242+
;; CHECK-NEXT: (i32.shr_s
2243+
;; CHECK-NEXT: (i32.shl
2244+
;; CHECK-NEXT: (i32.const 305419896)
2245+
;; CHECK-NEXT: (i32.const 24)
2246+
;; CHECK-NEXT: )
2247+
;; CHECK-NEXT: (i32.const 24)
2248+
;; CHECK-NEXT: )
2249+
;; CHECK-NEXT: )
2250+
;; CHECK-NEXT: )
2251+
;; CHECK-NEXT: (drop
2252+
;; CHECK-NEXT: (block (result i32)
2253+
;; CHECK-NEXT: (drop
2254+
;; CHECK-NEXT: (ref.as_non_null
2255+
;; CHECK-NEXT: (struct.new $A_16
2256+
;; CHECK-NEXT: (i32.const 305419896)
2257+
;; CHECK-NEXT: )
2258+
;; CHECK-NEXT: )
2259+
;; CHECK-NEXT: )
2260+
;; CHECK-NEXT: (i32.shr_s
2261+
;; CHECK-NEXT: (i32.shl
2262+
;; CHECK-NEXT: (i32.const 305419896)
2263+
;; CHECK-NEXT: (i32.const 16)
2264+
;; CHECK-NEXT: )
2265+
;; CHECK-NEXT: (i32.const 16)
2266+
;; CHECK-NEXT: )
2267+
;; CHECK-NEXT: )
2268+
;; CHECK-NEXT: )
2269+
;; CHECK-NEXT: (drop
2270+
;; CHECK-NEXT: (block (result i32)
2271+
;; CHECK-NEXT: (drop
2272+
;; CHECK-NEXT: (ref.as_non_null
2273+
;; CHECK-NEXT: (struct.new $B_16
2274+
;; CHECK-NEXT: (global.get $g)
2275+
;; CHECK-NEXT: )
2276+
;; CHECK-NEXT: )
2277+
;; CHECK-NEXT: )
2278+
;; CHECK-NEXT: (i32.shr_s
2279+
;; CHECK-NEXT: (i32.shl
2280+
;; CHECK-NEXT: (global.get $g)
2281+
;; CHECK-NEXT: (i32.const 16)
2282+
;; CHECK-NEXT: )
2283+
;; CHECK-NEXT: (i32.const 16)
2284+
;; CHECK-NEXT: )
2285+
;; CHECK-NEXT: )
2286+
;; CHECK-NEXT: )
2287+
;; CHECK-NEXT: )
2288+
(func $test_signed
2289+
;; As above, but with signed gets.
2290+
(drop
2291+
(struct.get_s $A_8 0
2292+
(struct.new $A_8
2293+
(i32.const 0x12345678)
2294+
)
2295+
)
2296+
)
2297+
(drop
2298+
(struct.get_s $A_16 0
2299+
(struct.new $A_16
2300+
(i32.const 0x12345678)
2301+
)
2302+
)
2303+
)
2304+
(drop
2305+
(struct.get_s $B_16 0
2306+
(struct.new $B_16
2307+
(global.get $g)
2308+
)
2309+
)
2310+
)
2311+
)
22312312
)
22322313

22332314
(module

‎test/lit/passes/heap2local.wast‎

Lines changed: 20 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -191,10 +191,7 @@
191191
;; CHECK-NEXT: (i32.const 1338)
192192
;; CHECK-NEXT: )
193193
;; CHECK-NEXT: (local.set $1
194-
;; CHECK-NEXT: (i32.and
195-
;; CHECK-NEXT: (local.get $3)
196-
;; CHECK-NEXT: (i32.const 255)
197-
;; CHECK-NEXT: )
194+
;; CHECK-NEXT: (local.get $3)
198195
;; CHECK-NEXT: )
199196
;; CHECK-NEXT: (local.set $2
200197
;; CHECK-NEXT: (local.get $4)
@@ -207,18 +204,18 @@
207204
;; CHECK-NEXT: (ref.null none)
208205
;; CHECK-NEXT: )
209206
;; CHECK-NEXT: (local.set $1
210-
;; CHECK-NEXT: (i32.and
211-
;; CHECK-NEXT: (i32.const 99998)
212-
;; CHECK-NEXT: (i32.const 255)
213-
;; CHECK-NEXT: )
207+
;; CHECK-NEXT: (i32.const 99998)
214208
;; CHECK-NEXT: )
215209
;; CHECK-NEXT: )
216210
;; CHECK-NEXT: (drop
217211
;; CHECK-NEXT: (block (result i32)
218212
;; CHECK-NEXT: (drop
219213
;; CHECK-NEXT: (ref.null none)
220214
;; CHECK-NEXT: )
221-
;; CHECK-NEXT: (local.get $1)
215+
;; CHECK-NEXT: (i32.and
216+
;; CHECK-NEXT: (local.get $1)
217+
;; CHECK-NEXT: (i32.const 255)
218+
;; CHECK-NEXT: )
222219
;; CHECK-NEXT: )
223220
;; CHECK-NEXT: )
224221
;; CHECK-NEXT: (drop
@@ -265,8 +262,6 @@
265262
;; CHECK-NEXT: )
266263
(func $packed
267264
(local $temp (ref $struct.packed))
268-
;; Packed fields require masking of irrelevant bits, which we apply on the
269-
;; sets.
270265
(local.set $temp
271266
(struct.new $struct.packed
272267
(i32.const 1337)
@@ -277,12 +272,13 @@
277272
(local.get $temp)
278273
(i32.const 99998)
279274
)
275+
;; Packed fields require masking of irrelevant bits on unsigned gets and
276+
;; sign-extending on signed ones.
280277
(drop
281278
(struct.get $struct.packed 0
282279
(local.get $temp)
283280
)
284281
)
285-
;; Signed gets require us to sign-extend them.
286282
(drop
287283
(struct.get_s $struct.packed 0
288284
(local.get $temp)
@@ -298,8 +294,7 @@
298294
(local.get $temp)
299295
)
300296
)
301-
;; When using struct.new_default we do not need any masking, as the values
302-
;; written are 0 anyhow.
297+
;; Check we do not add any masking in new_default either.
303298
(local.set $temp
304299
(struct.new_default $struct.packed)
305300
)
@@ -3463,18 +3458,18 @@
34633458
;; CHECK-NEXT: (ref.null none)
34643459
;; CHECK-NEXT: )
34653460
;; CHECK-NEXT: (local.set $2
3466-
;; CHECK-NEXT: (i32.and
3467-
;; CHECK-NEXT: (i32.const 1337)
3468-
;; CHECK-NEXT: (i32.const 255)
3469-
;; CHECK-NEXT: )
3461+
;; CHECK-NEXT: (i32.const 1337)
34703462
;; CHECK-NEXT: )
34713463
;; CHECK-NEXT: )
34723464
;; CHECK-NEXT: (drop
34733465
;; CHECK-NEXT: (block (result i32)
34743466
;; CHECK-NEXT: (drop
34753467
;; CHECK-NEXT: (ref.null none)
34763468
;; CHECK-NEXT: )
3477-
;; CHECK-NEXT: (local.get $2)
3469+
;; CHECK-NEXT: (i32.and
3470+
;; CHECK-NEXT: (local.get $2)
3471+
;; CHECK-NEXT: (i32.const 255)
3472+
;; CHECK-NEXT: )
34783473
;; CHECK-NEXT: )
34793474
;; CHECK-NEXT: )
34803475
;; CHECK-NEXT: (drop
@@ -3504,13 +3499,13 @@
35043499
(i32.const 1)
35053500
(i32.const 1337)
35063501
)
3502+
;; As with structs, we truncate/sign-extend gets of packed fields.
35073503
(drop
35083504
(array.get $array8
35093505
(local.get $temp)
35103506
(i32.const 1)
35113507
)
35123508
)
3513-
;; As with structs, a signed get causes us to emit a sign extend.
35143509
(drop
35153510
(array.get_s $array8
35163511
(local.get $temp)
@@ -3543,17 +3538,17 @@
35433538
;; CHECK-NEXT: (ref.null none)
35443539
;; CHECK-NEXT: )
35453540
;; CHECK-NEXT: (local.set $2
3546-
;; CHECK-NEXT: (i32.and
3547-
;; CHECK-NEXT: (i32.const 1337)
3548-
;; CHECK-NEXT: (i32.const 65535)
3549-
;; CHECK-NEXT: )
3541+
;; CHECK-NEXT: (i32.const 1337)
35503542
;; CHECK-NEXT: )
35513543
;; CHECK-NEXT: )
35523544
;; CHECK-NEXT: (block (result i32)
35533545
;; CHECK-NEXT: (drop
35543546
;; CHECK-NEXT: (ref.null none)
35553547
;; CHECK-NEXT: )
3556-
;; CHECK-NEXT: (local.get $2)
3548+
;; CHECK-NEXT: (i32.and
3549+
;; CHECK-NEXT: (local.get $2)
3550+
;; CHECK-NEXT: (i32.const 65535)
3551+
;; CHECK-NEXT: )
35573552
;; CHECK-NEXT: )
35583553
;; CHECK-NEXT: )
35593554
(func $array16 (result i32)

0 commit comments

Comments
 (0)