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.

4 files changed, 92 insertions, 9 deletions

diff --git a/src/passes/Precompute.cpp b/src/passes/Precompute.cpp
index 5baf2331f..89e618ab3 100644
--- a/src/passes/Precompute.cpp
+++ b/src/passes/Precompute.cpp
@@ -799,9 +799,11 @@ private:
     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 3baec4409..a7d8c3487 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<StringConst>();
+}
+
 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<GlobalSet>()) {
-      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 29f64ba07..48db023bd 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 000000000..813e2964c
--- /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)
+  )
+)