From bb999ec1e98e7a22a2d1f3e155c01550f1cc0446 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 4 Jun 2024 11:57:14 -0700 Subject: [PATCH 1/5] test --- test/lit/passes/string-lowering_types.wast | 73 ++++++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100644 test/lit/passes/string-lowering_types.wast diff --git a/test/lit/passes/string-lowering_types.wast b/test/lit/passes/string-lowering_types.wast new file mode 100644 index 00000000000..118f1f59f7d --- /dev/null +++ b/test/lit/passes/string-lowering_types.wast @@ -0,0 +1,73 @@ +;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. + +;; RUN: foreach %s %t wasm-opt --string-lowering -all -S -o - | filecheck %s + +;; A private type exists, and a public type is used by imports (one explicitly, +;; one implicitly). When we lower stringref into externref the import's types +;; should not be part of a rec group with the private type: public and private +;; types must remain separate. +(module + (rec + ;; CHECK: (type $0 (func (param externref externref) (result i32))) + + ;; CHECK: (type $1 (array (mut i16))) + + ;; CHECK: (rec + ;; CHECK-NEXT: (type $public (func (param externref))) + + ;; CHECK: (type $private (struct (field externref))) + (type $private (struct (field stringref))) + ) + (type $public (func (param stringref))) + + ;; CHECK: (type $4 (func)) + + ;; CHECK: (type $5 (func (param (ref null $1) i32 i32) (result (ref extern)))) + + ;; CHECK: (type $6 (func (param i32) (result (ref extern)))) + + ;; CHECK: (type $7 (func (param externref externref) (result (ref extern)))) + + ;; CHECK: (type $8 (func (param externref (ref null $1) i32) (result i32))) + + ;; CHECK: (type $9 (func (param externref) (result i32))) + + ;; CHECK: (type $10 (func (param externref i32) (result i32))) + + ;; CHECK: (type $11 (func (param externref i32 i32) (result (ref extern)))) + + ;; CHECK: (import "a" "b" (func $import (type $public) (param externref))) + (import "a" "b" (func $import (type $public) (param stringref))) + + ;; CHECK: (import "a" "b" (func $import-implicit (type $public) (param externref))) + (import "a" "b" (func $import-implicit (param stringref))) + + ;; CHECK: (import "wasm:js-string" "fromCharCodeArray" (func $fromCharCodeArray (type $5) (param (ref null $1) i32 i32) (result (ref extern)))) + + ;; CHECK: (import "wasm:js-string" "fromCodePoint" (func $fromCodePoint (type $6) (param i32) (result (ref extern)))) + + ;; CHECK: (import "wasm:js-string" "concat" (func $concat (type $7) (param externref externref) (result (ref extern)))) + + ;; CHECK: (import "wasm:js-string" "intoCharCodeArray" (func $intoCharCodeArray (type $8) (param externref (ref null $1) i32) (result i32))) + + ;; CHECK: (import "wasm:js-string" "equals" (func $equals (type $0) (param externref externref) (result i32))) + + ;; CHECK: (import "wasm:js-string" "compare" (func $compare (type $0) (param externref externref) (result i32))) + + ;; CHECK: (import "wasm:js-string" "length" (func $length (type $9) (param externref) (result i32))) + + ;; CHECK: (import "wasm:js-string" "charCodeAt" (func $charCodeAt (type $10) (param externref i32) (result i32))) + + ;; CHECK: (import "wasm:js-string" "substring" (func $substring (type $11) (param externref i32 i32) (result (ref extern)))) + + ;; CHECK: (export "export" (func $export)) + + ;; CHECK: (func $export (type $4) + ;; CHECK-NEXT: (local $0 (ref $private)) + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: ) + (func $export (export "export") + ;; Keep the private type alive. + (local (ref $private)) + ) +) From 77381933658683c9eec36589e30b649ff485ced1 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 4 Jun 2024 13:17:02 -0700 Subject: [PATCH 2/5] fix --- src/passes/StringLowering.cpp | 43 +++++++++++++++------ test/lit/passes/string-lowering_types.wast | 45 +++++++++++----------- 2 files changed, 55 insertions(+), 33 deletions(-) diff --git a/src/passes/StringLowering.cpp b/src/passes/StringLowering.cpp index 9903f090e30..66326304d29 100644 --- a/src/passes/StringLowering.cpp +++ b/src/passes/StringLowering.cpp @@ -288,19 +288,40 @@ struct StringLowering : public StringGathering { } } - // We consider all types that use strings as modifiable, which means we - // mark them as non-public. That is, we are doing something TypeMapper - // normally does not, as we are changing the external interface/ABI of the - // module: we are changing that ABI from using strings to externs. - auto publicTypes = ModuleUtils::getPublicHeapTypes(*module); - std::vector stringUsers; - for (auto t : publicTypes) { - if (Type(t, Nullable).getFeatures().hasStrings()) { - stringUsers.push_back(t); + TypeMapper(*module, updates).map(); + + // TypeMapper will not handle public types, but we do want to modify them as + // well: we are modifying the public ABI here. We can't simply tell + // TypeMapper to consider them private, as then they'd end up in the new big + // rec group with the private types (and as they are public, that would make + // the entire rec group public, and all types in the module with it). + // Instead, manually handle singleton-rec groups of function types. This + // keeps them at size 1, as expected, and handles the cases of function + // imports and exports. If we need more (non-function types, non-singleton + // rec groups then more work will be necessary TODO + for (auto& func : module->functions) { + if (func->type.getRecGroup().size() != 1 || + !Type(func->type, Nullable).getFeatures().hasStrings()) { + continue; } - } - TypeMapper(*module, updates).map(stringUsers); + // Fix up the stringrefs in this type that uses strings and is in a + // singleton rec group. + std::vector params, results; + auto fix = [](Type t) { + if (t.isRef() && t.getHeapType() == HeapType::string) { + t = Type(HeapType::ext, t.getNullability()); + } + return t; + }; + for (auto param : func->type.getSignature().params) { + params.push_back(fix(param)); + } + for (auto result : func->type.getSignature().results) { + results.push_back(fix(result)); + } + func->type = Signature(params, results); + } } // Imported string functions. diff --git a/test/lit/passes/string-lowering_types.wast b/test/lit/passes/string-lowering_types.wast index 118f1f59f7d..54367e85af5 100644 --- a/test/lit/passes/string-lowering_types.wast +++ b/test/lit/passes/string-lowering_types.wast @@ -12,53 +12,54 @@ ;; CHECK: (type $1 (array (mut i16))) - ;; CHECK: (rec - ;; CHECK-NEXT: (type $public (func (param externref))) + ;; CHECK: (type $2 (func (param externref))) - ;; CHECK: (type $private (struct (field externref))) + ;; CHECK: (type $3 (func (param (ref extern)))) + + ;; CHECK: (type $4 (func)) + + ;; CHECK: (type $private (struct (field externref))) (type $private (struct (field stringref))) ) (type $public (func (param stringref))) - ;; CHECK: (type $4 (func)) - - ;; CHECK: (type $5 (func (param (ref null $1) i32 i32) (result (ref extern)))) + ;; CHECK: (type $6 (func (param (ref null $1) i32 i32) (result (ref extern)))) - ;; CHECK: (type $6 (func (param i32) (result (ref extern)))) + ;; CHECK: (type $7 (func (param i32) (result (ref extern)))) - ;; CHECK: (type $7 (func (param externref externref) (result (ref extern)))) + ;; CHECK: (type $8 (func (param externref externref) (result (ref extern)))) - ;; CHECK: (type $8 (func (param externref (ref null $1) i32) (result i32))) + ;; CHECK: (type $9 (func (param externref (ref null $1) i32) (result i32))) - ;; CHECK: (type $9 (func (param externref) (result i32))) + ;; CHECK: (type $10 (func (param externref) (result i32))) - ;; CHECK: (type $10 (func (param externref i32) (result i32))) + ;; CHECK: (type $11 (func (param externref i32) (result i32))) - ;; CHECK: (type $11 (func (param externref i32 i32) (result (ref extern)))) + ;; CHECK: (type $12 (func (param externref i32 i32) (result (ref extern)))) - ;; CHECK: (import "a" "b" (func $import (type $public) (param externref))) + ;; CHECK: (import "a" "b" (func $import (type $2) (param externref))) (import "a" "b" (func $import (type $public) (param stringref))) - ;; CHECK: (import "a" "b" (func $import-implicit (type $public) (param externref))) - (import "a" "b" (func $import-implicit (param stringref))) + ;; CHECK: (import "a" "b" (func $import-implicit (type $3) (param (ref extern)))) + (import "a" "b" (func $import-implicit (param (ref string)))) - ;; CHECK: (import "wasm:js-string" "fromCharCodeArray" (func $fromCharCodeArray (type $5) (param (ref null $1) i32 i32) (result (ref extern)))) + ;; CHECK: (import "wasm:js-string" "fromCharCodeArray" (func $fromCharCodeArray (type $6) (param (ref null $1) i32 i32) (result (ref extern)))) - ;; CHECK: (import "wasm:js-string" "fromCodePoint" (func $fromCodePoint (type $6) (param i32) (result (ref extern)))) + ;; CHECK: (import "wasm:js-string" "fromCodePoint" (func $fromCodePoint (type $7) (param i32) (result (ref extern)))) - ;; CHECK: (import "wasm:js-string" "concat" (func $concat (type $7) (param externref externref) (result (ref extern)))) + ;; CHECK: (import "wasm:js-string" "concat" (func $concat (type $8) (param externref externref) (result (ref extern)))) - ;; CHECK: (import "wasm:js-string" "intoCharCodeArray" (func $intoCharCodeArray (type $8) (param externref (ref null $1) i32) (result i32))) + ;; CHECK: (import "wasm:js-string" "intoCharCodeArray" (func $intoCharCodeArray (type $9) (param externref (ref null $1) i32) (result i32))) ;; CHECK: (import "wasm:js-string" "equals" (func $equals (type $0) (param externref externref) (result i32))) ;; CHECK: (import "wasm:js-string" "compare" (func $compare (type $0) (param externref externref) (result i32))) - ;; CHECK: (import "wasm:js-string" "length" (func $length (type $9) (param externref) (result i32))) + ;; CHECK: (import "wasm:js-string" "length" (func $length (type $10) (param externref) (result i32))) - ;; CHECK: (import "wasm:js-string" "charCodeAt" (func $charCodeAt (type $10) (param externref i32) (result i32))) + ;; CHECK: (import "wasm:js-string" "charCodeAt" (func $charCodeAt (type $11) (param externref i32) (result i32))) - ;; CHECK: (import "wasm:js-string" "substring" (func $substring (type $11) (param externref i32 i32) (result (ref extern)))) + ;; CHECK: (import "wasm:js-string" "substring" (func $substring (type $12) (param externref i32 i32) (result (ref extern)))) ;; CHECK: (export "export" (func $export)) From 692163a5a6b2463c86da92e866e18fa7d48048de Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 4 Jun 2024 13:21:37 -0700 Subject: [PATCH 3/5] fix --- src/passes/StringLowering.cpp | 46 +++++++-------- .../passes/string-lowering-instructions.wast | 56 +++++++++---------- 2 files changed, 51 insertions(+), 51 deletions(-) diff --git a/src/passes/StringLowering.cpp b/src/passes/StringLowering.cpp index 66326304d29..375884b1750 100644 --- a/src/passes/StringLowering.cpp +++ b/src/passes/StringLowering.cpp @@ -267,29 +267,6 @@ struct StringLowering : public StringGathering { Type nnExt = Type(HeapType::ext, NonNullable); void updateTypes(Module* module) { - TypeMapper::TypeUpdates updates; - - // Strings turn into externref. - updates[HeapType::string] = HeapType::ext; - - // The module may have its own array16 type inside a big rec group, but - // imported strings expects that type in its own rec group as part of the - // ABI. Fix that up here. (This is valid to do as this type has no sub- or - // super-types anyhow; it is "plain old data" for communicating with the - // outside.) - auto allTypes = ModuleUtils::collectHeapTypes(*module); - auto array16 = nullArray16.getHeapType(); - auto array16Element = array16.getArray().element; - for (auto type : allTypes) { - // Match an array type with no super and that is closed. - if (type.isArray() && !type.getDeclaredSuperType() && !type.isOpen() && - type.getArray().element == array16Element) { - updates[type] = array16; - } - } - - TypeMapper(*module, updates).map(); - // TypeMapper will not handle public types, but we do want to modify them as // well: we are modifying the public ABI here. We can't simply tell // TypeMapper to consider them private, as then they'd end up in the new big @@ -322,6 +299,29 @@ struct StringLowering : public StringGathering { } func->type = Signature(params, results); } + + TypeMapper::TypeUpdates updates; + + // Strings turn into externref. + updates[HeapType::string] = HeapType::ext; + + // The module may have its own array16 type inside a big rec group, but + // imported strings expects that type in its own rec group as part of the + // ABI. Fix that up here. (This is valid to do as this type has no sub- or + // super-types anyhow; it is "plain old data" for communicating with the + // outside.) + auto allTypes = ModuleUtils::collectHeapTypes(*module); + auto array16 = nullArray16.getHeapType(); + auto array16Element = array16.getArray().element; + for (auto type : allTypes) { + // Match an array type with no super and that is closed. + if (type.isArray() && !type.getDeclaredSuperType() && !type.isOpen() && + type.getArray().element == array16Element) { + updates[type] = array16; + } + } + + TypeMapper(*module, updates).map(); } // Imported string functions. diff --git a/test/lit/passes/string-lowering-instructions.wast b/test/lit/passes/string-lowering-instructions.wast index b98b9af233b..459f1817019 100644 --- a/test/lit/passes/string-lowering-instructions.wast +++ b/test/lit/passes/string-lowering-instructions.wast @@ -8,14 +8,12 @@ ;; CHECK: (type $1 (func)) - ;; CHECK: (type $2 (func (param externref externref) (result i32))) + ;; CHECK: (type $2 (func (result externref))) - ;; CHECK: (rec - ;; CHECK-NEXT: (type $3 (func (param externref i32 externref))) - - ;; CHECK: (type $4 (func (result externref))) + ;; CHECK: (type $3 (func (param externref externref) (result i32))) - ;; CHECK: (type $5 (func (param externref))) + ;; CHECK: (rec + ;; CHECK-NEXT: (type $4 (func (param externref))) ;; CHECK: (type $struct-of-string (struct (field externref) (field i32) (field anyref))) (type $struct-of-string (struct (field stringref) (field i32) (field anyref))) @@ -39,17 +37,19 @@ (type $array16 (array (mut i16))) ) - ;; CHECK: (type $13 (func (param externref) (result externref))) + ;; CHECK: (type $12 (func (param externref) (result externref))) + + ;; CHECK: (type $13 (func (param externref) (result i32))) - ;; CHECK: (type $14 (func (param externref) (result i32))) + ;; CHECK: (type $14 (func (param externref externref) (result i32))) - ;; CHECK: (type $15 (func (param externref externref) (result i32))) + ;; CHECK: (type $15 (func (param externref (ref $0)) (result i32))) - ;; CHECK: (type $16 (func (param externref (ref $0)) (result i32))) + ;; CHECK: (type $16 (func (param externref externref) (result (ref extern)))) - ;; CHECK: (type $17 (func (param externref externref) (result (ref extern)))) + ;; CHECK: (type $17 (func (param (ref $0)))) - ;; CHECK: (type $18 (func (param (ref $0)))) + ;; CHECK: (type $18 (func (param externref i32 externref))) ;; CHECK: (type $19 (func (param (ref null $0) i32 i32) (result (ref extern)))) @@ -81,9 +81,9 @@ ;; CHECK: (import "wasm:js-string" "intoCharCodeArray" (func $intoCharCodeArray (type $22) (param externref (ref null $0) i32) (result i32))) - ;; CHECK: (import "wasm:js-string" "equals" (func $equals (type $2) (param externref externref) (result i32))) + ;; CHECK: (import "wasm:js-string" "equals" (func $equals (type $3) (param externref externref) (result i32))) - ;; CHECK: (import "wasm:js-string" "compare" (func $compare (type $2) (param externref externref) (result i32))) + ;; CHECK: (import "wasm:js-string" "compare" (func $compare (type $3) (param externref externref) (result i32))) ;; CHECK: (import "wasm:js-string" "length" (func $length (type $23) (param externref) (result i32))) @@ -98,7 +98,7 @@ ;; CHECK: (export "export.2" (func $exported-string-receiver)) - ;; CHECK: (func $string.new.gc (type $18) (param $array16 (ref $0)) + ;; CHECK: (func $string.new.gc (type $17) (param $array16 (ref $0)) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (call $fromCharCodeArray ;; CHECK-NEXT: (local.get $array16) @@ -117,7 +117,7 @@ ) ) - ;; CHECK: (func $string.from_code_point (type $4) (result externref) + ;; CHECK: (func $string.from_code_point (type $2) (result externref) ;; CHECK-NEXT: (call $fromCodePoint_18 ;; CHECK-NEXT: (i32.const 1) ;; CHECK-NEXT: ) @@ -128,7 +128,7 @@ ) ) - ;; CHECK: (func $string.concat (type $17) (param $0 externref) (param $1 externref) (result (ref extern)) + ;; CHECK: (func $string.concat (type $16) (param $0 externref) (param $1 externref) (result (ref extern)) ;; CHECK-NEXT: (call $concat ;; CHECK-NEXT: (local.get $0) ;; CHECK-NEXT: (local.get $1) @@ -141,7 +141,7 @@ ) ) - ;; CHECK: (func $string.encode (type $16) (param $ref externref) (param $array16 (ref $0)) (result i32) + ;; CHECK: (func $string.encode (type $15) (param $ref externref) (param $array16 (ref $0)) (result i32) ;; CHECK-NEXT: (call $intoCharCodeArray ;; CHECK-NEXT: (local.get $ref) ;; CHECK-NEXT: (local.get $array16) @@ -156,7 +156,7 @@ ) ) - ;; CHECK: (func $string.eq (type $15) (param $a externref) (param $b externref) (result i32) + ;; CHECK: (func $string.eq (type $14) (param $a externref) (param $b externref) (result i32) ;; CHECK-NEXT: (call $equals ;; CHECK-NEXT: (local.get $a) ;; CHECK-NEXT: (local.get $b) @@ -169,7 +169,7 @@ ) ) - ;; CHECK: (func $string.compare (type $15) (param $a externref) (param $b externref) (result i32) + ;; CHECK: (func $string.compare (type $14) (param $a externref) (param $b externref) (result i32) ;; CHECK-NEXT: (call $compare ;; CHECK-NEXT: (local.get $a) ;; CHECK-NEXT: (local.get $b) @@ -182,7 +182,7 @@ ) ) - ;; CHECK: (func $string.length (type $14) (param $ref externref) (result i32) + ;; CHECK: (func $string.length (type $13) (param $ref externref) (result i32) ;; CHECK-NEXT: (call $length ;; CHECK-NEXT: (local.get $ref) ;; CHECK-NEXT: ) @@ -193,7 +193,7 @@ ) ) - ;; CHECK: (func $string.get_codeunit (type $14) (param $ref externref) (result i32) + ;; CHECK: (func $string.get_codeunit (type $13) (param $ref externref) (result i32) ;; CHECK-NEXT: (call $charCodeAt ;; CHECK-NEXT: (local.get $ref) ;; CHECK-NEXT: (i32.const 2) @@ -206,7 +206,7 @@ ) ) - ;; CHECK: (func $string.slice (type $13) (param $ref externref) (result externref) + ;; CHECK: (func $string.slice (type $12) (param $ref externref) (result externref) ;; CHECK-NEXT: (call $substring ;; CHECK-NEXT: (local.get $ref) ;; CHECK-NEXT: (i32.const 2) @@ -221,7 +221,7 @@ ) ) - ;; CHECK: (func $if.string (type $13) (param $ref externref) (result externref) + ;; CHECK: (func $if.string (type $12) (param $ref externref) (result externref) ;; CHECK-NEXT: (if (result externref) ;; CHECK-NEXT: (i32.const 0) ;; CHECK-NEXT: (then @@ -244,7 +244,7 @@ ) ) - ;; CHECK: (func $if.string.flip (type $13) (param $ref externref) (result externref) + ;; CHECK: (func $if.string.flip (type $12) (param $ref externref) (result externref) ;; CHECK-NEXT: (if (result externref) ;; CHECK-NEXT: (i32.const 0) ;; CHECK-NEXT: (then @@ -268,7 +268,7 @@ ) ) - ;; CHECK: (func $exported-string-returner (type $4) (result externref) + ;; CHECK: (func $exported-string-returner (type $2) (result externref) ;; CHECK-NEXT: (global.get $string.const_exported) ;; CHECK-NEXT: ) (func $exported-string-returner (export "export.1") (result stringref) @@ -277,7 +277,7 @@ (string.const "exported") ) - ;; CHECK: (func $exported-string-receiver (type $3) (param $x externref) (param $y i32) (param $z externref) + ;; CHECK: (func $exported-string-receiver (type $18) (param $x externref) (param $y i32) (param $z externref) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (local.get $x) ;; CHECK-NEXT: ) @@ -398,7 +398,7 @@ ) ) - ;; CHECK: (func $call-param-null (type $5) (param $str externref) + ;; CHECK: (func $call-param-null (type $4) (param $str externref) ;; CHECK-NEXT: (call $call-param-null ;; CHECK-NEXT: (ref.null noextern) ;; CHECK-NEXT: ) From dd536bfb67aa17f1593a6c083903867ecd7ef6c8 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 4 Jun 2024 13:22:49 -0700 Subject: [PATCH 4/5] comment --- src/passes/StringLowering.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/passes/StringLowering.cpp b/src/passes/StringLowering.cpp index 375884b1750..614be9d81cc 100644 --- a/src/passes/StringLowering.cpp +++ b/src/passes/StringLowering.cpp @@ -276,6 +276,11 @@ struct StringLowering : public StringGathering { // keeps them at size 1, as expected, and handles the cases of function // imports and exports. If we need more (non-function types, non-singleton // rec groups then more work will be necessary TODO + // + // Note that we do this before TypeMapper, which allows it to them modify + // things like the types of parameters (which depend on the type of the + // function, which must be modified either in TypeMapper - but as just + // explained we cannot do that - or inside it). for (auto& func : module->functions) { if (func->type.getRecGroup().size() != 1 || !Type(func->type, Nullable).getFeatures().hasStrings()) { From 9fcea4097b0b5f678c3ec54ec5a2c86e02207e82 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 4 Jun 2024 15:36:09 -0700 Subject: [PATCH 5/5] comments --- src/passes/StringLowering.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/passes/StringLowering.cpp b/src/passes/StringLowering.cpp index 614be9d81cc..ca0ba773c76 100644 --- a/src/passes/StringLowering.cpp +++ b/src/passes/StringLowering.cpp @@ -275,12 +275,12 @@ struct StringLowering : public StringGathering { // Instead, manually handle singleton-rec groups of function types. This // keeps them at size 1, as expected, and handles the cases of function // imports and exports. If we need more (non-function types, non-singleton - // rec groups then more work will be necessary TODO + // rec groups, etc.) then more work will be necessary TODO // - // Note that we do this before TypeMapper, which allows it to them modify + // Note that we do this before TypeMapper, which allows it to then fix up // things like the types of parameters (which depend on the type of the // function, which must be modified either in TypeMapper - but as just - // explained we cannot do that - or inside it). + // explained we cannot do that - or before it, which is what we do here). for (auto& func : module->functions) { if (func->type.getRecGroup().size() != 1 || !Type(func->type, Nullable).getFeatures().hasStrings()) {