From 9090ce56fcc67e15005aeedc59c6bc6773220f11 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 23 Jan 2024 17:30:38 -0800 Subject: [PATCH] Stop propagating/inlining string constants (#6234) This causes overhead atm since in VMs executing a string.const will actually allocate a string, and more copies means more allocations. For now, just do not add more. This required changes to two passes: SimplifyGlobals and Precompute. --- src/passes/Precompute.cpp | 6 +- src/passes/SimplifyGlobals.cpp | 22 +++++-- test/lit/passes/precompute-gc.wast | 8 ++- test/lit/passes/simplify-globals-strings.wast | 65 +++++++++++++++++++ 4 files changed, 92 insertions(+), 9 deletions(-) create mode 100644 test/lit/passes/simplify-globals-strings.wast diff --git a/src/passes/Precompute.cpp b/src/passes/Precompute.cpp index 5baf2331f14..89e618ab3e1 100644 --- a/src/passes/Precompute.cpp +++ b/src/passes/Precompute.cpp @@ -799,9 +799,11 @@ struct Precompute if (type.isFunction()) { return true; } - // We can emit a StringConst for a string constant. + // We can emit a StringConst for a string constant in principle, but we do + // not want to as that might cause more allocations to happen. See similar + // code in SimplifyGlobals. if (type.isString()) { - return true; + return false; } // All other reference types cannot be precomputed. Even an immutable GC // reference is not currently something this pass can handle, as it will diff --git a/src/passes/SimplifyGlobals.cpp b/src/passes/SimplifyGlobals.cpp index 3baec440931..a7d8c348746 100644 --- a/src/passes/SimplifyGlobals.cpp +++ b/src/passes/SimplifyGlobals.cpp @@ -52,6 +52,20 @@ namespace wasm { namespace { +// Checks if an expression is a constant, and one that we can copy without +// downsides. This is the set of constant values that we will inline from +// globals. +bool isCopyableConstant(Expression* curr) { + // Anything that is truly constant is suitable for us, *except* for string + // constants, which in VMs may be implemented not as a constant but as an + // allocation. We prefer to keep string.const in globals where any such + // allocation only happens once (note that that makes them equivalent to + // strings imported from JS, which would be in imported globals, which are + // similarly not optimizable). + // TODO: revisit this if/when VMs implement and optimize string.const. + return Properties::isConstantExpression(curr) && !curr->is(); +} + struct GlobalInfo { // Whether the global is imported and exported. bool imported = false; @@ -358,7 +372,7 @@ struct ConstantGlobalApplier void visitExpression(Expression* curr) { if (auto* set = curr->dynCast()) { - if (Properties::isConstantExpression(set->value)) { + if (isCopyableConstant(set->value)) { currConstantGlobals[set->name] = getLiteralsFromConstExpression(set->value); } else { @@ -369,7 +383,7 @@ struct ConstantGlobalApplier // Check if the global is known to be constant all the time. if (constantGlobals->count(get->name)) { auto* global = getModule()->getGlobal(get->name); - assert(Properties::isConstantExpression(global->init)); + assert(isCopyableConstant(global->init)); replaceCurrent(ExpressionManipulator::copy(global->init, *getModule())); replaced = true; return; @@ -671,7 +685,7 @@ struct SimplifyGlobals : public Pass { // go, as well as applying them where possible. for (auto& global : module->globals) { if (!global->imported()) { - if (Properties::isConstantExpression(global->init)) { + if (isCopyableConstant(global->init)) { constantGlobals[global->name] = getLiteralsFromConstExpression(global->init); } else { @@ -695,7 +709,7 @@ struct SimplifyGlobals : public Pass { NameSet constantGlobals; for (auto& global : module->globals) { if (!global->mutable_ && !global->imported() && - Properties::isConstantExpression(global->init)) { + isCopyableConstant(global->init)) { constantGlobals.insert(global->name); } } diff --git a/test/lit/passes/precompute-gc.wast b/test/lit/passes/precompute-gc.wast index 29f64ba0771..48db023bda3 100644 --- a/test/lit/passes/precompute-gc.wast +++ b/test/lit/passes/precompute-gc.wast @@ -1021,10 +1021,10 @@ ;; CHECK-NEXT: (string.const "hello, world") ;; CHECK-NEXT: ) ;; CHECK-NEXT: (call $strings - ;; CHECK-NEXT: (string.const "hello, world") + ;; CHECK-NEXT: (local.get $s) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (call $strings - ;; CHECK-NEXT: (string.const "hello, world") + ;; CHECK-NEXT: (local.get $s) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) (func $strings (param $param (ref string)) @@ -1032,7 +1032,9 @@ (local.set $s (string.const "hello, world") ) - ;; The constant string should be propagated twice, to both of these calls. + ;; The constant string could be propagated twice, to both of these calls, but + ;; we do not do so as it might cause more allocations to happen. + ;; TODO if VMs optimize that, handle it (call $strings (local.get $s) ) diff --git a/test/lit/passes/simplify-globals-strings.wast b/test/lit/passes/simplify-globals-strings.wast new file mode 100644 index 00000000000..813e2964ce4 --- /dev/null +++ b/test/lit/passes/simplify-globals-strings.wast @@ -0,0 +1,65 @@ +;; 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. + +;; We do not "inline" strings from globals, as that might cause more +;; allocations to happen. TODO if VMs optimize that, remove this + +;; RUN: foreach %s %t wasm-opt --simplify-globals -all -S -o - | filecheck %s + +;; Also test with -O3 --gufa to see that no other pass does this kind of thing, +;; as extra coverage. + +;; RUN: foreach %s %t wasm-opt -O3 --gufa -all -S -o - | filecheck %s --check-prefix=O3GUF + +(module + ;; CHECK: (type $0 (func (result anyref))) + + ;; CHECK: (global $string stringref (string.const "one")) + ;; O3GUF: (type $0 (func (result anyref))) + + ;; O3GUF: (global $string (ref string) (string.const "one")) + (global $string stringref (string.const "one")) + + ;; CHECK: (global $string-mut (mut stringref) (string.const "two")) + ;; O3GUF: (global $string-mut (mut (ref string)) (string.const "two")) + (global $string-mut (mut stringref) (string.const "two")) + + ;; CHECK: (export "global" (func $global)) + ;; O3GUF: (export "global" (func $global)) + (export "global" (func $global)) + + ;; CHECK: (export "written" (func $written)) + ;; O3GUF: (export "written" (func $written)) + (export "written" (func $written)) + + ;; CHECK: (func $global (type $0) (result anyref) + ;; CHECK-NEXT: (global.get $string) + ;; CHECK-NEXT: ) + ;; O3GUF: (func $global (type $0) (result anyref) + ;; O3GUF-NEXT: (global.get $string) + ;; O3GUF-NEXT: ) + (func $global (result anyref) + ;; This should not turn into "one". + (global.get $string) + ) + + ;; CHECK: (func $written (type $0) (result anyref) + ;; CHECK-NEXT: (global.set $string-mut + ;; CHECK-NEXT: (string.const "three") + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (global.get $string-mut) + ;; CHECK-NEXT: ) + ;; O3GUF: (func $written (type $0) (result anyref) + ;; O3GUF-NEXT: (global.set $string-mut + ;; O3GUF-NEXT: (string.const "three") + ;; O3GUF-NEXT: ) + ;; O3GUF-NEXT: (global.get $string-mut) + ;; O3GUF-NEXT: ) + (func $written (result anyref) + (global.set $string-mut + (string.const "three") + ) + ;; This should not turn into "three". + (global.get $string-mut) + ) +)