diff --git a/CHANGELOG.md b/CHANGELOG.md index a97be02a49b..2d14ee35204 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` 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`). diff --git a/README.md b/README.md index 19334a96662..aaf4a6457b2 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 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 `--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/scripts/test/fuzzing.py b/scripts/test/fuzzing.py index 971a5c18e4d..35ecad78c52 100644 --- a/scripts/test/fuzzing.py +++ b/scripts/test/fuzzing.py @@ -19,10 +19,14 @@ unfuzzable = [ # Float16 is still experimental. 'f16.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.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 diff --git a/src/pass.h b/src/pass.h index 0ebd7e6d6bd..876d6e85925 100644 --- a/src/pass.h +++ b/src/pass.h @@ -395,6 +395,18 @@ 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 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 + // (note: no -O2 optimization passes in the middle can add strings to lower). + bool liftedStrings = false; + +public: + void setLiftedStrings(bool lifted) { liftedStrings = lifted; } + bool getLiftedStrings() { return liftedStrings; } }; // diff --git a/src/passes/StringLifting.cpp b/src/passes/StringLifting.cpp index 58e2d28882e..bf62c4c0b6d 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; @@ -82,6 +88,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 +200,12 @@ struct StringLifting : public Pass { return; } + if (paired) { + // We found strings to lift. Mark this on the PassRunner, so that the + // corresponding lowering will occur. + getPassRunner()->setLiftedStrings(true); + } + struct StringApplier : public WalkerPass> { bool isFunctionParallel() override { return true; } @@ -292,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/StringLowering.cpp b/src/passes/StringLowering.cpp index 84dfb7cb049..7659eb25aeb 100644 --- a/src/passes/StringLowering.cpp +++ b/src/passes/StringLowering.cpp @@ -204,8 +204,18 @@ struct StringLowering : public StringGathering { // imports. bool assertUTF8; - StringLowering(bool useMagicImports = false, bool assertUTF8 = false) - : useMagicImports(useMagicImports), assertUTF8(assertUTF8) { + // 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 features + // (we run it during the optimization pipeline, which should not do such + // things). + bool paired; + + StringLowering(bool useMagicImports = false, + bool assertUTF8 = false, + bool paired = false) + : useMagicImports(useMagicImports), assertUTF8(assertUTF8), paired(paired) { // If we are asserting valid UTF-8, we must be using magic imports. assert(!assertUTF8 || useMagicImports); } @@ -215,6 +225,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); @@ -231,10 +253,12 @@ struct StringLowering : public StringGathering { replaceNulls(module); // ReFinalize to apply all the above changes. - ReFinalize().run(getPassRunner(), module); + ReFinalize().run(runner, module); - // Disable the feature here after we lowered everything away. - module->features.disable(FeatureSet::Strings); + if (!paired) { + // Disable the feature here after we lowered everything away. + module->features.disable(FeatureSet::Strings); + } } void makeImports(Module* module) { @@ -553,10 +577,15 @@ 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* createStringLoweringPairedPass() { + // Use magic imports; no asserts; paired. + return new StringLowering(true, false, true); +} } // namespace wasm diff --git a/src/passes/pass.cpp b/src/passes/pass.cpp index 2042bc71d3a..acb8dc6dd79 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); + 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); @@ -536,6 +539,11 @@ void PassRegistry::registerPasses() { "same as string-lowering-magic-imports, but raise a fatal error " "if there are invalid strings", createStringLoweringMagicImportAssertPass); + 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)", + createStringLoweringPairedPass); registerPass( "strip", "deprecated; same as strip-debug", createStripDebugPass); registerPass("stack-check", @@ -724,6 +732,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-paired"); + } // 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 @@ -794,16 +807,17 @@ 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"); + // 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. + 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"); } - // may allow more inlining/dae/etc., need --converge for that + // May allow more inlining/dae/etc., need --converge for that addIfNoDWARFIssues("directize"); } diff --git a/src/passes/passes.h b/src/passes/passes.h index e051e466e72..49a5ce28db1 100644 --- a/src/passes/passes.h +++ b/src/passes/passes.h @@ -162,9 +162,11 @@ Pass* createSimplifyLocalsNoTeeNoStructurePass(); Pass* createStackCheckPass(); Pass* createStringGatheringPass(); Pass* createStringLiftingPass(); +Pass* createStringLiftingPairedPass(); Pass* createStringLoweringPass(); Pass* createStringLoweringMagicImportPass(); Pass* createStringLoweringMagicImportAssertPass(); +Pass* createStringLoweringPairedPass(); Pass* createStripDebugPass(); Pass* createStripDWARFPass(); Pass* createStripProducersPass(); 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/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 { 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_O3_strings.wast b/test/lit/passes/O2_O3_strings.wast new file mode 100644 index 00000000000..1018428d20a --- /dev/null +++ b/test/lit/passes/O2_O3_strings.wast @@ -0,0 +1,55 @@ +;; 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 -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 -O3 -all -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)))) + + ;; CHECK: (type $0 (func (result (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_\"foobarbar\"") + ;; CHECK-NEXT: ) + (func $string.concat (export "string.concat") (result (ref extern)) + (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) + ) + ) +) diff --git a/test/lit/passes/O2_strings.wast b/test/lit/passes/O2_strings.wast new file mode 100644 index 00000000000..9af7addc43d --- /dev/null +++ b/test/lit/passes/O2_strings.wast @@ -0,0 +1,46 @@ +;; 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, which should lift, optimize, and lower strings. Also see that +;; -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 --no-string-lowering -all -S -o - | filecheck %s --check-prefix=NO_LOWER + +(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) + ) + ) +)