From aac7e4b7bfaecf983d55a926f78d567753c391f4 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Fri, 18 Apr 2025 13:21:48 -0700 Subject: [PATCH 01/32] start --- README.md | 6 +++++- src/passes/StringLowering.cpp | 18 ++++++++++++++---- src/passes/pass.cpp | 18 +++++++++++++++++- src/passes/passes.h | 1 + 4 files changed, 37 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 19334a96662..2710b87d4f3 100644 --- a/README.md +++ b/README.md @@ -175,7 +175,11 @@ There are a few differences between Binaryen IR and the WebAssembly language: output has the right type in the wasm spec. That may cause a few bytes of extra size in rare cases (we avoid this overhead in the common case where the `br_if` value is unused). - * Strings + * Strings: Binaryen has support for something close the stringref proposal, + mainly for internal optimization purposes. When the strings feature is + enabled, we "lift" JS string imports into string instructions, which are then + optimized, and after optimizations we lower back into JS string imports. (If + you do not want the lowering, use `--skip-pass=string-loewring XXXXXXXXXXXXXXXXXXXXXXXXXXXXx` * Binaryen allows string views (`stringview_wtf16` etc.) to be cast using `ref.cast`. This simplifies the IR, as it allows `ref.cast` to always be used in all places (and it is lowered to `ref.as_non_null` where possible diff --git a/src/passes/StringLowering.cpp b/src/passes/StringLowering.cpp index 84dfb7cb049..3de22054e84 100644 --- a/src/passes/StringLowering.cpp +++ b/src/passes/StringLowering.cpp @@ -204,8 +204,12 @@ struct StringLowering : public StringGathering { // imports. bool assertUTF8; - StringLowering(bool useMagicImports = false, bool assertUTF8 = false) - : useMagicImports(useMagicImports), assertUTF8(assertUTF8) { + // Normally we disable the strings feature after the lowering (like other + // lowering passes), but an optionally keep it. + bool keepFeature; + + StringLowering(bool useMagicImports = false, bool assertUTF8 = false, bool keepFeature = false) + : useMagicImports(useMagicImports), assertUTF8(assertUTF8), keepFeature(keepFeature) { // If we are asserting valid UTF-8, we must be using magic imports. assert(!assertUTF8 || useMagicImports); } @@ -233,8 +237,10 @@ struct StringLowering : public StringGathering { // ReFinalize to apply all the above changes. ReFinalize().run(getPassRunner(), module); - // Disable the feature here after we lowered everything away. - module->features.disable(FeatureSet::Strings); + if (!keepFeature) { + // Disable the feature here after we lowered everything away. + module->features.disable(FeatureSet::Strings); + } } void makeImports(Module* module) { @@ -553,10 +559,14 @@ struct StringLowering : public StringGathering { }; Pass* createStringGatheringPass() { return new StringGathering(); } + Pass* createStringLoweringPass() { return new StringLowering(); } Pass* createStringLoweringMagicImportPass() { return new StringLowering(true); } Pass* createStringLoweringMagicImportAssertPass() { return new StringLowering(true, true); } +Pass* createStringLoweringMagicImportKeepFeaturePass() { + return new StringLowering(true, false, true); +} } // namespace wasm diff --git a/src/passes/pass.cpp b/src/passes/pass.cpp index 2042bc71d3a..ecac6f83432 100644 --- a/src/passes/pass.cpp +++ b/src/passes/pass.cpp @@ -536,6 +536,10 @@ void PassRegistry::registerPasses() { "same as string-lowering-magic-imports, but raise a fatal error " "if there are invalid strings", createStringLoweringMagicImportAssertPass); + registerPass( + "string-lowering-magic-imports-keep-feature", + "same as string-lowering, but keeps the strings feature enabled after", + createStringLoweringMagicImportKeepFeaturePass); registerPass( "strip", "deprecated; same as strip-debug", createStripDebugPass); registerPass("stack-check", @@ -724,6 +728,11 @@ void PassRunner::addDefaultFunctionOptimizationPasses() { } void PassRunner::addDefaultGlobalOptimizationPrePasses() { + // Start by lifting strings into the optimizable IR form, which can help + // everything else. + if (wasm->features.hasStrings() && options.optimizeLevel >= 2) { + addIfNoDwarfIssues("string-lifting"); + } // Removing duplicate functions is fast and saves work later. addIfNoDWARFIssues("duplicate-function-elimination"); // Do a global cleanup before anything heavy, as it is fairly fast and can @@ -803,8 +812,15 @@ void PassRunner::addDefaultGlobalOptimizationPostPasses() { if (options.optimizeLevel >= 2 || options.shrinkLevel >= 1) { addIfNoDWARFIssues("reorder-globals"); } - // may allow more inlining/dae/etc., need --converge for that + // May allow more inlining/dae/etc., need --converge for that addIfNoDWARFIssues("directize"); + // Lower away strings at the very end. We do keep the strings feature enabled, + // as (1) it would be odd for the optimization pipeline to disable a feature, + // and also we want -O3 -O3 to work properly: if the first -O3 disabled the + // feature then the second would not lift strings at the start. + if (wasm->features.hasStrings() && options.optimizeLevel >= 2) { + addIfNoDwarfIssues("string-lowering-magic-imports-keep-feature"); + } } static void dumpWasm(Name name, Module* wasm, const PassOptions& options) { diff --git a/src/passes/passes.h b/src/passes/passes.h index e051e466e72..a451854dfd1 100644 --- a/src/passes/passes.h +++ b/src/passes/passes.h @@ -165,6 +165,7 @@ Pass* createStringLiftingPass(); Pass* createStringLoweringPass(); Pass* createStringLoweringMagicImportPass(); Pass* createStringLoweringMagicImportAssertPass(); +Pass* createStringLoweringMagicImportKeepFeaturePass(); Pass* createStripDebugPass(); Pass* createStripDWARFPass(); Pass* createStripProducersPass(); From 258a8cf68498c7455cccd3218c67a0057d907bbd Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Fri, 18 Apr 2025 13:24:36 -0700 Subject: [PATCH 02/32] test --- test/lit/passes/O2_strings.wast | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 test/lit/passes/O2_strings.wast diff --git a/test/lit/passes/O2_strings.wast b/test/lit/passes/O2_strings.wast new file mode 100644 index 00000000000..5fba6beb5e7 --- /dev/null +++ b/test/lit/passes/O2_strings.wast @@ -0,0 +1,28 @@ +;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. +;; NOTE: This test was ported using port_passes_tests_to_lit.py and could be cleaned up. + +;; RUN: foreach %s %t wasm-opt -O2 -all -S -o - | filecheck %s + +;; Test that -O2 lifts strings, optimizes them, and lowers them back down. +;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. + +;; RUN: foreach %s %t wasm-opt -all --string-lifting -S -o - | filecheck %s + +(module + (type $array16 (array (mut i16))) + + (import "\'" "foo" (global $foo (ref extern))) + + (import "\'" "bar" (global $bar (ref extern))) + + (import "wasm:js-string" "concat" (func $concat (param externref externref) (result (ref extern)))) + + (func $string.concat (export "string.concat") (result (ref extern)) + ;; We concatenate "foo" and "bar" here, and can optimize this at compile + ;; time to "foobar". + (call $concat + (global.get $foo) + (global.get $bar) + ) + ) +) From 21be4e421af49f4d037e623d88bd5307b183e3e3 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Fri, 18 Apr 2025 13:26:44 -0700 Subject: [PATCH 03/32] nop --- src/passes/pass.cpp | 4 ++-- test/lit/passes/O2_strings.wast | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/passes/pass.cpp b/src/passes/pass.cpp index ecac6f83432..c27fb1ec7d6 100644 --- a/src/passes/pass.cpp +++ b/src/passes/pass.cpp @@ -731,7 +731,7 @@ void PassRunner::addDefaultGlobalOptimizationPrePasses() { // Start by lifting strings into the optimizable IR form, which can help // everything else. if (wasm->features.hasStrings() && options.optimizeLevel >= 2) { - addIfNoDwarfIssues("string-lifting"); + addIfNoDWARFIssues("string-lifting"); } // Removing duplicate functions is fast and saves work later. addIfNoDWARFIssues("duplicate-function-elimination"); @@ -819,7 +819,7 @@ void PassRunner::addDefaultGlobalOptimizationPostPasses() { // and also we want -O3 -O3 to work properly: if the first -O3 disabled the // feature then the second would not lift strings at the start. if (wasm->features.hasStrings() && options.optimizeLevel >= 2) { - addIfNoDwarfIssues("string-lowering-magic-imports-keep-feature"); + addIfNoDWARFIssues("string-lowering-magic-imports-keep-feature"); } } diff --git a/test/lit/passes/O2_strings.wast b/test/lit/passes/O2_strings.wast index 5fba6beb5e7..f866705accc 100644 --- a/test/lit/passes/O2_strings.wast +++ b/test/lit/passes/O2_strings.wast @@ -11,12 +11,27 @@ (module (type $array16 (array (mut i16))) + ;; CHECK: (type $0 (func (param externref externref) (result (ref extern)))) + + ;; CHECK: (type $1 (func (result (ref extern)))) + + ;; CHECK: (import "\'" "foo" (global $foo (ref extern))) (import "\'" "foo" (global $foo (ref extern))) + ;; CHECK: (import "\'" "bar" (global $bar (ref extern))) (import "\'" "bar" (global $bar (ref extern))) + ;; CHECK: (import "wasm:js-string" "concat" (func $concat (type $0) (param externref externref) (result (ref extern)))) (import "wasm:js-string" "concat" (func $concat (param externref externref) (result (ref extern)))) + ;; CHECK: (export "string.concat" (func $string.concat)) + + ;; CHECK: (func $string.concat (type $1) (result (ref extern)) + ;; CHECK-NEXT: (string.concat + ;; CHECK-NEXT: (string.const "foo") + ;; CHECK-NEXT: (string.const "bar") + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) (func $string.concat (export "string.concat") (result (ref extern)) ;; We concatenate "foo" and "bar" here, and can optimize this at compile ;; time to "foobar". From 3f3753675d8317e05fe30c8fefcdec3dc390c35a Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Fri, 18 Apr 2025 13:29:15 -0700 Subject: [PATCH 04/32] work --- src/passes/pass.cpp | 20 +++++++++----------- test/lit/passes/O2_strings.wast | 20 ++++++-------------- 2 files changed, 15 insertions(+), 25 deletions(-) diff --git a/src/passes/pass.cpp b/src/passes/pass.cpp index c27fb1ec7d6..aac1d2bcf11 100644 --- a/src/passes/pass.cpp +++ b/src/passes/pass.cpp @@ -803,24 +803,22 @@ void PassRunner::addDefaultGlobalOptimizationPostPasses() { } else { addIfNoDWARFIssues("simplify-globals"); } - addIfNoDWARFIssues("remove-unused-module-elements"); - if (options.optimizeLevel >= 2 && wasm->features.hasStrings()) { - // Gather strings to globals right before reorder-globals, which will then - // sort them properly. - addIfNoDWARFIssues("string-gathering"); - } - if (options.optimizeLevel >= 2 || options.shrinkLevel >= 1) { - addIfNoDWARFIssues("reorder-globals"); - } - // May allow more inlining/dae/etc., need --converge for that - addIfNoDWARFIssues("directize"); // Lower away strings at the very end. We do keep the strings feature enabled, // as (1) it would be odd for the optimization pipeline to disable a feature, // and also we want -O3 -O3 to work properly: if the first -O3 disabled the // feature then the second would not lift strings at the start. + // + // We do this before remove-unused-module-elements so we don't add unused + // imports, and also before reorder-globals, which will sort the new globals. if (wasm->features.hasStrings() && options.optimizeLevel >= 2) { addIfNoDWARFIssues("string-lowering-magic-imports-keep-feature"); } + addIfNoDWARFIssues("remove-unused-module-elements"); + if (options.optimizeLevel >= 2 || options.shrinkLevel >= 1) { + addIfNoDWARFIssues("reorder-globals"); + } + // May allow more inlining/dae/etc., need --converge for that + addIfNoDWARFIssues("directize"); } static void dumpWasm(Name name, Module* wasm, const PassOptions& options) { diff --git a/test/lit/passes/O2_strings.wast b/test/lit/passes/O2_strings.wast index f866705accc..6739664cd08 100644 --- a/test/lit/passes/O2_strings.wast +++ b/test/lit/passes/O2_strings.wast @@ -6,31 +6,23 @@ ;; Test that -O2 lifts strings, optimizes them, and lowers them back down. ;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. -;; RUN: foreach %s %t wasm-opt -all --string-lifting -S -o - | filecheck %s - (module (type $array16 (array (mut i16))) - ;; CHECK: (type $0 (func (param externref externref) (result (ref extern)))) - - ;; CHECK: (type $1 (func (result (ref extern)))) - - ;; CHECK: (import "\'" "foo" (global $foo (ref extern))) (import "\'" "foo" (global $foo (ref extern))) - ;; CHECK: (import "\'" "bar" (global $bar (ref extern))) (import "\'" "bar" (global $bar (ref extern))) - ;; CHECK: (import "wasm:js-string" "concat" (func $concat (type $0) (param externref externref) (result (ref extern)))) (import "wasm:js-string" "concat" (func $concat (param externref externref) (result (ref extern)))) + ;; CHECK: (type $0 (func (result (ref extern)))) + + ;; CHECK: (import "\'" "foobar" (global $"string.const_\"foobar\"" (ref extern))) + ;; CHECK: (export "string.concat" (func $string.concat)) - ;; CHECK: (func $string.concat (type $1) (result (ref extern)) - ;; CHECK-NEXT: (string.concat - ;; CHECK-NEXT: (string.const "foo") - ;; CHECK-NEXT: (string.const "bar") - ;; CHECK-NEXT: ) + ;; CHECK: (func $string.concat (type $0) (result (ref extern)) + ;; CHECK-NEXT: (global.get $"string.const_\"foobar\"") ;; CHECK-NEXT: ) (func $string.concat (export "string.concat") (result (ref extern)) ;; We concatenate "foo" and "bar" here, and can optimize this at compile From 2db78d1ba6b6bf0308001fbacf88ec29abeb6d5e Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Fri, 18 Apr 2025 13:30:21 -0700 Subject: [PATCH 05/32] work --- test/lit/passes/O2_strings.wast | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/lit/passes/O2_strings.wast b/test/lit/passes/O2_strings.wast index 6739664cd08..b1a5dcfcf84 100644 --- a/test/lit/passes/O2_strings.wast +++ b/test/lit/passes/O2_strings.wast @@ -26,7 +26,8 @@ ;; CHECK-NEXT: ) (func $string.concat (export "string.concat") (result (ref extern)) ;; We concatenate "foo" and "bar" here, and can optimize this at compile - ;; time to "foobar". + ;; time to "foobar". A new imported global will appear for that, and we + ;; will only do a get of that global here. (call $concat (global.get $foo) (global.get $bar) From 59138a5be4722117730e2f9dee3bdfedd137fd64 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Fri, 18 Apr 2025 14:27:48 -0700 Subject: [PATCH 06/32] minimal fix for test --- src/ir/subtype-exprs.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/ir/subtype-exprs.h b/src/ir/subtype-exprs.h index 7d9f30b6778..93da1b7d6f8 100644 --- a/src/ir/subtype-exprs.h +++ b/src/ir/subtype-exprs.h @@ -415,7 +415,10 @@ struct SubtypingDiscoverer : public OverriddenVisitor { void visitContNew(ContNew* curr) { WASM_UNREACHABLE("not implemented"); } void visitContBind(ContBind* curr) { WASM_UNREACHABLE("not implemented"); } void visitSuspend(Suspend* curr) { WASM_UNREACHABLE("not implemented"); } - void visitResume(Resume* curr) { WASM_UNREACHABLE("not implemented"); } + void visitResume(Resume* curr) { + // TODO: Can we do better here? + self()->noteNonFlowSubtype(curr->cont, Type(HeapType::cont, Nullable)); + } void visitResumeThrow(ResumeThrow* curr) { WASM_UNREACHABLE("not implemented"); } From 38683e25f360d61017931686e4fa97792a84cf6e Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Fri, 18 Apr 2025 14:39:39 -0700 Subject: [PATCH 07/32] undo --- src/ir/subtype-exprs.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/ir/subtype-exprs.h b/src/ir/subtype-exprs.h index 93da1b7d6f8..7d9f30b6778 100644 --- a/src/ir/subtype-exprs.h +++ b/src/ir/subtype-exprs.h @@ -415,10 +415,7 @@ struct SubtypingDiscoverer : public OverriddenVisitor { void visitContNew(ContNew* curr) { WASM_UNREACHABLE("not implemented"); } void visitContBind(ContBind* curr) { WASM_UNREACHABLE("not implemented"); } void visitSuspend(Suspend* curr) { WASM_UNREACHABLE("not implemented"); } - void visitResume(Resume* curr) { - // TODO: Can we do better here? - self()->noteNonFlowSubtype(curr->cont, Type(HeapType::cont, Nullable)); - } + void visitResume(Resume* curr) { WASM_UNREACHABLE("not implemented"); } void visitResumeThrow(ResumeThrow* curr) { WASM_UNREACHABLE("not implemented"); } From c883cef3b61dd92b52ee364aa3665cccd8c96902 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Fri, 18 Apr 2025 14:40:40 -0700 Subject: [PATCH 08/32] optimize --- src/passes/StringLowering.cpp | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/src/passes/StringLowering.cpp b/src/passes/StringLowering.cpp index 3de22054e84..be1cc18237e 100644 --- a/src/passes/StringLowering.cpp +++ b/src/passes/StringLowering.cpp @@ -56,6 +56,12 @@ struct StringGathering : public Pass { using StringPtrs = std::vector; StringPtrs stringPtrs; + // Set to true if we found any uses of strings in the module. We note this as + // we scan the module for strings, and take into account not just strings + // constants but the string type in general - any use of the strings feature. + // Subclasses of this class use that information to avoid wasted work. + std::atomic stringsUsed = false; + // Main entry point. void run(Module* module) override { processModule(module); @@ -66,11 +72,19 @@ struct StringGathering : public Pass { // Scan the entire wasm to find the relevant strings to populate our global // data structures. void processModule(Module* module) { - struct StringWalker : public PostWalker { + struct StringWalker : public PostWalker> { StringPtrs& stringPtrs; StringWalker(StringPtrs& stringPtrs) : stringPtrs(stringPtrs) {} + bool stringsUsed = false; + + void visitExpression(Expression* curr) { + if (curr->type.isString()) { + stringsUsed = true; + } + } + void visitStringConst(StringConst* curr) { stringPtrs.push_back(getCurrentPointer()); } @@ -79,14 +93,22 @@ struct StringGathering : public Pass { ModuleUtils::ParallelFunctionAnalysis analysis( *module, [&](Function* func, StringPtrs& stringPtrs) { if (!func->imported()) { - StringWalker(stringPtrs).walk(func->body); + StringWalker walker(stringPtrs); + walker.walk(func->body); + if (walker.stringsUsed) { + stringsUsed = true; + } } }); // Also walk the global module code (for simplicity, also add it to the // function map, using a "function" key of nullptr). auto& globalStrings = analysis.map[nullptr]; - StringWalker(globalStrings).walkModuleCode(module); + StringWalker walker(globalStrings); + walker.walkModuleCode(module); + if (walker.stringsUsed) { + stringsUsed = true; + } // Combine all the strings. std::unordered_set stringSet; @@ -222,6 +244,11 @@ struct StringLowering : public StringGathering { // First, run the gathering operation so all string.consts are in one place. StringGathering::run(module); + // If we saw no strings during StringGathering, there is no work. + if (!stringsUsed) { + return; + } + // Remove all HeapType::string etc. in favor of externref. updateTypes(module); From bbe51690f674c5c3d459c15f2ca882c38768dfcb Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Fri, 18 Apr 2025 14:41:17 -0700 Subject: [PATCH 09/32] style --- src/passes/StringLowering.cpp | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/passes/StringLowering.cpp b/src/passes/StringLowering.cpp index be1cc18237e..c59c031e825 100644 --- a/src/passes/StringLowering.cpp +++ b/src/passes/StringLowering.cpp @@ -72,7 +72,9 @@ struct StringGathering : public Pass { // Scan the entire wasm to find the relevant strings to populate our global // data structures. void processModule(Module* module) { - struct StringWalker : public PostWalker> { + struct StringWalker + : public PostWalker> { StringPtrs& stringPtrs; StringWalker(StringPtrs& stringPtrs) : stringPtrs(stringPtrs) {} @@ -230,8 +232,11 @@ struct StringLowering : public StringGathering { // lowering passes), but an optionally keep it. bool keepFeature; - StringLowering(bool useMagicImports = false, bool assertUTF8 = false, bool keepFeature = false) - : useMagicImports(useMagicImports), assertUTF8(assertUTF8), keepFeature(keepFeature) { + StringLowering(bool useMagicImports = false, + bool assertUTF8 = false, + bool keepFeature = false) + : useMagicImports(useMagicImports), assertUTF8(assertUTF8), + keepFeature(keepFeature) { // If we are asserting valid UTF-8, we must be using magic imports. assert(!assertUTF8 || useMagicImports); } @@ -244,7 +249,7 @@ struct StringLowering : public StringGathering { // First, run the gathering operation so all string.consts are in one place. StringGathering::run(module); - // If we saw no strings during StringGathering, there is no work. + // If we saw no strings during StringGathering, there is no work. if (!stringsUsed) { return; } From 214b94c0d1e47238efa51c0e98ed26e42e0514e8 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Fri, 18 Apr 2025 14:44:01 -0700 Subject: [PATCH 10/32] fix --- src/passes/StringLowering.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/passes/StringLowering.cpp b/src/passes/StringLowering.cpp index c59c031e825..7ff42a0fbe1 100644 --- a/src/passes/StringLowering.cpp +++ b/src/passes/StringLowering.cpp @@ -89,6 +89,7 @@ struct StringGathering : public Pass { void visitStringConst(StringConst* curr) { stringPtrs.push_back(getCurrentPointer()); + stringsUsed = true; } }; From 0cbd3720e99fb5d21efd5409b7006420324f2cde Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Fri, 18 Apr 2025 14:45:59 -0700 Subject: [PATCH 11/32] help --- test/lit/help/wasm-metadce.test | 4 ++++ test/lit/help/wasm-opt.test | 4 ++++ test/lit/help/wasm2js.test | 4 ++++ 3 files changed, 12 insertions(+) diff --git a/test/lit/help/wasm-metadce.test b/test/lit/help/wasm-metadce.test index 5870b5c9d45..0b407a95d04 100644 --- a/test/lit/help/wasm-metadce.test +++ b/test/lit/help/wasm-metadce.test @@ -506,6 +506,10 @@ ;; CHECK-NEXT: but raise a fatal error if there ;; CHECK-NEXT: are invalid strings ;; CHECK-NEXT: +;; CHECK-NEXT: --string-lowering-magic-imports-keep-feature same as string-lowering, but +;; CHECK-NEXT: keeps the strings feature +;; CHECK-NEXT: enabled after +;; CHECK-NEXT: ;; CHECK-NEXT: --strip deprecated; same as strip-debug ;; CHECK-NEXT: ;; CHECK-NEXT: --strip-debug strip debug info (including the diff --git a/test/lit/help/wasm-opt.test b/test/lit/help/wasm-opt.test index ac98198d2dd..95ec6d5874b 100644 --- a/test/lit/help/wasm-opt.test +++ b/test/lit/help/wasm-opt.test @@ -518,6 +518,10 @@ ;; CHECK-NEXT: but raise a fatal error if there ;; CHECK-NEXT: are invalid strings ;; CHECK-NEXT: +;; CHECK-NEXT: --string-lowering-magic-imports-keep-feature same as string-lowering, but +;; CHECK-NEXT: keeps the strings feature +;; CHECK-NEXT: enabled after +;; CHECK-NEXT: ;; CHECK-NEXT: --strip deprecated; same as strip-debug ;; CHECK-NEXT: ;; CHECK-NEXT: --strip-debug strip debug info (including the diff --git a/test/lit/help/wasm2js.test b/test/lit/help/wasm2js.test index ac68667554b..07a68e75fb1 100644 --- a/test/lit/help/wasm2js.test +++ b/test/lit/help/wasm2js.test @@ -470,6 +470,10 @@ ;; CHECK-NEXT: but raise a fatal error if there ;; CHECK-NEXT: are invalid strings ;; CHECK-NEXT: +;; CHECK-NEXT: --string-lowering-magic-imports-keep-feature same as string-lowering, but +;; CHECK-NEXT: keeps the strings feature +;; CHECK-NEXT: enabled after +;; CHECK-NEXT: ;; CHECK-NEXT: --strip deprecated; same as strip-debug ;; CHECK-NEXT: ;; CHECK-NEXT: --strip-debug strip debug info (including the From a6cc9c81cb6db82e0b477f1a85ce3ed86078e9b1 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 21 Apr 2025 15:34:18 -0700 Subject: [PATCH 12/32] anoter way --- src/pass.h | 11 +++++++++++ src/passes/StringLifting.cpp | 5 +++++ src/passes/StringLowering.cpp | 29 +---------------------------- src/passes/pass.cpp | 6 ++++-- src/wasm.h | 2 +- 5 files changed, 22 insertions(+), 31 deletions(-) diff --git a/src/pass.h b/src/pass.h index 0ebd7e6d6bd..cae6f84060b 100644 --- a/src/pass.h +++ b/src/pass.h @@ -395,6 +395,17 @@ struct PassRunner { void handleAfterEffects(Pass* pass, Function* func = nullptr); bool shouldPreserveDWARF(); + + // Whether we lifted strings during our work. If we did, the caller may want + // to lower later. Noting the lifting work avoids always running the + // lowering pass at the end, which would add work. With this mechanism, if we + // lift strings at the beginning of -O2 or such then we will lower them at the + // end, and if we failed to see any strings to lift then we have no work to do + // (and no -O2 optimization passes in the middle can add strings to lower). + bool liftedStrings = false; + +public: + void noteLiftedStrings() { liftedStrings = true; } }; // diff --git a/src/passes/StringLifting.cpp b/src/passes/StringLifting.cpp index 58e2d28882e..a5843e6f6b1 100644 --- a/src/passes/StringLifting.cpp +++ b/src/passes/StringLifting.cpp @@ -82,6 +82,7 @@ struct StringLifting : public Pass { for (auto& section : module->customSections) { if (section.name == "string.consts") { // We found the string consts section. Parse it. + found = true; auto copy = section.data; json::Value array; array.parse(copy.data(), json::Value::WTF16); @@ -193,6 +194,10 @@ struct StringLifting : public Pass { return; } + // We found strings to lift. Mark this on the PassRunner, so that the + // corresponding lowering will occur, if we want one. + getPassRunner().noteLiftedStrings(); + struct StringApplier : public WalkerPass> { bool isFunctionParallel() override { return true; } diff --git a/src/passes/StringLowering.cpp b/src/passes/StringLowering.cpp index 7ff42a0fbe1..8ef0c1b8bcd 100644 --- a/src/passes/StringLowering.cpp +++ b/src/passes/StringLowering.cpp @@ -56,12 +56,6 @@ struct StringGathering : public Pass { using StringPtrs = std::vector; StringPtrs stringPtrs; - // Set to true if we found any uses of strings in the module. We note this as - // we scan the module for strings, and take into account not just strings - // constants but the string type in general - any use of the strings feature. - // Subclasses of this class use that information to avoid wasted work. - std::atomic stringsUsed = false; - // Main entry point. void run(Module* module) override { processModule(module); @@ -73,23 +67,13 @@ struct StringGathering : public Pass { // data structures. void processModule(Module* module) { struct StringWalker - : public PostWalker> { + : public PostWalker> { StringPtrs& stringPtrs; StringWalker(StringPtrs& stringPtrs) : stringPtrs(stringPtrs) {} - bool stringsUsed = false; - - void visitExpression(Expression* curr) { - if (curr->type.isString()) { - stringsUsed = true; - } - } - void visitStringConst(StringConst* curr) { stringPtrs.push_back(getCurrentPointer()); - stringsUsed = true; } }; @@ -98,9 +82,6 @@ struct StringGathering : public Pass { if (!func->imported()) { StringWalker walker(stringPtrs); walker.walk(func->body); - if (walker.stringsUsed) { - stringsUsed = true; - } } }); @@ -109,9 +90,6 @@ struct StringGathering : public Pass { auto& globalStrings = analysis.map[nullptr]; StringWalker walker(globalStrings); walker.walkModuleCode(module); - if (walker.stringsUsed) { - stringsUsed = true; - } // Combine all the strings. std::unordered_set stringSet; @@ -250,11 +228,6 @@ struct StringLowering : public StringGathering { // First, run the gathering operation so all string.consts are in one place. StringGathering::run(module); - // If we saw no strings during StringGathering, there is no work. - if (!stringsUsed) { - return; - } - // Remove all HeapType::string etc. in favor of externref. updateTypes(module); diff --git a/src/passes/pass.cpp b/src/passes/pass.cpp index aac1d2bcf11..629b90dc9e6 100644 --- a/src/passes/pass.cpp +++ b/src/passes/pass.cpp @@ -803,14 +803,16 @@ void PassRunner::addDefaultGlobalOptimizationPostPasses() { } else { addIfNoDWARFIssues("simplify-globals"); } - // Lower away strings at the very end. We do keep the strings feature enabled, + // Lower away strings at the very end, if we lifted. Only lowering if we + // lifted avoids running the pass + // We do keep the strings feature enabled, // as (1) it would be odd for the optimization pipeline to disable a feature, // and also we want -O3 -O3 to work properly: if the first -O3 disabled the // feature then the second would not lift strings at the start. // // We do this before remove-unused-module-elements so we don't add unused // imports, and also before reorder-globals, which will sort the new globals. - if (wasm->features.hasStrings() && options.optimizeLevel >= 2) { + if (liftedStrings) { addIfNoDWARFIssues("string-lowering-magic-imports-keep-feature"); } addIfNoDWARFIssues("remove-unused-module-elements"); diff --git a/src/wasm.h b/src/wasm.h index b6541fc6cf5..71e61f47bb2 100644 --- a/src/wasm.h +++ b/src/wasm.h @@ -2137,7 +2137,7 @@ struct BinaryLocations { std::unordered_map functions; }; -// Forward declaration for FuncEffectsMap. +// Forward declaration for function effects. class EffectAnalyzer; class Function : public Importable { From 0546b6f594855799e6e597bba0b8a795c1c4913f Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 21 Apr 2025 16:33:50 -0700 Subject: [PATCH 13/32] work --- src/pass.h | 3 ++- src/passes/StringLifting.cpp | 2 +- src/passes/StringLowering.cpp | 31 ++++++++++++++++++++++++------- src/passes/pass.cpp | 15 ++++++--------- src/passes/passes.h | 2 +- 5 files changed, 34 insertions(+), 19 deletions(-) diff --git a/src/pass.h b/src/pass.h index cae6f84060b..67885fb3fc2 100644 --- a/src/pass.h +++ b/src/pass.h @@ -405,7 +405,8 @@ struct PassRunner { bool liftedStrings = false; public: - void noteLiftedStrings() { liftedStrings = true; } + void setLiftedStrings(bool lifted) { liftedStrings = lifted; } + bool getLiftedStrings() { return liftedStrings; } }; // diff --git a/src/passes/StringLifting.cpp b/src/passes/StringLifting.cpp index a5843e6f6b1..7a4d2c42d7a 100644 --- a/src/passes/StringLifting.cpp +++ b/src/passes/StringLifting.cpp @@ -196,7 +196,7 @@ struct StringLifting : public Pass { // We found strings to lift. Mark this on the PassRunner, so that the // corresponding lowering will occur, if we want one. - getPassRunner().noteLiftedStrings(); + getPassRunner()->setLiftedStrings(true); struct StringApplier : public WalkerPass> { bool isFunctionParallel() override { return true; } diff --git a/src/passes/StringLowering.cpp b/src/passes/StringLowering.cpp index 8ef0c1b8bcd..52d298b1b44 100644 --- a/src/passes/StringLowering.cpp +++ b/src/passes/StringLowering.cpp @@ -207,15 +207,19 @@ struct StringLowering : public StringGathering { // imports. bool assertUTF8; - // Normally we disable the strings feature after the lowering (like other - // lowering passes), but an optionally keep it. - bool keepFeature; + // Whether we are "paired" to a lifting pass. The lifting pass happens early + // in the optimization pipeline in this mode, and we happen late, and we only + // want to lower if something was lifted. This mode also does not disable the + // strings feature, as we don't want the pair of passes to alter the feature + // set (we run it during the optimization pipeline, which should not do such + // things). + bool paired; StringLowering(bool useMagicImports = false, bool assertUTF8 = false, - bool keepFeature = false) + bool paired = false) : useMagicImports(useMagicImports), assertUTF8(assertUTF8), - keepFeature(keepFeature) { + paired(paired) { // If we are asserting valid UTF-8, we must be using magic imports. assert(!assertUTF8 || useMagicImports); } @@ -225,6 +229,18 @@ struct StringLowering : public StringGathering { return; } + auto* runner = getPassRunner(); + + if (paired) { + // We only want to lower if we previously lifted. + if (!runner->getLiftedStrings()) { + return; + } + + // Reset the flag for later optimization cycles. + runner->setLiftedStrings(false); + } + // First, run the gathering operation so all string.consts are in one place. StringGathering::run(module); @@ -241,7 +257,7 @@ struct StringLowering : public StringGathering { replaceNulls(module); // ReFinalize to apply all the above changes. - ReFinalize().run(getPassRunner(), module); + ReFinalize().run(runner, module); if (!keepFeature) { // Disable the feature here after we lowered everything away. @@ -571,7 +587,8 @@ Pass* createStringLoweringMagicImportPass() { return new StringLowering(true); } Pass* createStringLoweringMagicImportAssertPass() { return new StringLowering(true, true); } -Pass* createStringLoweringMagicImportKeepFeaturePass() { +Pass* createStringLoweringPairedPass() { + // Use magic imports; no asserts; paired. return new StringLowering(true, false, true); } diff --git a/src/passes/pass.cpp b/src/passes/pass.cpp index 629b90dc9e6..e37405eaf5e 100644 --- a/src/passes/pass.cpp +++ b/src/passes/pass.cpp @@ -537,9 +537,10 @@ void PassRegistry::registerPasses() { "if there are invalid strings", createStringLoweringMagicImportAssertPass); registerPass( - "string-lowering-magic-imports-keep-feature", - "same as string-lowering, but keeps the strings feature enabled after", - createStringLoweringMagicImportKeepFeaturePass); + "string-lowering-paired", + "same as string-lowering, but paired with a lifting operation (only lowers " + "if we lifted, and does not reset the strings feature)", + createStringLoweringPairedPass); registerPass( "strip", "deprecated; same as strip-debug", createStripDebugPass); registerPass("stack-check", @@ -803,18 +804,14 @@ void PassRunner::addDefaultGlobalOptimizationPostPasses() { } else { addIfNoDWARFIssues("simplify-globals"); } - // Lower away strings at the very end, if we lifted. Only lowering if we - // lifted avoids running the pass - // We do keep the strings feature enabled, + // Lower away strings at the very end. We do keep the strings feature enabled, // as (1) it would be odd for the optimization pipeline to disable a feature, // and also we want -O3 -O3 to work properly: if the first -O3 disabled the // feature then the second would not lift strings at the start. // // We do this before remove-unused-module-elements so we don't add unused // imports, and also before reorder-globals, which will sort the new globals. - if (liftedStrings) { - addIfNoDWARFIssues("string-lowering-magic-imports-keep-feature"); - } + addIfNoDWARFIssues("string-lowering-paired"); addIfNoDWARFIssues("remove-unused-module-elements"); if (options.optimizeLevel >= 2 || options.shrinkLevel >= 1) { addIfNoDWARFIssues("reorder-globals"); diff --git a/src/passes/passes.h b/src/passes/passes.h index a451854dfd1..83e8b5ff887 100644 --- a/src/passes/passes.h +++ b/src/passes/passes.h @@ -165,7 +165,7 @@ Pass* createStringLiftingPass(); Pass* createStringLoweringPass(); Pass* createStringLoweringMagicImportPass(); Pass* createStringLoweringMagicImportAssertPass(); -Pass* createStringLoweringMagicImportKeepFeaturePass(); +Pass* createStringLoweringPairedPass(); Pass* createStripDebugPass(); Pass* createStripDWARFPass(); Pass* createStripProducersPass(); From 6ddd59cf39830282c596b48ca571da273271d427 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 21 Apr 2025 16:41:11 -0700 Subject: [PATCH 14/32] work --- src/passes/StringLowering.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/passes/StringLowering.cpp b/src/passes/StringLowering.cpp index 52d298b1b44..e5e84c30607 100644 --- a/src/passes/StringLowering.cpp +++ b/src/passes/StringLowering.cpp @@ -259,7 +259,7 @@ struct StringLowering : public StringGathering { // ReFinalize to apply all the above changes. ReFinalize().run(runner, module); - if (!keepFeature) { + if (!paired) { // Disable the feature here after we lowered everything away. module->features.disable(FeatureSet::Strings); } From 6049fc9f958ed314ffc829b1c910e9a4899ed894 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 21 Apr 2025 16:48:15 -0700 Subject: [PATCH 15/32] update --- test/lit/help/wasm-metadce.test | 8 +++++--- test/lit/help/wasm-opt.test | 8 +++++--- test/lit/help/wasm2js.test | 8 +++++--- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/test/lit/help/wasm-metadce.test b/test/lit/help/wasm-metadce.test index 0b407a95d04..45187b1eaf3 100644 --- a/test/lit/help/wasm-metadce.test +++ b/test/lit/help/wasm-metadce.test @@ -506,9 +506,11 @@ ;; CHECK-NEXT: but raise a fatal error if there ;; CHECK-NEXT: are invalid strings ;; CHECK-NEXT: -;; CHECK-NEXT: --string-lowering-magic-imports-keep-feature same as string-lowering, but -;; CHECK-NEXT: keeps the strings feature -;; CHECK-NEXT: enabled after +;; CHECK-NEXT: --string-lowering-paired same as string-lowering, but +;; CHECK-NEXT: paired with a lifting operation +;; CHECK-NEXT: (only lowers if we lifted, and +;; CHECK-NEXT: does not reset the strings +;; CHECK-NEXT: feature) ;; CHECK-NEXT: ;; CHECK-NEXT: --strip deprecated; same as strip-debug ;; CHECK-NEXT: diff --git a/test/lit/help/wasm-opt.test b/test/lit/help/wasm-opt.test index 95ec6d5874b..9ad9066e550 100644 --- a/test/lit/help/wasm-opt.test +++ b/test/lit/help/wasm-opt.test @@ -518,9 +518,11 @@ ;; CHECK-NEXT: but raise a fatal error if there ;; CHECK-NEXT: are invalid strings ;; CHECK-NEXT: -;; CHECK-NEXT: --string-lowering-magic-imports-keep-feature same as string-lowering, but -;; CHECK-NEXT: keeps the strings feature -;; CHECK-NEXT: enabled after +;; CHECK-NEXT: --string-lowering-paired same as string-lowering, but +;; CHECK-NEXT: paired with a lifting operation +;; CHECK-NEXT: (only lowers if we lifted, and +;; CHECK-NEXT: does not reset the strings +;; CHECK-NEXT: feature) ;; CHECK-NEXT: ;; CHECK-NEXT: --strip deprecated; same as strip-debug ;; CHECK-NEXT: diff --git a/test/lit/help/wasm2js.test b/test/lit/help/wasm2js.test index 07a68e75fb1..2c745c94dc1 100644 --- a/test/lit/help/wasm2js.test +++ b/test/lit/help/wasm2js.test @@ -470,9 +470,11 @@ ;; CHECK-NEXT: but raise a fatal error if there ;; CHECK-NEXT: are invalid strings ;; CHECK-NEXT: -;; CHECK-NEXT: --string-lowering-magic-imports-keep-feature same as string-lowering, but -;; CHECK-NEXT: keeps the strings feature -;; CHECK-NEXT: enabled after +;; CHECK-NEXT: --string-lowering-paired same as string-lowering, but +;; CHECK-NEXT: paired with a lifting operation +;; CHECK-NEXT: (only lowers if we lifted, and +;; CHECK-NEXT: does not reset the strings +;; CHECK-NEXT: feature) ;; CHECK-NEXT: ;; CHECK-NEXT: --strip deprecated; same as strip-debug ;; CHECK-NEXT: From 0f7565ab13ef978d5b7eecec1c4d80e01f56734b Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Apr 2025 11:00:16 -0700 Subject: [PATCH 16/32] work --- src/passes/StringLifting.cpp | 15 ++++++++++++--- src/passes/pass.cpp | 7 +++++-- src/passes/passes.h | 1 + 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/passes/StringLifting.cpp b/src/passes/StringLifting.cpp index 7a4d2c42d7a..adca842fe06 100644 --- a/src/passes/StringLifting.cpp +++ b/src/passes/StringLifting.cpp @@ -35,6 +35,12 @@ namespace wasm { struct StringLifting : public Pass { + // Whether this is paired with a later lowering operation. If so, we mark + // whether we lifted anything. + bool paired; + + StringLifting(bool paired = false) : paired(paired) {} + // Maps the global name of an imported string to the actual string. std::unordered_map importedStrings; @@ -194,9 +200,11 @@ struct StringLifting : public Pass { return; } - // We found strings to lift. Mark this on the PassRunner, so that the - // corresponding lowering will occur, if we want one. - getPassRunner()->setLiftedStrings(true); + if (paired) { + // We found strings to lift. Mark this on the PassRunner, so that the + // corresponding lowering will occur, if we want one. + getPassRunner()->setLiftedStrings(true); + } struct StringApplier : public WalkerPass> { bool isFunctionParallel() override { return true; } @@ -297,5 +305,6 @@ struct StringLifting : public Pass { }; Pass* createStringLiftingPass() { return new StringLifting(); } +Pass* createStringLiftingPairedPass() { return new StringLifting(true); } } // namespace wasm diff --git a/src/passes/pass.cpp b/src/passes/pass.cpp index e37405eaf5e..efdf7b3a056 100644 --- a/src/passes/pass.cpp +++ b/src/passes/pass.cpp @@ -525,6 +525,9 @@ void PassRegistry::registerPasses() { registerPass("string-lifting", "lift string imports to wasm strings", createStringLiftingPass); + registerPass("string-lifting-paired", + "same as string-lifting, but paired with a later lowering", + createStringLiftingPairedPass); registerPass("string-lowering", "lowers wasm strings and operations to imports", createStringLoweringPass); @@ -538,7 +541,7 @@ void PassRegistry::registerPasses() { createStringLoweringMagicImportAssertPass); registerPass( "string-lowering-paired", - "same as string-lowering, but paired with a lifting operation (only lowers " + "same as string-lowering, but paired with an earlier lifting (only lowers " "if we lifted, and does not reset the strings feature)", createStringLoweringPairedPass); registerPass( @@ -732,7 +735,7 @@ void PassRunner::addDefaultGlobalOptimizationPrePasses() { // Start by lifting strings into the optimizable IR form, which can help // everything else. if (wasm->features.hasStrings() && options.optimizeLevel >= 2) { - addIfNoDWARFIssues("string-lifting"); + addIfNoDWARFIssues("string-lifting-paired"); } // Removing duplicate functions is fast and saves work later. addIfNoDWARFIssues("duplicate-function-elimination"); diff --git a/src/passes/passes.h b/src/passes/passes.h index 83e8b5ff887..49a5ce28db1 100644 --- a/src/passes/passes.h +++ b/src/passes/passes.h @@ -162,6 +162,7 @@ Pass* createSimplifyLocalsNoTeeNoStructurePass(); Pass* createStackCheckPass(); Pass* createStringGatheringPass(); Pass* createStringLiftingPass(); +Pass* createStringLiftingPairedPass(); Pass* createStringLoweringPass(); Pass* createStringLoweringMagicImportPass(); Pass* createStringLoweringMagicImportAssertPass(); From 0dd90e83b6fbfdbd8d4263f2632dee0a1e040bc3 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Apr 2025 11:03:41 -0700 Subject: [PATCH 17/32] fix --- README.md | 4 ++-- test/lit/passes/O2_strings.wast | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 2710b87d4f3..f2e2efe4bc6 100644 --- a/README.md +++ b/README.md @@ -175,11 +175,11 @@ There are a few differences between Binaryen IR and the WebAssembly language: output has the right type in the wasm spec. That may cause a few bytes of extra size in rare cases (we avoid this overhead in the common case where the `br_if` value is unused). - * Strings: Binaryen has support for something close the stringref proposal, + * Strings: Binaryen has support for something close to the stringref proposal, mainly for internal optimization purposes. When the strings feature is enabled, we "lift" JS string imports into string instructions, which are then optimized, and after optimizations we lower back into JS string imports. (If - you do not want the lowering, use `--skip-pass=string-loewring XXXXXXXXXXXXXXXXXXXXXXXXXXXXx` + you do not want the lowering, use `--skip-pass=string-lowering-paired`.) * Binaryen allows string views (`stringview_wtf16` etc.) to be cast using `ref.cast`. This simplifies the IR, as it allows `ref.cast` to always be used in all places (and it is lowered to `ref.as_non_null` where possible diff --git a/test/lit/passes/O2_strings.wast b/test/lit/passes/O2_strings.wast index b1a5dcfcf84..074abbe2cc8 100644 --- a/test/lit/passes/O2_strings.wast +++ b/test/lit/passes/O2_strings.wast @@ -1,7 +1,13 @@ ;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. ;; NOTE: This test was ported using port_passes_tests_to_lit.py and could be cleaned up. +;; Run normal -O2, and also see that -O2 plus an instruction to skip the +;; lowering works properly. + ;; RUN: foreach %s %t wasm-opt -O2 -all -S -o - | filecheck %s +;; RUN: foreach %s %t wasm-opt -O2 --skip-pass=string-lowering-paired -all -S -o - | filecheck %s --check-prefix=NO_LOWER + + ;; Test that -O2 lifts strings, optimizes them, and lowers them back down. ;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. @@ -24,10 +30,19 @@ ;; CHECK: (func $string.concat (type $0) (result (ref extern)) ;; CHECK-NEXT: (global.get $"string.const_\"foobar\"") ;; CHECK-NEXT: ) + ;; NO_LOWER: (type $0 (func (result (ref extern)))) + + ;; NO_LOWER: (export "string.concat" (func $string.concat)) + + ;; NO_LOWER: (func $string.concat (type $0) (result (ref extern)) + ;; NO_LOWER-NEXT: (string.const "foobar") + ;; NO_LOWER-NEXT: ) (func $string.concat (export "string.concat") (result (ref extern)) ;; We concatenate "foo" and "bar" here, and can optimize this at compile ;; time to "foobar". A new imported global will appear for that, and we ;; will only do a get of that global here. + ;; + ;; When we skip the lowering, we will have a string.const here. (call $concat (global.get $foo) (global.get $bar) From 60ce65c0c5e70952bec18186f3ea88b9ca90c745 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Apr 2025 11:05:11 -0700 Subject: [PATCH 18/32] work --- src/pass.h | 4 ++-- src/passes/StringLifting.cpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/pass.h b/src/pass.h index 67885fb3fc2..876d6e85925 100644 --- a/src/pass.h +++ b/src/pass.h @@ -397,11 +397,11 @@ struct PassRunner { bool shouldPreserveDWARF(); // Whether we lifted strings during our work. If we did, the caller may want - // to lower later. Noting the lifting work avoids always running the + // to lower later. Noting the lifting work helps us avoid always running the // lowering pass at the end, which would add work. With this mechanism, if we // lift strings at the beginning of -O2 or such then we will lower them at the // end, and if we failed to see any strings to lift then we have no work to do - // (and no -O2 optimization passes in the middle can add strings to lower). + // (note: no -O2 optimization passes in the middle can add strings to lower). bool liftedStrings = false; public: diff --git a/src/passes/StringLifting.cpp b/src/passes/StringLifting.cpp index adca842fe06..bf62c4c0b6d 100644 --- a/src/passes/StringLifting.cpp +++ b/src/passes/StringLifting.cpp @@ -202,7 +202,7 @@ struct StringLifting : public Pass { if (paired) { // We found strings to lift. Mark this on the PassRunner, so that the - // corresponding lowering will occur, if we want one. + // corresponding lowering will occur. getPassRunner()->setLiftedStrings(true); } From 618cf3e426fd9aee4b75c9fa1b91c67bebbf23c6 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Apr 2025 11:08:32 -0700 Subject: [PATCH 19/32] work --- src/passes/StringLowering.cpp | 13 +++++-------- src/passes/pass.cpp | 10 +++------- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/src/passes/StringLowering.cpp b/src/passes/StringLowering.cpp index e5e84c30607..5c945f8d3be 100644 --- a/src/passes/StringLowering.cpp +++ b/src/passes/StringLowering.cpp @@ -66,8 +66,7 @@ struct StringGathering : public Pass { // Scan the entire wasm to find the relevant strings to populate our global // data structures. void processModule(Module* module) { - struct StringWalker - : public PostWalker> { + struct StringWalker : public PostWalker { StringPtrs& stringPtrs; StringWalker(StringPtrs& stringPtrs) : stringPtrs(stringPtrs) {} @@ -80,16 +79,14 @@ struct StringGathering : public Pass { ModuleUtils::ParallelFunctionAnalysis analysis( *module, [&](Function* func, StringPtrs& stringPtrs) { if (!func->imported()) { - StringWalker walker(stringPtrs); - walker.walk(func->body); + StringWalker(stringPtrs).walk(func->body); } }); // Also walk the global module code (for simplicity, also add it to the // function map, using a "function" key of nullptr). auto& globalStrings = analysis.map[nullptr]; - StringWalker walker(globalStrings); - walker.walkModuleCode(module); + StringWalker(globalStrings).walkModuleCode(module); // Combine all the strings. std::unordered_set stringSet; @@ -210,8 +207,8 @@ struct StringLowering : public StringGathering { // Whether we are "paired" to a lifting pass. The lifting pass happens early // in the optimization pipeline in this mode, and we happen late, and we only // want to lower if something was lifted. This mode also does not disable the - // strings feature, as we don't want the pair of passes to alter the feature - // set (we run it during the optimization pipeline, which should not do such + // strings feature, as we don't want the pair of passes to alter the features + // (we run it during the optimization pipeline, which should not do such // things). bool paired; diff --git a/src/passes/pass.cpp b/src/passes/pass.cpp index efdf7b3a056..41948190d89 100644 --- a/src/passes/pass.cpp +++ b/src/passes/pass.cpp @@ -807,13 +807,9 @@ void PassRunner::addDefaultGlobalOptimizationPostPasses() { } else { addIfNoDWARFIssues("simplify-globals"); } - // Lower away strings at the very end. We do keep the strings feature enabled, - // as (1) it would be odd for the optimization pipeline to disable a feature, - // and also we want -O3 -O3 to work properly: if the first -O3 disabled the - // feature then the second would not lift strings at the start. - // - // We do this before remove-unused-module-elements so we don't add unused - // imports, and also before reorder-globals, which will sort the new globals. + // Lower away strings at the very end. We do this before + // remove-unused-module-elements so we don't add unused imports, and also + // before reorder-globals, which will sort the new globals. addIfNoDWARFIssues("string-lowering-paired"); addIfNoDWARFIssues("remove-unused-module-elements"); if (options.optimizeLevel >= 2 || options.shrinkLevel >= 1) { From 13e3fd4c05fd36a19b1afed3e209dc1f07f381f4 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Apr 2025 11:13:31 -0700 Subject: [PATCH 20/32] work --- test/lit/passes/O2_O3_strings.wast | 51 ++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 test/lit/passes/O2_O3_strings.wast diff --git a/test/lit/passes/O2_O3_strings.wast b/test/lit/passes/O2_O3_strings.wast new file mode 100644 index 00000000000..074abbe2cc8 --- /dev/null +++ b/test/lit/passes/O2_O3_strings.wast @@ -0,0 +1,51 @@ +;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. +;; NOTE: This test was ported using port_passes_tests_to_lit.py and could be cleaned up. + +;; Run normal -O2, and also see that -O2 plus an instruction to skip the +;; lowering works properly. + +;; RUN: foreach %s %t wasm-opt -O2 -all -S -o - | filecheck %s +;; RUN: foreach %s %t wasm-opt -O2 --skip-pass=string-lowering-paired -all -S -o - | filecheck %s --check-prefix=NO_LOWER + + + +;; Test that -O2 lifts strings, optimizes them, and lowers them back down. +;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. + +(module + (type $array16 (array (mut i16))) + + (import "\'" "foo" (global $foo (ref extern))) + + (import "\'" "bar" (global $bar (ref extern))) + + (import "wasm:js-string" "concat" (func $concat (param externref externref) (result (ref extern)))) + + ;; CHECK: (type $0 (func (result (ref extern)))) + + ;; CHECK: (import "\'" "foobar" (global $"string.const_\"foobar\"" (ref extern))) + + ;; CHECK: (export "string.concat" (func $string.concat)) + + ;; CHECK: (func $string.concat (type $0) (result (ref extern)) + ;; CHECK-NEXT: (global.get $"string.const_\"foobar\"") + ;; CHECK-NEXT: ) + ;; NO_LOWER: (type $0 (func (result (ref extern)))) + + ;; NO_LOWER: (export "string.concat" (func $string.concat)) + + ;; NO_LOWER: (func $string.concat (type $0) (result (ref extern)) + ;; NO_LOWER-NEXT: (string.const "foobar") + ;; NO_LOWER-NEXT: ) + (func $string.concat (export "string.concat") (result (ref extern)) + ;; We concatenate "foo" and "bar" here, and can optimize this at compile + ;; time to "foobar". A new imported global will appear for that, and we + ;; will only do a get of that global here. + ;; + ;; When we skip the lowering, we will have a string.const here. + (call $concat + (global.get $foo) + (global.get $bar) + ) + ) +) From 0083cae4443e1e542565da160bace4aacd722096 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Apr 2025 11:13:42 -0700 Subject: [PATCH 21/32] format --- src/passes/StringLowering.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/passes/StringLowering.cpp b/src/passes/StringLowering.cpp index 5c945f8d3be..7659eb25aeb 100644 --- a/src/passes/StringLowering.cpp +++ b/src/passes/StringLowering.cpp @@ -215,8 +215,7 @@ struct StringLowering : public StringGathering { StringLowering(bool useMagicImports = false, bool assertUTF8 = false, bool paired = false) - : useMagicImports(useMagicImports), assertUTF8(assertUTF8), - paired(paired) { + : useMagicImports(useMagicImports), assertUTF8(assertUTF8), paired(paired) { // If we are asserting valid UTF-8, we must be using magic imports. assert(!assertUTF8 || useMagicImports); } From 1da2e684c0de14936139cb6d767dafa7059a79a6 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Apr 2025 11:20:42 -0700 Subject: [PATCH 22/32] test --- test/lit/passes/O2_O3_strings.wast | 52 ++++++++++++++++-------------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/test/lit/passes/O2_O3_strings.wast b/test/lit/passes/O2_O3_strings.wast index 074abbe2cc8..1018428d20a 100644 --- a/test/lit/passes/O2_O3_strings.wast +++ b/test/lit/passes/O2_O3_strings.wast @@ -1,16 +1,13 @@ ;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. ;; NOTE: This test was ported using port_passes_tests_to_lit.py and could be cleaned up. -;; Run normal -O2, and also see that -O2 plus an instruction to skip the -;; lowering works properly. +;; Run -O2 and then -O3. Both the -O2 and -O3 should lift and lower strings. If +;; the -O2 somehow prevented the -O3 from doing so (say, forgetting to reset the +;; state between the paired lifting and lowering pass) then we would see that +;; -O3 failed to fully optimize, as the function here requires +;; --precompute-propagate which only -O3 does. -;; RUN: foreach %s %t wasm-opt -O2 -all -S -o - | filecheck %s -;; RUN: foreach %s %t wasm-opt -O2 --skip-pass=string-lowering-paired -all -S -o - | filecheck %s --check-prefix=NO_LOWER - - - -;; Test that -O2 lifts strings, optimizes them, and lowers them back down. -;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. +;; RUN: foreach %s %t wasm-opt -O2 -O3 -all -S -o - | filecheck %s (module (type $array16 (array (mut i16))) @@ -23,29 +20,36 @@ ;; CHECK: (type $0 (func (result (ref extern)))) - ;; CHECK: (import "\'" "foobar" (global $"string.const_\"foobar\"" (ref extern))) + ;; CHECK: (import "\'" "foobarbar" (global $"string.const_\"foobarbar\"" (ref extern))) ;; CHECK: (export "string.concat" (func $string.concat)) ;; CHECK: (func $string.concat (type $0) (result (ref extern)) - ;; CHECK-NEXT: (global.get $"string.const_\"foobar\"") + ;; CHECK-NEXT: (global.get $"string.const_\"foobarbar\"") ;; CHECK-NEXT: ) - ;; NO_LOWER: (type $0 (func (result (ref extern)))) - - ;; NO_LOWER: (export "string.concat" (func $string.concat)) - - ;; NO_LOWER: (func $string.concat (type $0) (result (ref extern)) - ;; NO_LOWER-NEXT: (string.const "foobar") - ;; NO_LOWER-NEXT: ) (func $string.concat (export "string.concat") (result (ref extern)) - ;; We concatenate "foo" and "bar" here, and can optimize this at compile - ;; time to "foobar". A new imported global will appear for that, and we - ;; will only do a get of that global here. - ;; - ;; When we skip the lowering, we will have a string.const here. - (call $concat + (local $x (ref extern)) + (local $y (ref extern)) + ;; -O2 does not propagate the string constants through the locals, but -O3 + ;; will, allowing this function to return the concatenated final string. + (local.set $x (global.get $foo) + ) + (local.set $y (global.get $bar) ) + ;; Add an extra concat that is written to $x, so the local has multiple + ;; writes (otherwise, simplify-locals manages to remove the locals, and -O2 + ;; does just as well as -O3). + (local.set $x + (call $concat + (local.get $x) + (local.get $y) + ) + ) + (call $concat + (local.get $x) + (local.get $y) + ) ) ) From 6f6f5caa0891964c1b78e88f9a9d2c208a21123c Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Apr 2025 11:26:43 -0700 Subject: [PATCH 23/32] note --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a97be02a49b..6e3fdd54a9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,10 @@ full changeset diff at the end of each section. Current Trunk ------------- + - `-O2` and above will now automatically lift and lower strings, if the string + feature is enabled. That is, enable strings if you want the optimizer to lift + strings, optimize them (e.g. precompute a concat of two constants, and lower + them back down to js string imports). - Add a `--string-lifting` pass that raises imported string operations and constants into stringref in Binaryen IR (which can then be fully optimized, and typically lowered back down with `--string-lowering`). From 107697ec6fb307dbc65bc551647de1c377f61d25 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Apr 2025 11:31:19 -0700 Subject: [PATCH 24/32] work --- CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e3fdd54a9f..2d14ee35204 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,9 +16,9 @@ Current Trunk ------------- - `-O2` and above will now automatically lift and lower strings, if the string - feature is enabled. That is, enable strings if you want the optimizer to lift - strings, optimize them (e.g. precompute a concat of two constants, and lower - them back down to js string imports). + feature is enabled. That is, `--enable-strings` will now optimize imported + JS strings (by lifting at the start into stringref, which we can optimize, + and lowering at the end back to imported strings). - Add a `--string-lifting` pass that raises imported string operations and constants into stringref in Binaryen IR (which can then be fully optimized, and typically lowered back down with `--string-lowering`). From 4237c4da2f6bf845941ce0b24ae041f3174e8a58 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Apr 2025 11:34:48 -0700 Subject: [PATCH 25/32] guard --- src/passes/pass.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/passes/pass.cpp b/src/passes/pass.cpp index 41948190d89..34d0aaa1640 100644 --- a/src/passes/pass.cpp +++ b/src/passes/pass.cpp @@ -810,7 +810,9 @@ void PassRunner::addDefaultGlobalOptimizationPostPasses() { // Lower away strings at the very end. We do this before // remove-unused-module-elements so we don't add unused imports, and also // before reorder-globals, which will sort the new globals. - addIfNoDWARFIssues("string-lowering-paired"); + if (wasm->features.hasStrings() && options.optimizeLevel >= 2) { + addIfNoDWARFIssues("string-lowering-paired"); + } addIfNoDWARFIssues("remove-unused-module-elements"); if (options.optimizeLevel >= 2 || options.shrinkLevel >= 1) { addIfNoDWARFIssues("reorder-globals"); From 54305675d36ed259dcd4367ef5a66520b63f56c9 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Apr 2025 11:35:57 -0700 Subject: [PATCH 26/32] fix --- test/lit/help/wasm-metadce.test | 5 ++++- test/lit/help/wasm-opt.test | 5 ++++- test/lit/help/wasm2js.test | 5 ++++- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/test/lit/help/wasm-metadce.test b/test/lit/help/wasm-metadce.test index 45187b1eaf3..1d5ddc87a3e 100644 --- a/test/lit/help/wasm-metadce.test +++ b/test/lit/help/wasm-metadce.test @@ -494,6 +494,9 @@ ;; CHECK-NEXT: --string-lifting lift string imports to wasm ;; CHECK-NEXT: strings ;; CHECK-NEXT: +;; CHECK-NEXT: --string-lifting-paired same as string-lifting, but +;; CHECK-NEXT: paired with a later lowering +;; CHECK-NEXT: ;; CHECK-NEXT: --string-lowering lowers wasm strings and ;; CHECK-NEXT: operations to imports ;; CHECK-NEXT: @@ -507,7 +510,7 @@ ;; CHECK-NEXT: are invalid strings ;; CHECK-NEXT: ;; CHECK-NEXT: --string-lowering-paired same as string-lowering, but -;; CHECK-NEXT: paired with a lifting operation +;; CHECK-NEXT: paired with an earlier lifting ;; CHECK-NEXT: (only lowers if we lifted, and ;; CHECK-NEXT: does not reset the strings ;; CHECK-NEXT: feature) diff --git a/test/lit/help/wasm-opt.test b/test/lit/help/wasm-opt.test index 9ad9066e550..5fe4c86ee68 100644 --- a/test/lit/help/wasm-opt.test +++ b/test/lit/help/wasm-opt.test @@ -506,6 +506,9 @@ ;; CHECK-NEXT: --string-lifting lift string imports to wasm ;; CHECK-NEXT: strings ;; CHECK-NEXT: +;; CHECK-NEXT: --string-lifting-paired same as string-lifting, but +;; CHECK-NEXT: paired with a later lowering +;; CHECK-NEXT: ;; CHECK-NEXT: --string-lowering lowers wasm strings and ;; CHECK-NEXT: operations to imports ;; CHECK-NEXT: @@ -519,7 +522,7 @@ ;; CHECK-NEXT: are invalid strings ;; CHECK-NEXT: ;; CHECK-NEXT: --string-lowering-paired same as string-lowering, but -;; CHECK-NEXT: paired with a lifting operation +;; CHECK-NEXT: paired with an earlier lifting ;; CHECK-NEXT: (only lowers if we lifted, and ;; CHECK-NEXT: does not reset the strings ;; CHECK-NEXT: feature) diff --git a/test/lit/help/wasm2js.test b/test/lit/help/wasm2js.test index 2c745c94dc1..f5003a066de 100644 --- a/test/lit/help/wasm2js.test +++ b/test/lit/help/wasm2js.test @@ -458,6 +458,9 @@ ;; CHECK-NEXT: --string-lifting lift string imports to wasm ;; CHECK-NEXT: strings ;; CHECK-NEXT: +;; CHECK-NEXT: --string-lifting-paired same as string-lifting, but +;; CHECK-NEXT: paired with a later lowering +;; CHECK-NEXT: ;; CHECK-NEXT: --string-lowering lowers wasm strings and ;; CHECK-NEXT: operations to imports ;; CHECK-NEXT: @@ -471,7 +474,7 @@ ;; CHECK-NEXT: are invalid strings ;; CHECK-NEXT: ;; CHECK-NEXT: --string-lowering-paired same as string-lowering, but -;; CHECK-NEXT: paired with a lifting operation +;; CHECK-NEXT: paired with an earlier lifting ;; CHECK-NEXT: (only lowers if we lifted, and ;; CHECK-NEXT: does not reset the strings ;; CHECK-NEXT: feature) From bc97fe29b6bf394c9ee4c6c7dec7f9fdaec6761b Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Apr 2025 11:41:46 -0700 Subject: [PATCH 27/32] test --- test/lit/passes/O2_strings.wast | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/test/lit/passes/O2_strings.wast b/test/lit/passes/O2_strings.wast index 074abbe2cc8..bc1d5798477 100644 --- a/test/lit/passes/O2_strings.wast +++ b/test/lit/passes/O2_strings.wast @@ -1,17 +1,12 @@ ;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. ;; NOTE: This test was ported using port_passes_tests_to_lit.py and could be cleaned up. -;; Run normal -O2, and also see that -O2 plus an instruction to skip the -;; lowering works properly. +;; Run normal -O2, which should lift, optimize, and lower strings. Also see that +;; -O2 plus an instruction to skip the lowering works properly. ;; RUN: foreach %s %t wasm-opt -O2 -all -S -o - | filecheck %s ;; RUN: foreach %s %t wasm-opt -O2 --skip-pass=string-lowering-paired -all -S -o - | filecheck %s --check-prefix=NO_LOWER - - -;; Test that -O2 lifts strings, optimizes them, and lowers them back down. -;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. - (module (type $array16 (array (mut i16))) From d8fd20e4938e8b56569cd284ad77b86d1c04c2d8 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Apr 2025 14:29:41 -0700 Subject: [PATCH 28/32] fuzzer fix --- scripts/test/fuzzing.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/scripts/test/fuzzing.py b/scripts/test/fuzzing.py index 4935f8b52ec..bd120a2eed1 100644 --- a/scripts/test/fuzzing.py +++ b/scripts/test/fuzzing.py @@ -21,10 +21,14 @@ 'f16.wast', # not all relaxed SIMD instructions are implemented in the interpreter 'relaxed-simd.wast', - # TODO: fuzzer and interpreter support for strings + # TODO: fuzzer and interpreter support for strings, including limitations + # like the fuzzer not handling (ref extern) imports (there is no way + # to create a replacement value) 'strings.wast', 'simplify-locals-strings.wast', 'string-lowering-instructions.wast', + 'O2_strings', + 'O2_O3_strings', # TODO: fuzzer and interpreter support for extern conversions 'extern-conversions.wast', # ignore DWARF because it is incompatible with multivalue atm From 61e1c1a212bea7856a615d1a62246a110108af31 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Apr 2025 15:19:03 -0700 Subject: [PATCH 29/32] oops --- scripts/test/fuzzing.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/test/fuzzing.py b/scripts/test/fuzzing.py index bd120a2eed1..ecb9fe8ed25 100644 --- a/scripts/test/fuzzing.py +++ b/scripts/test/fuzzing.py @@ -27,8 +27,8 @@ 'strings.wast', 'simplify-locals-strings.wast', 'string-lowering-instructions.wast', - 'O2_strings', - 'O2_O3_strings', + 'O2_strings.wast', + 'O2_O3_strings.wast', # TODO: fuzzer and interpreter support for extern conversions 'extern-conversions.wast', # ignore DWARF because it is incompatible with multivalue atm From 6cb8b9a86889e5d99e4d759f6d829563b14d43f6 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Apr 2025 16:21:30 -0700 Subject: [PATCH 30/32] make the string-*-paired passes internal/test, so they don't clutter help --- src/passes/pass.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/passes/pass.cpp b/src/passes/pass.cpp index 34d0aaa1640..acb8dc6dd79 100644 --- a/src/passes/pass.cpp +++ b/src/passes/pass.cpp @@ -525,9 +525,9 @@ void PassRegistry::registerPasses() { registerPass("string-lifting", "lift string imports to wasm strings", createStringLiftingPass); - registerPass("string-lifting-paired", - "same as string-lifting, but paired with a later lowering", - createStringLiftingPairedPass); + registerTestPass("string-lifting-paired", + "same as string-lifting, but paired with a later lowering", + createStringLiftingPairedPass); registerPass("string-lowering", "lowers wasm strings and operations to imports", createStringLoweringPass); @@ -539,7 +539,7 @@ void PassRegistry::registerPasses() { "same as string-lowering-magic-imports, but raise a fatal error " "if there are invalid strings", createStringLoweringMagicImportAssertPass); - registerPass( + registerTestPass( "string-lowering-paired", "same as string-lowering, but paired with an earlier lifting (only lowers " "if we lifted, and does not reset the strings feature)", From 4db089046c38fd3968a4efff432c391c5ee6b2a1 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Apr 2025 16:21:52 -0700 Subject: [PATCH 31/32] update --- test/lit/help/wasm-metadce.test | 9 --------- test/lit/help/wasm-opt.test | 9 --------- test/lit/help/wasm2js.test | 9 --------- 3 files changed, 27 deletions(-) diff --git a/test/lit/help/wasm-metadce.test b/test/lit/help/wasm-metadce.test index 1d5ddc87a3e..5870b5c9d45 100644 --- a/test/lit/help/wasm-metadce.test +++ b/test/lit/help/wasm-metadce.test @@ -494,9 +494,6 @@ ;; CHECK-NEXT: --string-lifting lift string imports to wasm ;; CHECK-NEXT: strings ;; CHECK-NEXT: -;; CHECK-NEXT: --string-lifting-paired same as string-lifting, but -;; CHECK-NEXT: paired with a later lowering -;; CHECK-NEXT: ;; CHECK-NEXT: --string-lowering lowers wasm strings and ;; CHECK-NEXT: operations to imports ;; CHECK-NEXT: @@ -509,12 +506,6 @@ ;; CHECK-NEXT: but raise a fatal error if there ;; CHECK-NEXT: are invalid strings ;; CHECK-NEXT: -;; CHECK-NEXT: --string-lowering-paired same as string-lowering, but -;; CHECK-NEXT: paired with an earlier lifting -;; CHECK-NEXT: (only lowers if we lifted, and -;; CHECK-NEXT: does not reset the strings -;; CHECK-NEXT: feature) -;; CHECK-NEXT: ;; CHECK-NEXT: --strip deprecated; same as strip-debug ;; CHECK-NEXT: ;; CHECK-NEXT: --strip-debug strip debug info (including the diff --git a/test/lit/help/wasm-opt.test b/test/lit/help/wasm-opt.test index 5fe4c86ee68..ac98198d2dd 100644 --- a/test/lit/help/wasm-opt.test +++ b/test/lit/help/wasm-opt.test @@ -506,9 +506,6 @@ ;; CHECK-NEXT: --string-lifting lift string imports to wasm ;; CHECK-NEXT: strings ;; CHECK-NEXT: -;; CHECK-NEXT: --string-lifting-paired same as string-lifting, but -;; CHECK-NEXT: paired with a later lowering -;; CHECK-NEXT: ;; CHECK-NEXT: --string-lowering lowers wasm strings and ;; CHECK-NEXT: operations to imports ;; CHECK-NEXT: @@ -521,12 +518,6 @@ ;; CHECK-NEXT: but raise a fatal error if there ;; CHECK-NEXT: are invalid strings ;; CHECK-NEXT: -;; CHECK-NEXT: --string-lowering-paired same as string-lowering, but -;; CHECK-NEXT: paired with an earlier lifting -;; CHECK-NEXT: (only lowers if we lifted, and -;; CHECK-NEXT: does not reset the strings -;; CHECK-NEXT: feature) -;; CHECK-NEXT: ;; CHECK-NEXT: --strip deprecated; same as strip-debug ;; CHECK-NEXT: ;; CHECK-NEXT: --strip-debug strip debug info (including the diff --git a/test/lit/help/wasm2js.test b/test/lit/help/wasm2js.test index f5003a066de..ac68667554b 100644 --- a/test/lit/help/wasm2js.test +++ b/test/lit/help/wasm2js.test @@ -458,9 +458,6 @@ ;; CHECK-NEXT: --string-lifting lift string imports to wasm ;; CHECK-NEXT: strings ;; CHECK-NEXT: -;; CHECK-NEXT: --string-lifting-paired same as string-lifting, but -;; CHECK-NEXT: paired with a later lowering -;; CHECK-NEXT: ;; CHECK-NEXT: --string-lowering lowers wasm strings and ;; CHECK-NEXT: operations to imports ;; CHECK-NEXT: @@ -473,12 +470,6 @@ ;; CHECK-NEXT: but raise a fatal error if there ;; CHECK-NEXT: are invalid strings ;; CHECK-NEXT: -;; CHECK-NEXT: --string-lowering-paired same as string-lowering, but -;; CHECK-NEXT: paired with an earlier lifting -;; CHECK-NEXT: (only lowers if we lifted, and -;; CHECK-NEXT: does not reset the strings -;; CHECK-NEXT: feature) -;; CHECK-NEXT: ;; CHECK-NEXT: --strip deprecated; same as strip-debug ;; CHECK-NEXT: ;; CHECK-NEXT: --strip-debug strip debug info (including the From 911df992c2315fa9b9e3a55cfe8986aa5298bfa8 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Apr 2025 16:27:36 -0700 Subject: [PATCH 32/32] Add a flag for convenient disabling of string lowering --- README.md | 2 +- src/tools/optimization-options.h | 14 ++++++++++++++ test/lit/help/wasm-metadce.test | 6 ++++++ test/lit/help/wasm-opt.test | 6 ++++++ test/lit/help/wasm2js.test | 6 ++++++ test/lit/passes/O2_strings.wast | 4 ++-- 6 files changed, 35 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index f2e2efe4bc6..aaf4a6457b2 100644 --- a/README.md +++ b/README.md @@ -179,7 +179,7 @@ There are a few differences between Binaryen IR and the WebAssembly language: mainly for internal optimization purposes. When the strings feature is enabled, we "lift" JS string imports into string instructions, which are then optimized, and after optimizations we lower back into JS string imports. (If - you do not want the lowering, use `--skip-pass=string-lowering-paired`.) + you do not want the lowering, use `--no-string-lowering`.) * Binaryen allows string views (`stringview_wtf16` etc.) to be cast using `ref.cast`. This simplifies the IR, as it allows `ref.cast` to always be used in all places (and it is lowered to `ref.as_non_null` where possible diff --git a/src/tools/optimization-options.h b/src/tools/optimization-options.h index 333380d0490..a97a8427ba1 100644 --- a/src/tools/optimization-options.h +++ b/src/tools/optimization-options.h @@ -314,6 +314,20 @@ struct OptimizationOptions : public ToolOptions { Options::Arguments::N, [this](Options*, const std::string& pass) { passOptions.passesToSkip.insert(pass); + }) + .add("--no-string-lowering", + "", + "Disables the automatic lowering of stringref to imported strings " + "(that normally happens at the end of the optimization pipeline, if " + "-O2+ is specified)", + OptimizationOptionsCategory, + Options::Arguments::Zero, + [&](Options*, const std::string&) { + // string-lowering-paired is internal (it only makes sense as part + // of a pair of passes in the optimization pipeline), and so it + // does not show up in --help. To make it convenient for users to + // disable the lowering, we provide this flag. + passOptions.passesToSkip.insert("string-lowering-paired"); }); // add passes in registry diff --git a/test/lit/help/wasm-metadce.test b/test/lit/help/wasm-metadce.test index 5870b5c9d45..206e3f50bf7 100644 --- a/test/lit/help/wasm-metadce.test +++ b/test/lit/help/wasm-metadce.test @@ -663,6 +663,12 @@ ;; CHECK-NEXT: ;; CHECK-NEXT: --skip-pass,-sp Skip a pass (do not run it) ;; CHECK-NEXT: +;; CHECK-NEXT: --no-string-lowering Disables the automatic lowering +;; CHECK-NEXT: of stringref to imported strings +;; CHECK-NEXT: (that normally happens at the +;; CHECK-NEXT: end of the optimization +;; CHECK-NEXT: pipeline, if -O2+ is specified) +;; CHECK-NEXT: ;; CHECK-NEXT: ;; CHECK-NEXT: Tool options: ;; CHECK-NEXT: ------------- diff --git a/test/lit/help/wasm-opt.test b/test/lit/help/wasm-opt.test index ac98198d2dd..8ba6d16f42c 100644 --- a/test/lit/help/wasm-opt.test +++ b/test/lit/help/wasm-opt.test @@ -675,6 +675,12 @@ ;; CHECK-NEXT: ;; CHECK-NEXT: --skip-pass,-sp Skip a pass (do not run it) ;; CHECK-NEXT: +;; CHECK-NEXT: --no-string-lowering Disables the automatic lowering +;; CHECK-NEXT: of stringref to imported strings +;; CHECK-NEXT: (that normally happens at the +;; CHECK-NEXT: end of the optimization +;; CHECK-NEXT: pipeline, if -O2+ is specified) +;; CHECK-NEXT: ;; CHECK-NEXT: ;; CHECK-NEXT: Tool options: ;; CHECK-NEXT: ------------- diff --git a/test/lit/help/wasm2js.test b/test/lit/help/wasm2js.test index ac68667554b..0c897c1bfc4 100644 --- a/test/lit/help/wasm2js.test +++ b/test/lit/help/wasm2js.test @@ -627,6 +627,12 @@ ;; CHECK-NEXT: ;; CHECK-NEXT: --skip-pass,-sp Skip a pass (do not run it) ;; CHECK-NEXT: +;; CHECK-NEXT: --no-string-lowering Disables the automatic lowering +;; CHECK-NEXT: of stringref to imported strings +;; CHECK-NEXT: (that normally happens at the +;; CHECK-NEXT: end of the optimization +;; CHECK-NEXT: pipeline, if -O2+ is specified) +;; CHECK-NEXT: ;; CHECK-NEXT: ;; CHECK-NEXT: Tool options: ;; CHECK-NEXT: ------------- diff --git a/test/lit/passes/O2_strings.wast b/test/lit/passes/O2_strings.wast index bc1d5798477..9af7addc43d 100644 --- a/test/lit/passes/O2_strings.wast +++ b/test/lit/passes/O2_strings.wast @@ -2,10 +2,10 @@ ;; NOTE: This test was ported using port_passes_tests_to_lit.py and could be cleaned up. ;; Run normal -O2, which should lift, optimize, and lower strings. Also see that -;; -O2 plus an instruction to skip the lowering works properly. +;; -O2 plus the flag to skip the lowering works properly. ;; RUN: foreach %s %t wasm-opt -O2 -all -S -o - | filecheck %s -;; RUN: foreach %s %t wasm-opt -O2 --skip-pass=string-lowering-paired -all -S -o - | filecheck %s --check-prefix=NO_LOWER +;; RUN: foreach %s %t wasm-opt -O2 --no-string-lowering -all -S -o - | filecheck %s --check-prefix=NO_LOWER (module (type $array16 (array (mut i16)))