Skip to content

Commit 3d0e687

Browse files
kripkenkripken
authored
[Strings] Keep public and private types separate in StringLowering (#6642)
We need StringLowering to modify even public types, as it must replace every single stringref with externref, even if that modifies the ABI. To achieve that we told it that all string-using types were private, which let TypeUpdater update them, but the problem is that it moves all private types to a new single rec group, which meant public and private types ended up in the same group. As a result, a single public type would make it all public, preventing optimizations and breaking things as in #6630 #6640. Ideally TypeUpdater would modify public types while keeping them in the same rec groups, but this may be a very specific issue for StringLowering, and that might be a lot of work. Instead, just make StringLowering handle public types of functions in a manual way, which is simple and should handle all cases that matter in practice, at least in J2Wasm.
1 parent 0a1a59a commit 3d0e687

File tree

3 files changed

+141
-41
lines changed

3 files changed

+141
-41
lines changed

‎src/passes/StringLowering.cpp‎

Lines changed: 39 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,44 @@ struct StringLowering : public StringGathering {
267267
Type nnExt = Type(HeapType::ext, NonNullable);
268268

269269
void updateTypes(Module* module) {
270+
// TypeMapper will not handle public types, but we do want to modify them as
271+
// well: we are modifying the public ABI here. We can't simply tell
272+
// TypeMapper to consider them private, as then they'd end up in the new big
273+
// rec group with the private types (and as they are public, that would make
274+
// the entire rec group public, and all types in the module with it).
275+
// Instead, manually handle singleton-rec groups of function types. This
276+
// keeps them at size 1, as expected, and handles the cases of function
277+
// imports and exports. If we need more (non-function types, non-singleton
278+
// rec groups, etc.) then more work will be necessary TODO
279+
//
280+
// Note that we do this before TypeMapper, which allows it to then fix up
281+
// things like the types of parameters (which depend on the type of the
282+
// function, which must be modified either in TypeMapper - but as just
283+
// explained we cannot do that - or before it, which is what we do here).
284+
for (auto& func : module->functions) {
285+
if (func->type.getRecGroup().size() != 1 ||
286+
!Type(func->type, Nullable).getFeatures().hasStrings()) {
287+
continue;
288+
}
289+
290+
// Fix up the stringrefs in this type that uses strings and is in a
291+
// singleton rec group.
292+
std::vector<Type> params, results;
293+
auto fix = [](Type t) {
294+
if (t.isRef() && t.getHeapType() == HeapType::string) {
295+
t = Type(HeapType::ext, t.getNullability());
296+
}
297+
return t;
298+
};
299+
for (auto param : func->type.getSignature().params) {
300+
params.push_back(fix(param));
301+
}
302+
for (auto result : func->type.getSignature().results) {
303+
results.push_back(fix(result));
304+
}
305+
func->type = Signature(params, results);
306+
}
307+
270308
TypeMapper::TypeUpdates updates;
271309

272310
// Strings turn into externref.
@@ -288,19 +326,7 @@ struct StringLowering : public StringGathering {
288326
}
289327
}
290328

291-
// We consider all types that use strings as modifiable, which means we
292-
// mark them as non-public. That is, we are doing something TypeMapper
293-
// normally does not, as we are changing the external interface/ABI of the
294-
// module: we are changing that ABI from using strings to externs.
295-
auto publicTypes = ModuleUtils::getPublicHeapTypes(*module);
296-
std::vector<HeapType> stringUsers;
297-
for (auto t : publicTypes) {
298-
if (Type(t, Nullable).getFeatures().hasStrings()) {
299-
stringUsers.push_back(t);
300-
}
301-
}
302-
303-
TypeMapper(*module, updates).map(stringUsers);
329+
TypeMapper(*module, updates).map();
304330
}
305331

306332
// Imported string functions.

‎test/lit/passes/string-lowering-instructions.wast‎

Lines changed: 28 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,12 @@
88

99
;; CHECK: (type $1 (func))
1010

11-
;; CHECK: (type $2 (func (param externref externref) (result i32)))
11+
;; CHECK: (type $2 (func (result externref)))
1212

13-
;; CHECK: (rec
14-
;; CHECK-NEXT: (type $3 (func (param externref i32 externref)))
15-
16-
;; CHECK: (type $4 (func (result externref)))
13+
;; CHECK: (type $3 (func (param externref externref) (result i32)))
1714

18-
;; CHECK: (type $5 (func (param externref)))
15+
;; CHECK: (rec
16+
;; CHECK-NEXT: (type $4 (func (param externref)))
1917

2018
;; CHECK: (type $struct-of-string (struct (field externref) (field i32) (field anyref)))
2119
(type $struct-of-string (struct (field stringref) (field i32) (field anyref)))
@@ -39,17 +37,19 @@
3937
(type $array16 (array (mut i16)))
4038
)
4139

42-
;; CHECK: (type $13 (func (param externref) (result externref)))
40+
;; CHECK: (type $12 (func (param externref) (result externref)))
41+
42+
;; CHECK: (type $13 (func (param externref) (result i32)))
4343

44-
;; CHECK: (type $14 (func (param externref) (result i32)))
44+
;; CHECK: (type $14 (func (param externref externref) (result i32)))
4545

46-
;; CHECK: (type $15 (func (param externref externref) (result i32)))
46+
;; CHECK: (type $15 (func (param externref (ref $0)) (result i32)))
4747

48-
;; CHECK: (type $16 (func (param externref (ref $0)) (result i32)))
48+
;; CHECK: (type $16 (func (param externref externref) (result (ref extern))))
4949

50-
;; CHECK: (type $17 (func (param externref externref) (result (ref extern))))
50+
;; CHECK: (type $17 (func (param (ref $0))))
5151

52-
;; CHECK: (type $18 (func (param (ref $0))))
52+
;; CHECK: (type $18 (func (param externref i32 externref)))
5353

5454
;; CHECK: (type $19 (func (param (ref null $0) i32 i32) (result (ref extern))))
5555

@@ -81,9 +81,9 @@
8181

8282
;; CHECK: (import "wasm:js-string" "intoCharCodeArray" (func $intoCharCodeArray (type $22) (param externref (ref null $0) i32) (result i32)))
8383

84-
;; CHECK: (import "wasm:js-string" "equals" (func $equals (type $2) (param externref externref) (result i32)))
84+
;; CHECK: (import "wasm:js-string" "equals" (func $equals (type $3) (param externref externref) (result i32)))
8585

86-
;; CHECK: (import "wasm:js-string" "compare" (func $compare (type $2) (param externref externref) (result i32)))
86+
;; CHECK: (import "wasm:js-string" "compare" (func $compare (type $3) (param externref externref) (result i32)))
8787

8888
;; CHECK: (import "wasm:js-string" "length" (func $length (type $23) (param externref) (result i32)))
8989

@@ -98,7 +98,7 @@
9898

9999
;; CHECK: (export "export.2" (func $exported-string-receiver))
100100

101-
;; CHECK: (func $string.new.gc (type $18) (param $array16 (ref $0))
101+
;; CHECK: (func $string.new.gc (type $17) (param $array16 (ref $0))
102102
;; CHECK-NEXT: (drop
103103
;; CHECK-NEXT: (call $fromCharCodeArray
104104
;; CHECK-NEXT: (local.get $array16)
@@ -117,7 +117,7 @@
117117
)
118118
)
119119

120-
;; CHECK: (func $string.from_code_point (type $4) (result externref)
120+
;; CHECK: (func $string.from_code_point (type $2) (result externref)
121121
;; CHECK-NEXT: (call $fromCodePoint_18
122122
;; CHECK-NEXT: (i32.const 1)
123123
;; CHECK-NEXT: )
@@ -128,7 +128,7 @@
128128
)
129129
)
130130

131-
;; CHECK: (func $string.concat (type $17) (param $0 externref) (param $1 externref) (result (ref extern))
131+
;; CHECK: (func $string.concat (type $16) (param $0 externref) (param $1 externref) (result (ref extern))
132132
;; CHECK-NEXT: (call $concat
133133
;; CHECK-NEXT: (local.get $0)
134134
;; CHECK-NEXT: (local.get $1)
@@ -141,7 +141,7 @@
141141
)
142142
)
143143

144-
;; CHECK: (func $string.encode (type $16) (param $ref externref) (param $array16 (ref $0)) (result i32)
144+
;; CHECK: (func $string.encode (type $15) (param $ref externref) (param $array16 (ref $0)) (result i32)
145145
;; CHECK-NEXT: (call $intoCharCodeArray
146146
;; CHECK-NEXT: (local.get $ref)
147147
;; CHECK-NEXT: (local.get $array16)
@@ -156,7 +156,7 @@
156156
)
157157
)
158158

159-
;; CHECK: (func $string.eq (type $15) (param $a externref) (param $b externref) (result i32)
159+
;; CHECK: (func $string.eq (type $14) (param $a externref) (param $b externref) (result i32)
160160
;; CHECK-NEXT: (call $equals
161161
;; CHECK-NEXT: (local.get $a)
162162
;; CHECK-NEXT: (local.get $b)
@@ -169,7 +169,7 @@
169169
)
170170
)
171171

172-
;; CHECK: (func $string.compare (type $15) (param $a externref) (param $b externref) (result i32)
172+
;; CHECK: (func $string.compare (type $14) (param $a externref) (param $b externref) (result i32)
173173
;; CHECK-NEXT: (call $compare
174174
;; CHECK-NEXT: (local.get $a)
175175
;; CHECK-NEXT: (local.get $b)
@@ -182,7 +182,7 @@
182182
)
183183
)
184184

185-
;; CHECK: (func $string.length (type $14) (param $ref externref) (result i32)
185+
;; CHECK: (func $string.length (type $13) (param $ref externref) (result i32)
186186
;; CHECK-NEXT: (call $length
187187
;; CHECK-NEXT: (local.get $ref)
188188
;; CHECK-NEXT: )
@@ -193,7 +193,7 @@
193193
)
194194
)
195195

196-
;; CHECK: (func $string.get_codeunit (type $14) (param $ref externref) (result i32)
196+
;; CHECK: (func $string.get_codeunit (type $13) (param $ref externref) (result i32)
197197
;; CHECK-NEXT: (call $charCodeAt
198198
;; CHECK-NEXT: (local.get $ref)
199199
;; CHECK-NEXT: (i32.const 2)
@@ -206,7 +206,7 @@
206206
)
207207
)
208208

209-
;; CHECK: (func $string.slice (type $13) (param $ref externref) (result externref)
209+
;; CHECK: (func $string.slice (type $12) (param $ref externref) (result externref)
210210
;; CHECK-NEXT: (call $substring
211211
;; CHECK-NEXT: (local.get $ref)
212212
;; CHECK-NEXT: (i32.const 2)
@@ -221,7 +221,7 @@
221221
)
222222
)
223223

224-
;; CHECK: (func $if.string (type $13) (param $ref externref) (result externref)
224+
;; CHECK: (func $if.string (type $12) (param $ref externref) (result externref)
225225
;; CHECK-NEXT: (if (result externref)
226226
;; CHECK-NEXT: (i32.const 0)
227227
;; CHECK-NEXT: (then
@@ -244,7 +244,7 @@
244244
)
245245
)
246246

247-
;; CHECK: (func $if.string.flip (type $13) (param $ref externref) (result externref)
247+
;; CHECK: (func $if.string.flip (type $12) (param $ref externref) (result externref)
248248
;; CHECK-NEXT: (if (result externref)
249249
;; CHECK-NEXT: (i32.const 0)
250250
;; CHECK-NEXT: (then
@@ -268,7 +268,7 @@
268268
)
269269
)
270270

271-
;; CHECK: (func $exported-string-returner (type $4) (result externref)
271+
;; CHECK: (func $exported-string-returner (type $2) (result externref)
272272
;; CHECK-NEXT: (global.get $string.const_exported)
273273
;; CHECK-NEXT: )
274274
(func $exported-string-returner (export "export.1") (result stringref)
@@ -277,7 +277,7 @@
277277
(string.const "exported")
278278
)
279279

280-
;; CHECK: (func $exported-string-receiver (type $3) (param $x externref) (param $y i32) (param $z externref)
280+
;; CHECK: (func $exported-string-receiver (type $18) (param $x externref) (param $y i32) (param $z externref)
281281
;; CHECK-NEXT: (drop
282282
;; CHECK-NEXT: (local.get $x)
283283
;; CHECK-NEXT: )
@@ -398,7 +398,7 @@
398398
)
399399
)
400400

401-
;; CHECK: (func $call-param-null (type $5) (param $str externref)
401+
;; CHECK: (func $call-param-null (type $4) (param $str externref)
402402
;; CHECK-NEXT: (call $call-param-null
403403
;; CHECK-NEXT: (ref.null noextern)
404404
;; CHECK-NEXT: )

‎test/lit/passes/string-lowering_types.wast‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.
2+
3+
;; RUN: foreach %s %t wasm-opt --string-lowering -all -S -o - | filecheck %s
4+
5+
;; A private type exists, and a public type is used by imports (one explicitly,
6+
;; one implicitly). When we lower stringref into externref the import's types
7+
;; should not be part of a rec group with the private type: public and private
8+
;; types must remain separate.
9+
(module
10+
(rec
11+
;; CHECK: (type $0 (func (param externref externref) (result i32)))
12+
13+
;; CHECK: (type $1 (array (mut i16)))
14+
15+
;; CHECK: (type $2 (func (param externref)))
16+
17+
;; CHECK: (type $3 (func (param (ref extern))))
18+
19+
;; CHECK: (type $4 (func))
20+
21+
;; CHECK: (type $private (struct (field externref)))
22+
(type $private (struct (field stringref)))
23+
)
24+
(type $public (func (param stringref)))
25+
26+
;; CHECK: (type $6 (func (param (ref null $1) i32 i32) (result (ref extern))))
27+
28+
;; CHECK: (type $7 (func (param i32) (result (ref extern))))
29+
30+
;; CHECK: (type $8 (func (param externref externref) (result (ref extern))))
31+
32+
;; CHECK: (type $9 (func (param externref (ref null $1) i32) (result i32)))
33+
34+
;; CHECK: (type $10 (func (param externref) (result i32)))
35+
36+
;; CHECK: (type $11 (func (param externref i32) (result i32)))
37+
38+
;; CHECK: (type $12 (func (param externref i32 i32) (result (ref extern))))
39+
40+
;; CHECK: (import "a" "b" (func $import (type $2) (param externref)))
41+
(import "a" "b" (func $import (type $public) (param stringref)))
42+
43+
;; CHECK: (import "a" "b" (func $import-implicit (type $3) (param (ref extern))))
44+
(import "a" "b" (func $import-implicit (param (ref string))))
45+
46+
;; CHECK: (import "wasm:js-string" "fromCharCodeArray" (func $fromCharCodeArray (type $6) (param (ref null $1) i32 i32) (result (ref extern))))
47+
48+
;; CHECK: (import "wasm:js-string" "fromCodePoint" (func $fromCodePoint (type $7) (param i32) (result (ref extern))))
49+
50+
;; CHECK: (import "wasm:js-string" "concat" (func $concat (type $8) (param externref externref) (result (ref extern))))
51+
52+
;; CHECK: (import "wasm:js-string" "intoCharCodeArray" (func $intoCharCodeArray (type $9) (param externref (ref null $1) i32) (result i32)))
53+
54+
;; CHECK: (import "wasm:js-string" "equals" (func $equals (type $0) (param externref externref) (result i32)))
55+
56+
;; CHECK: (import "wasm:js-string" "compare" (func $compare (type $0) (param externref externref) (result i32)))
57+
58+
;; CHECK: (import "wasm:js-string" "length" (func $length (type $10) (param externref) (result i32)))
59+
60+
;; CHECK: (import "wasm:js-string" "charCodeAt" (func $charCodeAt (type $11) (param externref i32) (result i32)))
61+
62+
;; CHECK: (import "wasm:js-string" "substring" (func $substring (type $12) (param externref i32 i32) (result (ref extern))))
63+
64+
;; CHECK: (export "export" (func $export))
65+
66+
;; CHECK: (func $export (type $4)
67+
;; CHECK-NEXT: (local $0 (ref $private))
68+
;; CHECK-NEXT: (nop)
69+
;; CHECK-NEXT: )
70+
(func $export (export "export")
71+
;; Keep the private type alive.
72+
(local (ref $private))
73+
)
74+
)

0 commit comments

Comments
 (0)