[NFC] Use SubtypingDiscoverer in StringLowering (#6325)

This replaces horrible hacks to find which nulls need to switch (from none to
noext) with general code using SubtypingDiscoverer. That helper is aware of
where each expression is written, so we can find those nulls trivially.

This is NFC on existing usage but should fix any remaining bugs with null
constants.

2 files changed, 66 insertions, 55 deletions

diff --git a/src/ir/subtype-exprs.h b/src/ir/subtype-exprs.h
index 7ebb0b017..056b373c2 100644
--- a/src/ir/subtype-exprs.h
+++ b/src/ir/subtype-exprs.h
@@ -27,7 +27,7 @@ namespace wasm {
 // Analyze subtyping relationships between expressions. This must CRTP with a
 // class that implements:
 //
-//  * noteSubType(A, B) indicating A must be a subtype of B
+//  * noteSubtype(A, B) indicating A must be a subtype of B
 //  * noteCast(A, B) indicating A is cast to B
 //
 // There must be multiple versions of each of those, supporting A and B being
@@ -35,20 +35,20 @@ namespace wasm {
 // indicating a flexible requirement that depends on the type of that
 // expression. Specifically:
 //
-//  * noteSubType(Type, Type) - A constraint not involving expressions at all,
+//  * noteSubtype(Type, Type) - A constraint not involving expressions at all,
 //                              for example, an element segment's type must be
 //                              a subtype of the corresponding table's.
-//  * noteSubType(HeapType, HeapType) - Ditto, with heap types, for example in a
+//  * noteSubtype(HeapType, HeapType) - Ditto, with heap types, for example in a
 //                                      CallIndirect.
-//  * noteSubType(Type, Expression) - A fixed type must be a subtype of an
+//  * noteSubtype(Type, Expression) - A fixed type must be a subtype of an
 //                                    expression's type, for example, in BrOn
 //                                    (the declared sent type must be a subtype
 //                                    of the block we branch to).
-//  * noteSubType(Expression, Type) - An expression's type must be a subtype of
+//  * noteSubtype(Expression, Type) - An expression's type must be a subtype of
 //                                    a fixed type, for example, a Call operand
 //                                    must be a subtype of the signature's
 //                                    param.
-//  * noteSubType(Expression, Expression) - An expression's type must be a
+//  * noteSubtype(Expression, Expression) - An expression's type must be a
 //                                          subtype of anothers, for example,
 //                                          a block and its last child.
 //
@@ -59,6 +59,17 @@ namespace wasm {
 //  * noteCast(Expression, Expression) - An expression's type is cast to
 //                                       another, for example, in RefCast.
 //
+// The concrete signatures are:
+//
+//      void noteSubtype(Type, Type);
+//      void noteSubtype(HeapType, HeapType);
+//      void noteSubtype(Type, Expression*);
+//      void noteSubtype(Expression*, Type);
+//      void noteSubtype(Expression*, Expression*);
+//      void noteCast(HeapType, HeapType);
+//      void noteCast(Expression*, Type);
+//      void noteCast(Expression*, Expression*);
+//
 // Note that noteCast(Type, Type) and noteCast(Type, Expression) never occur and
 // do not need to be implemented.
 //
diff --git a/src/passes/StringLowering.cpp b/src/passes/StringLowering.cpp
index c70691ea1..c04695c50 100644
--- a/src/passes/StringLowering.cpp
+++ b/src/passes/StringLowering.cpp
@@ -34,6 +34,7 @@
 
 #include "ir/module-utils.h"
 #include "ir/names.h"
+#include "ir/subtype-exprs.h"
 #include "ir/type-updating.h"
 #include "ir/utils.h"
 #include "pass.h"
@@ -199,6 +200,12 @@ struct StringLowering : public StringGathering {
     // Replace string.* etc. operations with imported ones.
     replaceInstructions(module);
 
+    // Replace ref.null types as needed.
+    replaceNulls(module);
+
+    // 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);
   }
@@ -433,65 +440,58 @@ struct StringLowering : public StringGathering {
             WASM_UNREACHABLE("TODO: all string.slice*");
         }
       }
+    };
 
-      // Additional hacks: We fix up a none that should be noext. Before the
-      // lowering we can use none for stringref, but after we must use noext as
-      // the two do not share a bottom type.
-      //
-      // The code here and in the visitors below is course wildly insufficient
-      // (we need selects and blocks and all other joins, and not just nulls,
-      // etc.) but in practice this is enough for now. TODO extend as needed
-      void ensureNullIsExt(Expression* curr) {
-        if (auto* null = curr->dynCast<RefNull>()) {
-          null->finalize(HeapType::noext);
-        }
-      }
+    Replacer replacer(*this);
+    replacer.run(getPassRunner(), module);
+    replacer.walkModuleCode(module);
+  }
 
-      bool isExt(Type type) {
-        return type.isRef() && type.getHeapType() == HeapType::ext;
+  // A ref.null of none needs to be noext if it is going to a location of type
+  // stringref.
+  void replaceNulls(Module* module) {
+    // Use SubtypingDiscoverer to find when a ref.null of none flows into a
+    // place that has been changed from stringref to externref.
+    struct NullFixer
+      : public WalkerPass<
+          ControlFlowWalker<NullFixer, SubtypingDiscoverer<NullFixer>>> {
+      // Hooks for SubtypingDiscoverer.
+      void noteSubtype(Type, Type) {
+        // Nothing to do for pure types.
       }
-
-      void visitIf(If* curr) {
-        // If the if outputs an ext, fix up the arms to contain proper nulls for
-        // that type.
-        if (isExt(curr->type)) {
-          ensureNullIsExt(curr->ifTrue);
-          ensureNullIsExt(curr->ifFalse);
-        }
+      void noteSubtype(HeapType, HeapType) {
+        // Nothing to do for pure types.
       }
-
-      void visitCall(Call* curr) {
-        auto targetSig =
-          getModule()->getFunction(curr->target)->type.getSignature();
-        for (Index i = 0; i < curr->operands.size(); i++) {
-          if (isExt(targetSig.params[i])) {
-            ensureNullIsExt(curr->operands[i]);
-          }
-        }
+      void noteSubtype(Type, Expression*) {
+        // Nothing to do for a subtype of an expression.
       }
-
-      void visitStructNew(StructNew* curr) {
-        if (curr->type == Type::unreachable || curr->operands.empty()) {
-          return;
-        }
-
-        // If we write a none into an ext field, fix that.
-        auto& fields = curr->type.getHeapType().getStruct().fields;
-        assert(curr->operands.size() == fields.size());
-        for (Index i = 0; i < fields.size(); i++) {
-          if (isExt(fields[i].type)) {
-            ensureNullIsExt(curr->operands[i]);
+      void noteSubtype(Expression* a, Type b) {
+        // This is the case we care about: if |a| is a null that must be a
+        // subtype of ext then we fix that up.
+        if (b.isRef() && b.getHeapType().getTop() == HeapType::ext) {
+          if (auto* null = a->dynCast<RefNull>()) {
+            null->finalize(HeapType::noext);
           }
         }
       }
+      void noteSubtype(Expression* a, Expression* b) {
+        // Only the type matters of the place we assign to.
+        noteSubtype(a, b->type);
+      }
+      void noteCast(HeapType, HeapType) {
+        // Casts do not concern us.
+      }
+      void noteCast(Expression*, Type) {
+        // Casts do not concern us.
+      }
+      void noteCast(Expression*, Expression*) {
+        // Casts do not concern us.
+      }
     };
 
-    Replacer replacer(*this);
-    replacer.run(getPassRunner(), module);
-    replacer.walkModuleCode(module);
-
-    // ReFinalize to apply changes to parents.
-    ReFinalize().run(getPassRunner(), module);
+    NullFixer fixer;
+    fixer.run(getPassRunner(), module);
+    fixer.walkModuleCode(module);
   }
 };