2 files changed, 87 insertions, 24 deletions

diff --git a/src/ir/localize.h b/src/ir/localize.h
index 270901c11..d44fb9be5 100644
--- a/src/ir/localize.h
+++ b/src/ir/localize.h
@@ -46,28 +46,50 @@ struct Localizer {
 
 // Replaces all children with gets of locals, if they have any effects that
 // interact with any of the others, or if they have side effects which cannot be
-// removed.
+// removed. Also replace unreachable things with an unreachable, leaving in
+// place only things without interacting effects. For example:
 //
-// After this, the original input has only local.gets as inputs, or other things
-// that have no interacting effects, and so those children can be reordered
-// and/or removed as needed.
+//  (parent
+//    (call $foo)
+//    (br $out)
+//    (i32.const)
+//  )
 //
-// The sets of the locals are emitted on a |sets| property on the class. Those
-// must be emitted right before the input.
+// =>
 //
-// This stops at the first unreachable child, as there is no code executing
-// after that point anyhow.
+//  (local.set $temp.foo
+//    (call $foo)            ;; moved out
+//  )
+//  (br $out)                ;; moved out
+//  (parent
+//    (local.get $temp.foo)  ;; value saved to a local
+//    (unreachable)          ;; complex effect replaced by unreachable
+//    (i32.const)
+//  )
+//
+// After this it is safe to reorder and remove things from the parent: all
+// interesting interactions happen before the parent.
+//
+// Typical usage is to call getReplacement() will produces the entire output
+// just shown (i.e., possible initial local.sets and other stuff that was pulled
+// out, followed by the parent, as relevant). Note that getReplacement() may
+// omit the parent, if it had an unreachable child. That is useful behavior in
+// that it removes unneeded code (& otherwise some users of this code would need
+// to write their own removal logic). However, that does imply that it is valid
+// to remove the parent in such cases, which is not so for e.g. br when it is
+// the last thing keeping a block reachable. Calling this with something like a
+// struct.new or a call (the current intended users) is valid; if we want to
+// generalize this fully then we need to make changes here.
 //
 // TODO: use in more places
 struct ChildLocalizer {
-  std::vector<LocalSet*> sets;
-
-  ChildLocalizer(Expression* input,
+  ChildLocalizer(Expression* parent,
                  Function* func,
-                 Module* wasm,
-                 const PassOptions& options) {
-    Builder builder(*wasm);
-    ChildIterator iterator(input);
+                 Module& wasm,
+                 const PassOptions& options)
+    : parent(parent), wasm(wasm) {
+    Builder builder(wasm);
+    ChildIterator iterator(parent);
     auto& children = iterator.children;
     auto num = children.size();
 
@@ -77,7 +99,7 @@ struct ChildLocalizer {
       // The children are in reverse order in ChildIterator, but we want to
       // process them in the normal order.
       auto* child = *children[num - 1 - i];
-      effects.emplace_back(options, *wasm, child);
+      effects.emplace_back(options, wasm, child);
     }
 
     // Go through the children and move to locals those that we need to.
@@ -85,7 +107,23 @@ struct ChildLocalizer {
       auto** childp = children[num - 1 - i];
       auto* child = *childp;
       if (child->type == Type::unreachable) {
-        break;
+        // Move the child out, and put an unreachable in its place (note that we
+        // don't need an actual set here, as there is no value to set to a
+        // local).
+        sets.push_back(child);
+        *childp = builder.makeUnreachable();
+        hasUnreachableChild = true;
+        continue;
+      }
+
+      if (hasUnreachableChild) {
+        // Once we pass one unreachable, we only need to copy the children over.
+        // (The only reason we still need them is that they may be needed for
+        // validation, e.g. if one contains a break to a block that is the only
+        // reason the block has type none.)
+        sets.push_back(builder.makeDrop(child));
+        *childp = builder.makeUnreachable();
+        continue;
       }
 
       // Use a local if we need to. That is the case either if this has side
@@ -106,6 +144,36 @@ struct ChildLocalizer {
       }
     }
   }
+
+  // Helper that gets a replacement for the parent: a block containing the
+  // sets + the parent. This will not contain the parent if we don't need it
+  // (if it was never reached).
+  Expression* getReplacement() {
+    if (sets.empty()) {
+      // Nothing to add.
+      return parent;
+    }
+
+    auto* block = Builder(wasm).makeBlock();
+    block->list.set(sets);
+    if (hasUnreachableChild) {
+      // If there is an unreachable child then we do not need the parent at all,
+      // and we know the type is unreachable.
+      block->type = Type::unreachable;
+    } else {
+      // Otherwise, add the parent and finalize.
+      block->list.push_back(parent);
+      block->finalize();
+    }
+    return block;
+  }
+
+private:
+  Expression* parent;
+  Module& wasm;
+
+  std::vector<Expression*> sets;
+  bool hasUnreachableChild = false;
 };
 
 } // namespace wasm
diff --git a/src/passes/GlobalTypeOptimization.cpp b/src/passes/GlobalTypeOptimization.cpp
index 8ef6a3b89..e5cd4e410 100644
--- a/src/passes/GlobalTypeOptimization.cpp
+++ b/src/passes/GlobalTypeOptimization.cpp
@@ -365,13 +365,8 @@ struct GlobalTypeOptimization : public Pass {
           if (!func) {
             Fatal() << "TODO: side effects in removed fields in globals\n";
           }
-          auto* block = Builder(*getModule()).makeBlock();
-          auto sets =
-            ChildLocalizer(curr, func, getModule(), getPassOptions()).sets;
-          block->list.set(sets);
-          block->list.push_back(curr);
-          block->finalize(curr->type);
-          replaceCurrent(block);
+          ChildLocalizer localizer(curr, func, *getModule(), getPassOptions());
+          replaceCurrent(localizer.getReplacement());
         }
 
         // Remove the unneeded operands.