From fca13cee080d5c50154db81d24d67ad0180ee3cc Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Wed, 6 Mar 2024 16:31:57 -0800
Subject: [PATCH 01/49] yolo

---
 src/wasm/wasm-validator.cpp | 28 +++++++++++++++++++++++++++-
 src/wasm/wasm.cpp           | 13 ++++---------
 2 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp
index 4bb5561604a..7923da4d87c 100644
--- a/src/wasm/wasm-validator.cpp
+++ b/src/wasm/wasm-validator.cpp
@@ -247,6 +247,13 @@ struct FunctionValidator : public WalkerPass<PostWalker<FunctionValidator>> {
 
   void noteLabelName(Name name);
 
+  // Maps a label name (block etc.) to the br_ifs targetting it.
+  std::unordered_map<Name, std::vector<Break*>> labelBrIfs;
+
+  // Given a label and the type of it, like a block's name and its type,
+  // validate all br_ifs we've seen that target it.
+  void validateBrIfs(Name label, Type type);
+
 public:
   // visitors
 
@@ -612,6 +619,18 @@ void FunctionValidator::noteLabelName(Name name) {
     "names in Binaryen IR must be unique - IR generators must ensure that");
 }
 
+void FunctionValidator::validateBrIfs(Name label, Type type) {
+  if (auto iter = labelBrIfs.find(name); iter != labelBrIfs.end()) {
+    for (auto* brIf : iter->second) {
+      assert(brIf->value && brIf->condition); // This must be an actual br_if.
+      shouldBeEqual(type,
+                    brIf->type,
+                    brIf,
+                    "br_ifs to a block must have the block's type");
+    }
+  }
+}
+
 void FunctionValidator::validatePoppyExpression(Expression* curr) {
   if (curr->type == Type::unreachable) {
     shouldBeTrue(StackUtils::mayBeUnreachable(curr),
@@ -652,8 +671,8 @@ void FunctionValidator::visitBlock(Block* curr) {
       curr,
       "Multivalue block type require multivalue [--enable-multivalue]");
   }
-  // if we are break'ed to, then the value must be right for us
   if (curr->name.is()) {
+    // If we are break'ed to, then the value must be right for us
     noteLabelName(curr->name);
     auto iter = breakTypes.find(curr->name);
     assert(iter != breakTypes.end()); // we set it ourselves
@@ -669,6 +688,8 @@ void FunctionValidator::visitBlock(Block* curr) {
                       "break type must be a subtype of the target block type");
     }
     breakTypes.erase(iter);
+
+    validateBrIfs(curr->name, curr->type);
   }
   switch (getFunction()->profile) {
     case IRProfile::Normal:
@@ -781,6 +802,8 @@ void FunctionValidator::visitLoop(Loop* curr) {
                     "breaks to a loop cannot pass a value");
     }
     breakTypes.erase(iter);
+
+    validateBrIfs(curr->name, curr->type);
   }
   if (curr->type == Type::none) {
     shouldBeFalse(curr->body->type.isConcrete(),
@@ -890,6 +913,9 @@ void FunctionValidator::visitBreak(Break* curr) {
                  curr,
                  "break condition must be i32");
   }
+  if (curr->condition && curr->value) {
+    labelBrIfs[curr->name].push_back(curr);
+  }
 }
 
 void FunctionValidator::visitSwitch(Switch* curr) {
diff --git a/src/wasm/wasm.cpp b/src/wasm/wasm.cpp
index 374a97e1902..5e11047b764 100644
--- a/src/wasm/wasm.cpp
+++ b/src/wasm/wasm.cpp
@@ -251,17 +251,12 @@ void Break::finalize() {
   if (condition) {
     if (condition->type == Type::unreachable) {
       type = Type::unreachable;
-    } else if (value) {
-      // N.B. This is not correct wrt the spec, which mandates that it be the
-      // type of the block we target. In practice this does not matter because
-      // the br_if return value is not really used in the wild. To fix this,
-      // we'd need to do something like what we do for local.tee's type, which
-      // is to fix it up in a way that is aware of function-level context and
-      // not just the instruction itself (which would be a pain).
-      type = value->type;
-    } else {
+    } else if (!value) {
       type = Type::none;
     }
+    // Note that we do nothing for a reachable br_if. Its type must match the
+    // type of the block it targets, which we do not see here, so it is up to
+    // optimization passes to manually update it as (rarely) needed.
   } else {
     type = Type::unreachable;
   }

From fc943d67b7cd796b34c1a20ea73936b2a882af0b Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Wed, 6 Mar 2024 16:42:09 -0800
Subject: [PATCH 02/49] fixen

---
 src/tools/fuzzing/fuzzing.cpp | 2 +-
 src/wasm-builder.h            | 9 ++++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/tools/fuzzing/fuzzing.cpp b/src/tools/fuzzing/fuzzing.cpp
index c1625d726a4..450cd8bab02 100644
--- a/src/tools/fuzzing/fuzzing.cpp
+++ b/src/tools/fuzzing/fuzzing.cpp
@@ -1536,7 +1536,7 @@ Expression* TranslateToFuzzReader::makeBreak(Type type) {
         // we need to break to a proper place
         continue;
       }
-      auto* ret = builder.makeBreak(name, make(type), condition);
+      auto* ret = builder.makeBreak(name, make(type), condition, valueType);
       funcContext->hangStack.pop_back();
       return ret;
     } else if (type == Type::none) {
diff --git a/src/wasm-builder.h b/src/wasm-builder.h
index eb1874c3b81..8bede5b1451 100644
--- a/src/wasm-builder.h
+++ b/src/wasm-builder.h
@@ -239,11 +239,18 @@ class Builder {
   }
   Break* makeBreak(Name name,
                    Expression* value = nullptr,
-                   Expression* condition = nullptr) {
+                   Expression* condition = nullptr,
+                   std::optional<Type> type = std::nullopt) {
     auto* ret = wasm.allocator.alloc<Break>();
     ret->name = name;
     ret->value = value;
     ret->condition = condition;
+    if (value && condition) {
+      // This is a br_if, for whom we must receive the type, as the type must
+      // match the block it targets (which we do not know here).
+      assert(type);
+      ret->type = *type;
+    }
     ret->finalize();
     return ret;
   }

From e91855514ba0c711301eaabd550256d307b0478b Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Wed, 6 Mar 2024 16:49:30 -0800
Subject: [PATCH 03/49] start

---
 src/wasm/wasm-binary.cpp   | 4 ++++
 src/wasm/wasm-s-parser.cpp | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/src/wasm/wasm-binary.cpp b/src/wasm/wasm-binary.cpp
index efb65bb5095..b14148a9658 100644
--- a/src/wasm/wasm-binary.cpp
+++ b/src/wasm/wasm-binary.cpp
@@ -4513,6 +4513,10 @@ void WasmBinaryReader::visitBreak(Break* curr, uint8_t code) {
   }
   if (target.type.isConcrete()) {
     curr->value = popTypedExpression(target.type);
+    if (curr->condition) {
+      // This is a br_if; set the type based on the target.
+      curr->type = target.type;
+    }
   }
   curr->finalize();
 }
diff --git a/src/wasm/wasm-s-parser.cpp b/src/wasm/wasm-s-parser.cpp
index 44ba8435113..f9c59c4789e 100644
--- a/src/wasm/wasm-s-parser.cpp
+++ b/src/wasm/wasm-s-parser.cpp
@@ -2599,6 +2599,11 @@ Expression* SExpressionWasmBuilder::makeBreak(Element& s, bool isConditional) {
   } else {
     ret->value = parseExpression(s[i]);
   }
+#if 0
+  if (isConditional && ret->value) {
+    ret->type = 
+  }
+#endif
   ret->finalize();
   return ret;
 }

From 0dae9600548139fb163b0f0f9b508be27f93b369 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Wed, 6 Mar 2024 16:59:40 -0800
Subject: [PATCH 04/49] start

---
 src/wasm-s-parser.h         | 6 ++++++
 src/wasm/wasm-s-parser.cpp  | 9 ++++++---
 src/wasm/wasm-validator.cpp | 2 +-
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/src/wasm-s-parser.h b/src/wasm-s-parser.h
index e1d9dab47ee..5853a9e65c7 100644
--- a/src/wasm-s-parser.h
+++ b/src/wasm-s-parser.h
@@ -206,6 +206,12 @@ class SExpressionWasmBuilder {
   bool isType(IString str) { return stringToType(str, true) != Type::none; }
   HeapType getFunctionType(Name name, Element& s);
 
+  // Maps block names to their types. This is built up as we go so that things
+  // like br_if can query the type of the thing they target. Note that we don't
+  // note the types of loops because breaks to them must not send a value
+  // anyhow.
+  std::unordered_map<Name, Type> blockTypes;
+
 public:
   Expression* parseExpression(Element* s) { return parseExpression(*s); }
   Expression* parseExpression(Element& s);
diff --git a/src/wasm/wasm-s-parser.cpp b/src/wasm/wasm-s-parser.cpp
index f9c59c4789e..afad9d6b330 100644
--- a/src/wasm/wasm-s-parser.cpp
+++ b/src/wasm/wasm-s-parser.cpp
@@ -1649,6 +1649,7 @@ Expression* SExpressionWasmBuilder::makeBlock(Element& s) {
     curr->name = nameMapper.pushLabelName(sName);
     // block signature
     curr->type = parseBlockType(s, i);
+    blockTypes[curr->name] = curr->type;
     if (i >= s.size()) {
       break; // empty block
     }
@@ -2599,11 +2600,13 @@ Expression* SExpressionWasmBuilder::makeBreak(Element& s, bool isConditional) {
   } else {
     ret->value = parseExpression(s[i]);
   }
-#if 0
   if (isConditional && ret->value) {
-    ret->type = 
+    auto iter = blockTypes.find(ret->name);
+    if (iter == blockTypes.end()) {
+      throw SParseException("br_if to invalid target", s);
+    }
+    ret->type = iter->second;
   }
-#endif
   ret->finalize();
   return ret;
 }
diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp
index 7923da4d87c..29b10141647 100644
--- a/src/wasm/wasm-validator.cpp
+++ b/src/wasm/wasm-validator.cpp
@@ -803,7 +803,7 @@ void FunctionValidator::visitLoop(Loop* curr) {
     }
     breakTypes.erase(iter);
 
-    validateBrIfs(curr->name, curr->type);
+    //validateBrIfs(curr->name, curr->type); // XXX breaks must have type none, so no br_ifs!
   }
   if (curr->type == Type::none) {
     shouldBeFalse(curr->body->type.isConcrete(),

From 5a0e7455a1f7494bfdaa901cfc13e3c1fdcb0e9d Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Wed, 6 Mar 2024 17:12:31 -0800
Subject: [PATCH 05/49] start

---
 src/wasm/wasm-s-parser.cpp |  3 +++
 src/wasm/wasm.cpp          | 10 +++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/wasm/wasm-s-parser.cpp b/src/wasm/wasm-s-parser.cpp
index afad9d6b330..e1a4f099f52 100644
--- a/src/wasm/wasm-s-parser.cpp
+++ b/src/wasm/wasm-s-parser.cpp
@@ -2600,14 +2600,17 @@ Expression* SExpressionWasmBuilder::makeBreak(Element& s, bool isConditional) {
   } else {
     ret->value = parseExpression(s[i]);
   }
+  std::cout << "maek break " << ret->condition << " : " << ret->value << '\n';
   if (isConditional && ret->value) {
     auto iter = blockTypes.find(ret->name);
     if (iter == blockTypes.end()) {
       throw SParseException("br_if to invalid target", s);
     }
     ret->type = iter->second;
+    std::cout << "  a.type " << ret->type << '\n';
   }
   ret->finalize();
+    std::cout << "  b.type " << ret->type << " : " << *ret << '\n';
   return ret;
 }
 
diff --git a/src/wasm/wasm.cpp b/src/wasm/wasm.cpp
index 5e11047b764..9c0002b8c1c 100644
--- a/src/wasm/wasm.cpp
+++ b/src/wasm/wasm.cpp
@@ -251,12 +251,16 @@ void Break::finalize() {
   if (condition) {
     if (condition->type == Type::unreachable) {
       type = Type::unreachable;
+    } else if (value) {
+      if (value->type == Type::unreachable) {
+        type = Type::unreachable;
+      }
+      // Note that we do nothing for a reachable br_if. Its type must match the
+      // type of the block it targets, which we do not see here, so it is up to
+      // optimization passes to manually update it as (rarely) needed.
     } else if (!value) {
       type = Type::none;
     }
-    // Note that we do nothing for a reachable br_if. Its type must match the
-    // type of the block it targets, which we do not see here, so it is up to
-    // optimization passes to manually update it as (rarely) needed.
   } else {
     type = Type::unreachable;
   }

From 25e7485927e10e721682170887a84728ab222f15 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Wed, 6 Mar 2024 17:13:07 -0800
Subject: [PATCH 06/49] start

---
 src/wasm/wasm-s-parser.cpp | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/wasm/wasm-s-parser.cpp b/src/wasm/wasm-s-parser.cpp
index e1a4f099f52..afad9d6b330 100644
--- a/src/wasm/wasm-s-parser.cpp
+++ b/src/wasm/wasm-s-parser.cpp
@@ -2600,17 +2600,14 @@ Expression* SExpressionWasmBuilder::makeBreak(Element& s, bool isConditional) {
   } else {
     ret->value = parseExpression(s[i]);
   }
-  std::cout << "maek break " << ret->condition << " : " << ret->value << '\n';
   if (isConditional && ret->value) {
     auto iter = blockTypes.find(ret->name);
     if (iter == blockTypes.end()) {
       throw SParseException("br_if to invalid target", s);
     }
     ret->type = iter->second;
-    std::cout << "  a.type " << ret->type << '\n';
   }
   ret->finalize();
-    std::cout << "  b.type " << ret->type << " : " << *ret << '\n';
   return ret;
 }
 

From 6bb5d7d42541a88d9643d4c079f748d5282151a0 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 09:20:19 -0800
Subject: [PATCH 07/49] fix

---
 src/passes/SimplifyLocals.cpp | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/passes/SimplifyLocals.cpp b/src/passes/SimplifyLocals.cpp
index 28b13980b2a..db852f01f88 100644
--- a/src/passes/SimplifyLocals.cpp
+++ b/src/passes/SimplifyLocals.cpp
@@ -608,11 +608,14 @@ struct SimplifyLocals
       auto* set = (*breakLocalSetPointer)->template cast<LocalSet>();
       if (br->condition) {
         br->value = set;
-        set->makeTee(this->getFunction()->getLocalType(set->index));
+        auto localType = this->getFunction()->getLocalType(set->index);
+        set->makeTee(localType);
         *breakLocalSetPointer =
           this->getModule()->allocator.template alloc<Nop>();
-        // in addition, as this is a conditional br that now has a value, it now
-        // returns a value, so it must be dropped
+        // In addition, as this is a conditional br that now has a value, it now
+        // returns a value, so we must update the type for that and also drop
+        // it.
+        br->type = localType;
         br->finalize();
         *brp = Builder(*this->getModule()).makeDrop(br);
       } else {

From e0118b3b2a8a802b0a1784b307f42953cf4aba99 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 10:04:45 -0800
Subject: [PATCH 08/49] fix

---
 src/wasm/wasm-s-parser.cpp | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/wasm/wasm-s-parser.cpp b/src/wasm/wasm-s-parser.cpp
index afad9d6b330..5ebbd8da374 100644
--- a/src/wasm/wasm-s-parser.cpp
+++ b/src/wasm/wasm-s-parser.cpp
@@ -2603,9 +2603,15 @@ Expression* SExpressionWasmBuilder::makeBreak(Element& s, bool isConditional) {
   if (isConditional && ret->value) {
     auto iter = blockTypes.find(ret->name);
     if (iter == blockTypes.end()) {
-      throw SParseException("br_if to invalid target", s);
+      // There is no block by that name, so this must target the function scope.
+      // We could also validate that it is in fact the function scope, but the
+      // main validator does that anyhow; all we need here is to generate valid
+      // IR if it is valid.
+      ret->type = currFunction->getResults();
+    } else {
+      // Get the type from the block.
+      ret->type = iter->second;
     }
-    ret->type = iter->second;
   }
   ret->finalize();
   return ret;

From de77ed375a0e4a709a67b732593f00c490708acd Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 10:27:24 -0800
Subject: [PATCH 09/49] fix

---
 src/wasm/wasm-ir-builder.cpp | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/wasm/wasm-ir-builder.cpp b/src/wasm/wasm-ir-builder.cpp
index a7c7439709a..b36ea2dce25 100644
--- a/src/wasm/wasm-ir-builder.cpp
+++ b/src/wasm/wasm-ir-builder.cpp
@@ -1114,7 +1114,14 @@ Result<> IRBuilder::makeBreak(Index label, bool isConditional) {
   // Use a dummy condition value if we need to pop a condition.
   curr.condition = isConditional ? &curr : nullptr;
   CHECK_ERR(visitBreak(&curr, label));
-  push(builder.makeBreak(curr.name, curr.value, curr.condition));
+  // If this is a br_if then we must provide the type (which is the type of the
+  // block we target).
+  std::optional<Type> type;
+  if (curr.value && curr.condition) {
+    // N.B. the label was already validated above when we did getLabelName.
+    type = scopeStack[scopeStack.size() - label - 1].getResultType();
+  }
+  push(builder.makeBreak(curr.name, curr.value, curr.condition, type));
   return Ok{};
 }
 

From e03e9a13714a57ed12e793f87895147a62dfe581 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 11:02:50 -0800
Subject: [PATCH 10/49] work

---
 src/passes/RemoveUnusedBrs.cpp | 50 ++++++++++++++++++++++++++++++----
 1 file changed, 44 insertions(+), 6 deletions(-)

diff --git a/src/passes/RemoveUnusedBrs.cpp b/src/passes/RemoveUnusedBrs.cpp
index 893cf9039bc..87df0c773ed 100644
--- a/src/passes/RemoveUnusedBrs.cpp
+++ b/src/passes/RemoveUnusedBrs.cpp
@@ -417,6 +417,11 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
             br->condition =
               builder.makeSelect(br->condition, curr->condition, zero);
           }
+          if (br->value) {
+            // Update the br_if's type based on the block.
+            assert(concreteBlockTypes.count(br->name));
+            br->type = concreteBlockTypes[br->name];
+          }
           br->finalize();
           replaceCurrent(Builder(*getModule()).dropIfConcretelyTyped(br));
           anotherCycle = true;
@@ -460,9 +465,9 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
   static void scan(RemoveUnusedBrs* self, Expression** currp) {
     self->pushTask(visitAny, currp);
 
-    auto* iff = (*currp)->dynCast<If>();
+    auto* curr = *currp;
 
-    if (iff) {
+    if (auto* iff = curr->dynCast<If>()) {
       if (iff->condition->type == Type::unreachable) {
         // avoid trying to optimize this, we never reach it anyhow
         return;
@@ -478,11 +483,21 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
       self->pushTask(scan, &iff->ifTrue);
       self->pushTask(clear, currp); // clear all flow after the condition
       self->pushTask(scan, &iff->condition);
-    } else {
-      super::scan(self, currp);
+      return;
     }
+
+    if (auto* block = curr->dynCast<Block>()) {
+      if (block->type.isConcrete()) {
+        self->concreteBlockTypes[block->name] = block->type;
+      }
+    }
+    super::scan(self, currp);
   }
 
+  // We track the types of blocks that have concrete types, as they may have
+  // br_ifs that target them, whose type depends on the block type.
+  std::unordered_map<Name, Type> concreteBlockTypes;
+
   // optimizes a loop. returns true if we made changes
   bool optimizeLoop(Loop* loop) {
     // if a loop ends in
@@ -1066,7 +1081,7 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
             // we are an if-else where the ifTrue is a break without a
             // condition, so we can do this
             ifTrueBreak->condition = iff->condition;
-            ifTrueBreak->finalize();
+            updateBrIfType(ifTrueBreak);
             list[i] = Builder(*getModule()).dropIfConcretelyTyped(ifTrueBreak);
             ExpressionManipulator::spliceIntoBlock(curr, i + 1, iff->ifFalse);
             continue;
@@ -1080,7 +1095,7 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
                                 *getModule())) {
             ifFalseBreak->condition =
               Builder(*getModule()).makeUnary(EqZInt32, iff->condition);
-            ifFalseBreak->finalize();
+            updateBrIfType(ifFalseBreak);
             list[i] = Builder(*getModule()).dropIfConcretelyTyped(ifFalseBreak);
             ExpressionManipulator::spliceIntoBlock(curr, i + 1, iff->ifTrue);
             continue;
@@ -1676,6 +1691,29 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
           start = end;
         }
       }
+
+      // Note types of blocks. This parallels the logic in the parent class,
+      // but we do not want to just reuse the data structure there: things may
+      // have changed since then.
+      static void scan(FinalOptimizer* self, Expression** currp) {
+        if (auto* block = (*currp)->dynCast<Block>()) {
+          if (block->type.isConcrete()) {
+            self->concreteBlockTypes[block->name] = block->type;
+          }
+        }
+
+        PostWalker<FinalOptimizer>::scan(self, currp);
+      }
+
+      std::unordered_map<Name, Type> concreteBlockTypes;
+
+      // Update the br_if's type based on the block.
+      void updateBrIfType(Break* br) {
+        assert(br->condition && br->value);
+        assert(concreteBlockTypes.count(br->name));
+        br->type = concreteBlockTypes[br->name];
+        br->finalize();
+      }
     };
     FinalOptimizer finalOptimizer(getPassOptions());
     finalOptimizer.setModule(getModule());

From ae589125377b5d2c2a9e624309bbce65389d2ad6 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 11:07:40 -0800
Subject: [PATCH 11/49] fix

---
 src/passes/RemoveUnusedBrs.cpp | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/passes/RemoveUnusedBrs.cpp b/src/passes/RemoveUnusedBrs.cpp
index 87df0c773ed..8c5323be3c0 100644
--- a/src/passes/RemoveUnusedBrs.cpp
+++ b/src/passes/RemoveUnusedBrs.cpp
@@ -1707,11 +1707,13 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
 
       std::unordered_map<Name, Type> concreteBlockTypes;
 
-      // Update the br_if's type based on the block.
+      // Update a br_if's type based on the block.
       void updateBrIfType(Break* br) {
-        assert(br->condition && br->value);
-        assert(concreteBlockTypes.count(br->name));
-        br->type = concreteBlockTypes[br->name];
+        assert(br->condition);
+        if (br->value) {
+          assert(concreteBlockTypes.count(br->name));
+          br->type = concreteBlockTypes[br->name];
+        }
         br->finalize();
       }
     };

From ab25bb65da203f804238c78745e09eb76a990f49 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 11:09:30 -0800
Subject: [PATCH 12/49] update test

---
 test/lit/passes/flatten_all-features.wast | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/test/lit/passes/flatten_all-features.wast b/test/lit/passes/flatten_all-features.wast
index aa3dc7578d8..e65e5b79f76 100644
--- a/test/lit/passes/flatten_all-features.wast
+++ b/test/lit/passes/flatten_all-features.wast
@@ -3586,13 +3586,13 @@
   ;; value type, we need the value to be set into two locals: one with the outer
   ;; block's type, and one with its value type.
   ;; CHECK:      (func $subtype (type $6) (result anyref)
-  ;; CHECK-NEXT:  (local $0 eqref)
+  ;; CHECK-NEXT:  (local $0 anyref)
   ;; CHECK-NEXT:  (local $1 anyref)
   ;; CHECK-NEXT:  (local $2 nullref)
   ;; CHECK-NEXT:  (local $3 nullref)
-  ;; CHECK-NEXT:  (local $4 eqref)
-  ;; CHECK-NEXT:  (local $5 eqref)
-  ;; CHECK-NEXT:  (local $6 eqref)
+  ;; CHECK-NEXT:  (local $4 anyref)
+  ;; CHECK-NEXT:  (local $5 anyref)
+  ;; CHECK-NEXT:  (local $6 anyref)
   ;; CHECK-NEXT:  (local $7 anyref)
   ;; CHECK-NEXT:  (block $label0
   ;; CHECK-NEXT:   (block
@@ -3633,9 +3633,9 @@
   ;; CHECK-NEXT:  )
   ;; CHECK-NEXT: )
   (func $subtype (result anyref)
-    (local $0 eqref)
+    (local $0 anyref)
     (block $label0 (result anyref)
-      (block (result eqref)
+      (block (result anyref)
         (local.tee $0
           (br_if $label0
             (ref.null eq)

From bcb3873c5c4b96578046c4f2e3e39e1a3d7ce583 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 11:26:57 -0800
Subject: [PATCH 13/49] work

---
 src/binaryen-c.cpp | 15 +++++++++++++--
 src/binaryen-c.h   | 10 ++++++++--
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/src/binaryen-c.cpp b/src/binaryen-c.cpp
index f9a778a68b6..5f8389ff464 100644
--- a/src/binaryen-c.cpp
+++ b/src/binaryen-c.cpp
@@ -1120,10 +1120,21 @@ BinaryenExpressionRef BinaryenLoop(BinaryenModuleRef module,
 BinaryenExpressionRef BinaryenBreak(BinaryenModuleRef module,
                                     const char* name,
                                     BinaryenExpressionRef condition,
-                                    BinaryenExpressionRef value) {
+                                    BinaryenExpressionRef value,
+                                    BinaryenType* type) {
+  std::optional<Type> optType;
+  if (type) {
+    optType = *type;
+  } else {
+    // As a convenience for users, if type is not provided but we can infer the
+    // type - which is the case for MVP types - then infer it.
+    if (condition && value && !value->type.isRef()) {
+      optType = value->type;
+    }
+  }
   return static_cast<Expression*>(
     Builder(*(Module*)module)
-      .makeBreak(name, (Expression*)value, (Expression*)condition));
+      .makeBreak(name, (Expression*)value, (Expression*)condition, optType));
 }
 BinaryenExpressionRef BinaryenSwitch(BinaryenModuleRef module,
                                      const char** names,
diff --git a/src/binaryen-c.h b/src/binaryen-c.h
index 4ead70ea49d..f8f8a773031 100644
--- a/src/binaryen-c.h
+++ b/src/binaryen-c.h
@@ -740,12 +740,15 @@ BINARYEN_API BinaryenExpressionRef BinaryenIf(BinaryenModuleRef module,
 BINARYEN_API BinaryenExpressionRef BinaryenLoop(BinaryenModuleRef module,
                                                 const char* in,
                                                 BinaryenExpressionRef body);
-// Break: value and condition can be NULL
+// Break: value and condition can be NULL. type can be NULL, but must be
+//        provided for a `br_if` with a value (as the type depends on the block
+//        that is targeted).
 BINARYEN_API BinaryenExpressionRef
 BinaryenBreak(BinaryenModuleRef module,
               const char* name,
               BinaryenExpressionRef condition,
-              BinaryenExpressionRef value);
+              BinaryenExpressionRef value,
+              BinaryenType* type);
 // Switch: value can be NULL
 BINARYEN_API BinaryenExpressionRef
 BinaryenSwitch(BinaryenModuleRef module,
@@ -1265,6 +1268,9 @@ BINARYEN_API void BinaryenBreakSetCondition(BinaryenExpressionRef expr,
 BINARYEN_API BinaryenExpressionRef
 BinaryenBreakGetValue(BinaryenExpressionRef expr);
 // Sets the value expression, if any, of a `br` or `br_if` expression.
+// Note that for a `br_if` with a value, if you change the type of the block
+// that this targets then you also need to change the type of the `br_if`, which
+// you can do using BinaryenExpressionSetType().
 BINARYEN_API void BinaryenBreakSetValue(BinaryenExpressionRef expr,
                                         BinaryenExpressionRef valueExpr);
 

From e4d2200b290d4cd55578d3f4aca42ddc2af44019 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 11:28:08 -0800
Subject: [PATCH 14/49] fix

---
 src/binaryen-c.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/binaryen-c.cpp b/src/binaryen-c.cpp
index 5f8389ff464..74be59c9f65 100644
--- a/src/binaryen-c.cpp
+++ b/src/binaryen-c.cpp
@@ -1124,7 +1124,7 @@ BinaryenExpressionRef BinaryenBreak(BinaryenModuleRef module,
                                     BinaryenType* type) {
   std::optional<Type> optType;
   if (type) {
-    optType = *type;
+    optType = Type(*type);
   } else {
     // As a convenience for users, if type is not provided but we can infer the
     // type - which is the case for MVP types - then infer it.

From 09d25bb4dfb76a26481d51140ad9f1c5c6e7e1c3 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 11:30:01 -0800
Subject: [PATCH 15/49] fix

---
 CHANGELOG.md                        |  8 ++++++++
 test/example/c-api-kitchen-sink.c   | 11 +++++++----
 test/example/c-api-kitchen-sink.txt |  6 ++++++
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index fc03cf492df..dd1389c8ea6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -19,6 +19,14 @@ Current Trunk
    now be placed above the instruction inside the `else` branch rather than on
    the `else` branch itself.
 
+ - Fix the result type of `br_if` to match the spec. This means that `br_if` now
+   returns a less precise type than before. In the IR it means that the type of
+   `br_if` with a value depends on the block it targets, which means it must be
+   provided when creating it, and updated if altered. The C API `BinaryenBreak`
+   now has a final parameter to allow setting the type (a `Type*`, which can be
+   NULL if the type can be inferred, which is the case for anything but a
+   `br_if` with a value, or even a `br_if` with a value for an MVP type).
+
 v117
 ----
 
diff --git a/test/example/c-api-kitchen-sink.c b/test/example/c-api-kitchen-sink.c
index bf80833a0ee..e8181097e84 100644
--- a/test/example/c-api-kitchen-sink.c
+++ b/test/example/c-api-kitchen-sink.c
@@ -974,10 +974,13 @@ void test_core() {
     BinaryenIf(module, temp4, temp5, NULL),
     BinaryenLoop(module, "in", makeInt32(module, 0)),
     BinaryenLoop(module, NULL, makeInt32(module, 0)),
-    BinaryenBreak(module, "the-value", temp6, temp7),
-    BinaryenBreak(module, "the-nothing", makeInt32(module, 2), NULL),
-    BinaryenBreak(module, "the-value", NULL, makeInt32(module, 3)),
-    BinaryenBreak(module, "the-nothing", NULL, NULL),
+    BinaryenBreak(module, "the-value", temp6, temp7, &i32),
+    // We can also provide NULL as the type of the br_if, as it is an MVP type
+    // that is inferred.
+    BinaryenBreak(module, "the-value", temp6, temp7, NULL),
+    BinaryenBreak(module, "the-nothing", makeInt32(module, 2), NULL, NULL),
+    BinaryenBreak(module, "the-value", NULL, makeInt32(module, 3), NULL),
+    BinaryenBreak(module, "the-nothing", NULL, NULL, NULL),
     BinaryenSwitch(module, switchValueNames, 1, "the-value", temp8, temp9),
     BinaryenSwitch(
       module, switchBodyNames, 1, "the-nothing", makeInt32(module, 2), NULL),
diff --git a/test/example/c-api-kitchen-sink.txt b/test/example/c-api-kitchen-sink.txt
index 469267a4dd1..2c1e802c0bb 100644
--- a/test/example/c-api-kitchen-sink.txt
+++ b/test/example/c-api-kitchen-sink.txt
@@ -2007,6 +2007,12 @@ BinaryenFeatureAll: 131071
         (i32.const 0)
        )
       )
+      (drop
+       (br_if $the-value
+        (i32.const 1)
+        (i32.const 0)
+       )
+      )
       (br_if $the-nothing
        (i32.const 2)
       )

From e08ace48810024635f3fc8999b377631d3afb39e Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 11:31:06 -0800
Subject: [PATCH 16/49] format

---
 src/wasm/wasm-validator.cpp | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp
index 29b10141647..2b8f8eb6d90 100644
--- a/src/wasm/wasm-validator.cpp
+++ b/src/wasm/wasm-validator.cpp
@@ -623,10 +623,8 @@ void FunctionValidator::validateBrIfs(Name label, Type type) {
   if (auto iter = labelBrIfs.find(name); iter != labelBrIfs.end()) {
     for (auto* brIf : iter->second) {
       assert(brIf->value && brIf->condition); // This must be an actual br_if.
-      shouldBeEqual(type,
-                    brIf->type,
-                    brIf,
-                    "br_ifs to a block must have the block's type");
+      shouldBeEqual(
+        type, brIf->type, brIf, "br_ifs to a block must have the block's type");
     }
   }
 }
@@ -803,7 +801,8 @@ void FunctionValidator::visitLoop(Loop* curr) {
     }
     breakTypes.erase(iter);
 
-    //validateBrIfs(curr->name, curr->type); // XXX breaks must have type none, so no br_ifs!
+    // validateBrIfs(curr->name, curr->type); // XXX breaks must have type none,
+    // so no br_ifs!
   }
   if (curr->type == Type::none) {
     shouldBeFalse(curr->body->type.isConcrete(),

From b424067004722054d2ae4e85e5861096575e92a2 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 11:34:03 -0800
Subject: [PATCH 17/49] work

---
 src/wasm/wasm-validator.cpp | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp
index 2b8f8eb6d90..bbb79082daf 100644
--- a/src/wasm/wasm-validator.cpp
+++ b/src/wasm/wasm-validator.cpp
@@ -250,10 +250,6 @@ struct FunctionValidator : public WalkerPass<PostWalker<FunctionValidator>> {
   // Maps a label name (block etc.) to the br_ifs targetting it.
   std::unordered_map<Name, std::vector<Break*>> labelBrIfs;
 
-  // Given a label and the type of it, like a block's name and its type,
-  // validate all br_ifs we've seen that target it.
-  void validateBrIfs(Name label, Type type);
-
 public:
   // visitors
 
@@ -619,16 +615,6 @@ void FunctionValidator::noteLabelName(Name name) {
     "names in Binaryen IR must be unique - IR generators must ensure that");
 }
 
-void FunctionValidator::validateBrIfs(Name label, Type type) {
-  if (auto iter = labelBrIfs.find(name); iter != labelBrIfs.end()) {
-    for (auto* brIf : iter->second) {
-      assert(brIf->value && brIf->condition); // This must be an actual br_if.
-      shouldBeEqual(
-        type, brIf->type, brIf, "br_ifs to a block must have the block's type");
-    }
-  }
-}
-
 void FunctionValidator::validatePoppyExpression(Expression* curr) {
   if (curr->type == Type::unreachable) {
     shouldBeTrue(StackUtils::mayBeUnreachable(curr),
@@ -687,7 +673,15 @@ void FunctionValidator::visitBlock(Block* curr) {
     }
     breakTypes.erase(iter);
 
-    validateBrIfs(curr->name, curr->type);
+    // Validate br_if types as matching this block's type.
+    if (auto iter = labelBrIfs.find(curr->name); iter != labelBrIfs.end()) {
+      for (auto* brIf : iter->second) {
+        // This must be an actual br_if with a value.
+        assert(brIf->value && brIf->condition);
+        shouldBeEqual(
+          curr->type, brIf->type, brIf, "br_ifs to a block must have the block's type");
+      }
+    }
   }
   switch (getFunction()->profile) {
     case IRProfile::Normal:
@@ -801,8 +795,10 @@ void FunctionValidator::visitLoop(Loop* curr) {
     }
     breakTypes.erase(iter);
 
-    // validateBrIfs(curr->name, curr->type); // XXX breaks must have type none,
-    // so no br_ifs!
+    // Validate there are no br_ifs with values targeting us.
+    shouldBeTrue(!labelBrIfs.count(curr->name),
+                 curr,
+                 "loops cannot have br_ifs ith values targeting them");
   }
   if (curr->type == Type::none) {
     shouldBeFalse(curr->body->type.isConcrete(),

From e481b44ba3a2ffcd2871a33a1fee8165af165eac Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 11:34:12 -0800
Subject: [PATCH 18/49] format

---
 src/wasm/wasm-validator.cpp | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp
index bbb79082daf..28e5d28ae1a 100644
--- a/src/wasm/wasm-validator.cpp
+++ b/src/wasm/wasm-validator.cpp
@@ -678,8 +678,10 @@ void FunctionValidator::visitBlock(Block* curr) {
       for (auto* brIf : iter->second) {
         // This must be an actual br_if with a value.
         assert(brIf->value && brIf->condition);
-        shouldBeEqual(
-          curr->type, brIf->type, brIf, "br_ifs to a block must have the block's type");
+        shouldBeEqual(curr->type,
+                      brIf->type,
+                      brIf,
+                      "br_ifs to a block must have the block's type");
       }
     }
   }

From ff5391abcf086bfd33f4fe555dcfa207c11c36f2 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 12:39:28 -0800
Subject: [PATCH 19/49] fork

---
 src/wasm-builder.h          | 3 +++
 src/wasm/wasm-validator.cpp | 9 +++++----
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/wasm-builder.h b/src/wasm-builder.h
index 8bede5b1451..ffcba650a4d 100644
--- a/src/wasm-builder.h
+++ b/src/wasm-builder.h
@@ -248,6 +248,9 @@ class Builder {
     if (value && condition) {
       // This is a br_if, for whom we must receive the type, as the type must
       // match the block it targets (which we do not know here).
+      //
+      // Note that we could infer the type in some cases, like for MVP types,
+      // but we intentionally do not so as to catch bugs earlier.
       assert(type);
       ret->type = *type;
     }
diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp
index 28e5d28ae1a..e260d7c5d27 100644
--- a/src/wasm/wasm-validator.cpp
+++ b/src/wasm/wasm-validator.cpp
@@ -678,10 +678,11 @@ void FunctionValidator::visitBlock(Block* curr) {
       for (auto* brIf : iter->second) {
         // This must be an actual br_if with a value.
         assert(brIf->value && brIf->condition);
-        shouldBeEqual(curr->type,
-                      brIf->type,
-                      brIf,
-                      "br_ifs to a block must have the block's type");
+        shouldBeEqualOrFirstIsUnreachable(
+          brIf->type,
+          curr->type,
+          brIf,
+          "br_ifs to a block must have the block's type");
       }
     }
   }

From 1ac48b9590d1b11ee0b339bd5ba15d84362097f7 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 14:20:37 -0800
Subject: [PATCH 20/49] work

---
 src/ir/ReFinalize.cpp              | 36 ++++++++++++++++++++++++++++++
 src/ir/utils.h                     |  9 +++++++-
 test/lit/passes/precompute-gc.wast | 17 ++++++++++++++
 3 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/src/ir/ReFinalize.cpp b/src/ir/ReFinalize.cpp
index 6d872056adb..5f5d4dfb849 100644
--- a/src/ir/ReFinalize.cpp
+++ b/src/ir/ReFinalize.cpp
@@ -24,6 +24,42 @@ static Type getValueType(Expression* value) {
   return value ? value->type : Type::none;
 }
 
+void ReFinalize::doWalkFunction(Function* func) {
+  using Super =
+    WalkerPass<PostWalker<ReFinalize, OverriddenVisitor<ReFinalize>>>;
+
+  while (1) {
+    // Walk normally.
+    Super::doWalkFunction(func);
+
+    // After that walk we may have br_ifs in need of refinalization. Update them
+    // and refinalize again, as they may enable further improvements. This is in
+    // theory very slow, but in practice one or two cycles suffices and we can't
+    // try to be frugal here as must propagate all the possible improvements (or
+    // else we'd end up with a situation where ReFinalize^2 != ReFinalize).
+    auto updatedBr = false;
+
+    for (auto& [block, brs] : blockBrs) {
+      assert(block->type.isConcrete());
+      auto blockType = block->type;
+      for (auto* br : brs) {
+        if (br->type != Type::unreachable && br->type != blockType) {
+          br->type = blockType;
+          updateBr = true;
+        }
+      }
+    }
+
+    if (!updatedBr) {
+      return;
+    }
+
+    // Clear all state and loop/walk again.
+    breakTypes.clear();
+    blockBrs.clear();
+  }
+}
+
 void ReFinalize::visitBlock(Block* curr) {
   if (curr->list.size() == 0) {
     curr->type = Type::none;
diff --git a/src/ir/utils.h b/src/ir/utils.h
index 72aa701bb77..683efcec82d 100644
--- a/src/ir/utils.h
+++ b/src/ir/utils.h
@@ -129,10 +129,17 @@ struct ReFinalize
 
   ReFinalize() { name = "refinalize"; }
 
-  // block finalization is O(bad) if we do each block by itself, so do it in
+  // Block finalization is O(bad) if we do each block by itself, so do it in
   // bulk, tracking break value types so we just do a linear pass.
   std::unordered_map<Name, std::unordered_set<Type>> breakTypes;
 
+  // Track br_ifs with values as we go, as those must be updated if their block
+  // type changes.
+  // TODO: BrOn too
+  std::unordered_map<Block*, std::vector<Break*>> blockBrs;
+
+  void doWalkFunction(Function* func);
+
 #define DELEGATE(CLASS_TO_VISIT)                                               \
   void visit##CLASS_TO_VISIT(CLASS_TO_VISIT* curr);
 
diff --git a/test/lit/passes/precompute-gc.wast b/test/lit/passes/precompute-gc.wast
index 2b16cb43f95..85770aaa83c 100644
--- a/test/lit/passes/precompute-gc.wast
+++ b/test/lit/passes/precompute-gc.wast
@@ -1196,4 +1196,21 @@
   )
   (local.get $x)
  )
+
+ (func $refinalize-refine-br_if (param $nn-any (ref any)) (result anyref)
+  ;; This tests refinalize (using precompute, because it always refinalizes).
+  ;; When we refine the block to non-nullable we must also update the br_if to
+  ;; the same type, as they must match.
+  (block $block (result anyref)
+   (drop
+    (block (result anyref) ;; This block will also get refined, after the br_if.
+     (br_if $block
+      (local.get $nn-any)
+      (i32.const 1)
+     )
+    )
+   )
+   (local.get $nn-any)
+  )
+ )
 )

From 86f487a2c6ceea8eafadbde706fd75257c5a0f0e Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 14:42:30 -0800
Subject: [PATCH 21/49] horribl

---
 src/ir/ReFinalize.cpp | 33 ++++++++++++++++++++++++++-------
 src/ir/utils.h        | 12 ++++++++++--
 2 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/src/ir/ReFinalize.cpp b/src/ir/ReFinalize.cpp
index 5f5d4dfb849..b9b28ed2f6b 100644
--- a/src/ir/ReFinalize.cpp
+++ b/src/ir/ReFinalize.cpp
@@ -35,17 +35,26 @@ void ReFinalize::doWalkFunction(Function* func) {
     // After that walk we may have br_ifs in need of refinalization. Update them
     // and refinalize again, as they may enable further improvements. This is in
     // theory very slow, but in practice one or two cycles suffices and we can't
-    // try to be frugal here as must propagate all the possible improvements (or
-    // else we'd end up with a situation where ReFinalize^2 != ReFinalize).
+    // try to be frugal here as must propagate all the possible improvements, or
+    // else we'd end up with a situation where ReFinalize^2 != ReFinalize. (If
+    // this ends up being slow in practice we'd want to build a custom graph IR
+    // for this operation here.)
+    //
+    // Note that this loop must surely terminate because the lattice of types is
+    // finite.
     auto updatedBr = false;
 
-    for (auto& [block, brs] : blockBrs) {
+    for (auto& [_, info] : blockBrInfoMap) {
+      auto* block = info.block;
       assert(block->type.isConcrete());
       auto blockType = block->type;
-      for (auto* br : brs) {
-        if (br->type != Type::unreachable && br->type != blockType) {
+      for (auto* br : info.brs) {
+        // We would have ignored an unreachable or MVP br_if before; only
+        // references must be refined.
+        assert(br->type.isRef());
+        if (br->type != blockType) {
           br->type = blockType;
-          updateBr = true;
+          updatedBr = true;
         }
       }
     }
@@ -56,7 +65,7 @@ void ReFinalize::doWalkFunction(Function* func) {
 
     // Clear all state and loop/walk again.
     breakTypes.clear();
-    blockBrs.clear();
+    blockBrInfoMap.clear();
   }
 }
 
@@ -74,6 +83,12 @@ void ReFinalize::visitBlock(Block* curr) {
       auto& types = iter->second;
       types.insert(curr->list.back()->type);
       curr->type = Type::getLeastUpperBound(types);
+
+      // If we have br_ifs then we must note ourselves there.
+      auto iter = blockBrInfoMap.find(curr->name);
+      if (iter != blockBrInfoMap.end()) {
+        iter->second.block = curr;
+      }
       return;
     }
   }
@@ -101,6 +116,10 @@ void ReFinalize::visitBreak(Break* curr) {
   } else {
     updateBreakValueType(curr->name, valueType);
   }
+  // Note relevant br_ifs for type updating later.
+  if (curr->condition && curr->value && curr->type.isRef()) {
+    blockBrInfoMap[curr->name].brs.push_back(curr);
+  }
 }
 void ReFinalize::visitSwitch(Switch* curr) {
   curr->finalize();
diff --git a/src/ir/utils.h b/src/ir/utils.h
index 683efcec82d..be83fe269da 100644
--- a/src/ir/utils.h
+++ b/src/ir/utils.h
@@ -134,9 +134,17 @@ struct ReFinalize
   std::unordered_map<Name, std::unordered_set<Type>> breakTypes;
 
   // Track br_ifs with values as we go, as those must be updated if their block
-  // type changes.
+  // type changes. We must also track block names to blocks for this, so we
+  // track all that using the block name as the key.
   // TODO: BrOn too
-  std::unordered_map<Block*, std::vector<Break*>> blockBrs;
+  struct BlockBrInfo {
+    Block* block = nullptr;
+    std::vector<Break*> brs;
+
+    BlockBrInfo() {}
+    BlockBrInfo(Block* block) : block(block) {}
+  };
+  std::unordered_map<Name, BlockBrInfo> blockBrInfoMap;
 
   void doWalkFunction(Function* func);
 

From f8a177a0b723a493bbd67c8a237b8095928e893a Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 14:46:17 -0800
Subject: [PATCH 22/49] fix

---
 test/lit/passes/precompute-gc.wast | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/test/lit/passes/precompute-gc.wast b/test/lit/passes/precompute-gc.wast
index 85770aaa83c..23426ec88f9 100644
--- a/test/lit/passes/precompute-gc.wast
+++ b/test/lit/passes/precompute-gc.wast
@@ -1197,13 +1197,29 @@
   (local.get $x)
  )
 
+ ;; CHECK:      (func $refinalize-refine-br_if (type $20) (param $nn-any (ref any)) (result anyref)
+ ;; CHECK-NEXT:  (nop)
+ ;; CHECK-NEXT:  (block $block (result (ref any))
+ ;; CHECK-NEXT:   (drop
+ ;; CHECK-NEXT:    (block (result (ref any))
+ ;; CHECK-NEXT:     (br_if $block
+ ;; CHECK-NEXT:      (local.get $nn-any)
+ ;; CHECK-NEXT:      (i32.const 1)
+ ;; CHECK-NEXT:     )
+ ;; CHECK-NEXT:    )
+ ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:   (local.get $nn-any)
+ ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT: )
  (func $refinalize-refine-br_if (param $nn-any (ref any)) (result anyref)
+  ;; A nop prevents the next block from being elided in printing.
+  (nop)
   ;; This tests refinalize (using precompute, because it always refinalizes).
   ;; When we refine the block to non-nullable we must also update the br_if to
   ;; the same type, as they must match.
   (block $block (result anyref)
    (drop
-    (block (result anyref) ;; This block will also get refined, after the br_if.
+    (block $ignore (result anyref) ;; This block will also get refined, after the br_if.
      (br_if $block
       (local.get $nn-any)
       (i32.const 1)

From 49f8ddec9fe73174526df3adbe559aa5b2d49c7a Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 14:53:19 -0800
Subject: [PATCH 23/49] typo

---
 src/ir/utils.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/ir/utils.h b/src/ir/utils.h
index be83fe269da..b8fdb9f1bf2 100644
--- a/src/ir/utils.h
+++ b/src/ir/utils.h
@@ -136,7 +136,6 @@ struct ReFinalize
   // Track br_ifs with values as we go, as those must be updated if their block
   // type changes. We must also track block names to blocks for this, so we
   // track all that using the block name as the key.
-  // TODO: BrOn too
   struct BlockBrInfo {
     Block* block = nullptr;
     std::vector<Break*> brs;

From beee1fb9b1f5f0da0579417f4d71e5bcf3f64409 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 14:55:36 -0800
Subject: [PATCH 24/49] changelog

---
 CHANGELOG.md | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index dd1389c8ea6..82151e98fa9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -22,10 +22,11 @@ Current Trunk
  - Fix the result type of `br_if` to match the spec. This means that `br_if` now
    returns a less precise type than before. In the IR it means that the type of
    `br_if` with a value depends on the block it targets, which means it must be
-   provided when creating it, and updated if altered. The C API `BinaryenBreak`
-   now has a final parameter to allow setting the type (a `Type*`, which can be
-   NULL if the type can be inferred, which is the case for anything but a
-   `br_if` with a value, or even a `br_if` with a value for an MVP type).
+   provided when creating it, and updated if altered.
+   - As a result the C API `BinaryenBreak` now has a final parameter to allow
+     setting the type, which is a `Type*`. If NULL then the type will be
+     inferred in common cases (MVP types, or anything but a `br_if` with a
+     value).
 
 v117
 ----

From 836ed0f143a6f3315b18c825bd7f2090e3357255 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 14:58:14 -0800
Subject: [PATCH 25/49] work

---
 src/binaryen-c.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/binaryen-c.h b/src/binaryen-c.h
index f8f8a773031..8b3a6710be7 100644
--- a/src/binaryen-c.h
+++ b/src/binaryen-c.h
@@ -742,7 +742,8 @@ BINARYEN_API BinaryenExpressionRef BinaryenLoop(BinaryenModuleRef module,
                                                 BinaryenExpressionRef body);
 // Break: value and condition can be NULL. type can be NULL, but must be
 //        provided for a `br_if` with a value (as the type depends on the block
-//        that is targeted).
+//        that is targeted). The type can also be NULL if the value type is an
+//        MVP type.
 BINARYEN_API BinaryenExpressionRef
 BinaryenBreak(BinaryenModuleRef module,
               const char* name,

From ca0ee1991985cb387a308a69ed4ddab4144b8a7d Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 15:47:57 -0800
Subject: [PATCH 26/49] work

---
 src/ir/ReFinalize.cpp              |  4 ++--
 src/passes/RemoveUnusedBrs.cpp     |  8 ++++----
 src/wasm-type.h                    |  2 ++
 src/wasm/wasm-type.cpp             | 12 ++++++++++++
 test/lit/passes/precompute-gc.wast | 22 ++++++++++++++++++++++
 5 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/src/ir/ReFinalize.cpp b/src/ir/ReFinalize.cpp
index b9b28ed2f6b..9c21f2d7e36 100644
--- a/src/ir/ReFinalize.cpp
+++ b/src/ir/ReFinalize.cpp
@@ -51,7 +51,7 @@ void ReFinalize::doWalkFunction(Function* func) {
       for (auto* br : info.brs) {
         // We would have ignored an unreachable or MVP br_if before; only
         // references must be refined.
-        assert(br->type.isRef());
+        assert(br->type.containsRef());
         if (br->type != blockType) {
           br->type = blockType;
           updatedBr = true;
@@ -117,7 +117,7 @@ void ReFinalize::visitBreak(Break* curr) {
     updateBreakValueType(curr->name, valueType);
   }
   // Note relevant br_ifs for type updating later.
-  if (curr->condition && curr->value && curr->type.isRef()) {
+  if (curr->condition && curr->value && curr->type.containsRef()) {
     blockBrInfoMap[curr->name].brs.push_back(curr);
   }
 }
diff --git a/src/passes/RemoveUnusedBrs.cpp b/src/passes/RemoveUnusedBrs.cpp
index 8c5323be3c0..0e639cb14c8 100644
--- a/src/passes/RemoveUnusedBrs.cpp
+++ b/src/passes/RemoveUnusedBrs.cpp
@@ -417,7 +417,7 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
             br->condition =
               builder.makeSelect(br->condition, curr->condition, zero);
           }
-          if (br->value) {
+          if (br->value && br->value->type.containsRef()) {
             // Update the br_if's type based on the block.
             assert(concreteBlockTypes.count(br->name));
             br->type = concreteBlockTypes[br->name];
@@ -487,7 +487,7 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
     }
 
     if (auto* block = curr->dynCast<Block>()) {
-      if (block->type.isConcrete()) {
+      if (block->type.containsRef()) {
         self->concreteBlockTypes[block->name] = block->type;
       }
     }
@@ -1697,7 +1697,7 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
       // have changed since then.
       static void scan(FinalOptimizer* self, Expression** currp) {
         if (auto* block = (*currp)->dynCast<Block>()) {
-          if (block->type.isConcrete()) {
+          if (block->type.containsRef()) {
             self->concreteBlockTypes[block->name] = block->type;
           }
         }
@@ -1710,7 +1710,7 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
       // Update a br_if's type based on the block.
       void updateBrIfType(Break* br) {
         assert(br->condition);
-        if (br->value) {
+        if (br->value && br->value->type.containsRef()) {
           assert(concreteBlockTypes.count(br->name));
           br->type = concreteBlockTypes[br->name];
         }
diff --git a/src/wasm-type.h b/src/wasm-type.h
index 5c47f405191..176a48da7aa 100644
--- a/src/wasm-type.h
+++ b/src/wasm-type.h
@@ -173,6 +173,8 @@ class Type {
   bool isException() const;
   bool isString() const;
   bool isDefaultable() const;
+  // The same as isRef() but also returns true for a tuple containing a ref.
+  bool containsRef() const;
 
   Nullability getNullability() const;
 
diff --git a/src/wasm/wasm-type.cpp b/src/wasm/wasm-type.cpp
index b14e5aa39d2..69693672cdc 100644
--- a/src/wasm/wasm-type.cpp
+++ b/src/wasm/wasm-type.cpp
@@ -824,6 +824,18 @@ bool Type::isDefaultable() const {
   return isConcrete() && !isNonNullable();
 }
 
+bool Type::containsRef() const {
+  if (isTuple()) {
+    for (auto t : *this) {
+      if (t.isRef()) {
+        return true;
+      }
+    }
+    return false;
+  }
+  return isRef();
+}
+
 Nullability Type::getNullability() const {
   return isNullable() ? Nullable : NonNullable;
 }
diff --git a/test/lit/passes/precompute-gc.wast b/test/lit/passes/precompute-gc.wast
index 23426ec88f9..1a40a1d32f6 100644
--- a/test/lit/passes/precompute-gc.wast
+++ b/test/lit/passes/precompute-gc.wast
@@ -1229,4 +1229,26 @@
    (local.get $nn-any)
   )
  )
+
+ (func $refinalize-refine-br_if-tuple (param $nn-any (ref any)) (result anyref i32)
+  ;; As above, but with a tuple containing a reference.
+  (nop)
+  (block $block (result anyref i32)
+   (tuple.drop 2
+    (block $ignore (result anyref i32)
+     (br_if $block
+      (tuple.make 2
+       (local.get $nn-any)
+       (i32.const 0)
+      )
+      (i32.const 1)
+     )
+    )
+   )
+   (tuple.make 2
+    (local.get $nn-any)
+    (i32.const 2)
+   )
+  )
+ )
 )

From 7bb4aa5e5343c0c240713c269bbffa31bf93b3cc Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 15:52:39 -0800
Subject: [PATCH 27/49] fix

---
 test/lit/passes/precompute-gc.wast | 42 ++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/test/lit/passes/precompute-gc.wast b/test/lit/passes/precompute-gc.wast
index 1a40a1d32f6..57e24c3f7bb 100644
--- a/test/lit/passes/precompute-gc.wast
+++ b/test/lit/passes/precompute-gc.wast
@@ -22,7 +22,7 @@
 
  (type $func-return-i32 (func (result i32)))
 
- ;; CHECK:      (import "fuzzing-support" "log-i32" (func $log (type $4) (param i32)))
+ ;; CHECK:      (import "fuzzing-support" "log-i32" (func $log (type $5) (param i32)))
  (import "fuzzing-support" "log-i32" (func $log (param i32)))
 
  ;; CHECK:      (func $test-fallthrough (type $func-return-i32) (result i32)
@@ -119,7 +119,7 @@
    (struct.get $struct 0 (local.get $x))
   )
  )
- ;; CHECK:      (func $load-from-struct-bad-merge (type $4) (param $i i32)
+ ;; CHECK:      (func $load-from-struct-bad-merge (type $5) (param $i i32)
  ;; CHECK-NEXT:  (local $x (ref null $struct))
  ;; CHECK-NEXT:  (if
  ;; CHECK-NEXT:   (local.get $i)
@@ -168,7 +168,7 @@
    (struct.get $struct 0 (local.get $x))
   )
  )
- ;; CHECK:      (func $modify-gc-heap (type $5) (param $x (ref null $struct))
+ ;; CHECK:      (func $modify-gc-heap (type $6) (param $x (ref null $struct))
  ;; CHECK-NEXT:  (struct.set $struct 0
  ;; CHECK-NEXT:   (local.get $x)
  ;; CHECK-NEXT:   (i32.add
@@ -222,7 +222,7 @@
    (struct.get $struct 0 (local.get $x))
   )
  )
- ;; CHECK:      (func $load-from-struct-bad-arrive (type $5) (param $x (ref null $struct))
+ ;; CHECK:      (func $load-from-struct-bad-arrive (type $6) (param $x (ref null $struct))
  ;; CHECK-NEXT:  (call $log
  ;; CHECK-NEXT:   (struct.get $struct 0
  ;; CHECK-NEXT:    (local.get $x)
@@ -388,7 +388,7 @@
   (local.get $tempresult)
  )
 
- ;; CHECK:      (func $propagate-uncertain-param (type $6) (param $input (ref $empty)) (result i32)
+ ;; CHECK:      (func $propagate-uncertain-param (type $7) (param $input (ref $empty)) (result i32)
  ;; CHECK-NEXT:  (local $tempresult i32)
  ;; CHECK-NEXT:  (local $tempref (ref null $empty))
  ;; CHECK-NEXT:  (local.set $tempresult
@@ -435,7 +435,7 @@
   (local.get $tempresult)
  )
 
- ;; CHECK:      (func $propagate-same-param (type $6) (param $input (ref $empty)) (result i32)
+ ;; CHECK:      (func $propagate-same-param (type $7) (param $input (ref $empty)) (result i32)
  ;; CHECK-NEXT:  (local $tempresult i32)
  ;; CHECK-NEXT:  (local.set $tempresult
  ;; CHECK-NEXT:   (ref.eq
@@ -850,7 +850,7 @@
   )
  )
 
- ;; CHECK:      (func $new_block_unreachable (type $8) (result anyref)
+ ;; CHECK:      (func $new_block_unreachable (type $9) (result anyref)
  ;; CHECK-NEXT:  (block ;; (replaces unreachable StructNew we can't emit)
  ;; CHECK-NEXT:   (drop
  ;; CHECK-NEXT:    (block
@@ -892,7 +892,7 @@
   )
  )
 
- ;; CHECK:      (func $ref.is_null (type $4) (param $param i32)
+ ;; CHECK:      (func $ref.is_null (type $5) (param $param i32)
  ;; CHECK-NEXT:  (local $ref (ref null $empty))
  ;; CHECK-NEXT:  (local.set $ref
  ;; CHECK-NEXT:   (struct.new_default $empty)
@@ -1052,7 +1052,7 @@
   )
  )
 
- ;; CHECK:      (func $get-nonnullable-in-unreachable (type $8) (result anyref)
+ ;; CHECK:      (func $get-nonnullable-in-unreachable (type $9) (result anyref)
  ;; CHECK-NEXT:  (local $x (ref any))
  ;; CHECK-NEXT:  (local.tee $x
  ;; CHECK-NEXT:   (unreachable)
@@ -1089,7 +1089,7 @@
   (local.get $x)
  )
 
- ;; CHECK:      (func $get-nonnullable-in-unreachable-entry (type $9) (param $x i32) (param $y (ref any))
+ ;; CHECK:      (func $get-nonnullable-in-unreachable-entry (type $10) (param $x i32) (param $y (ref any))
  ;; CHECK-NEXT:  (local $0 (ref any))
  ;; CHECK-NEXT:  (unreachable)
  ;; CHECK-NEXT:  (local.set $0
@@ -1123,7 +1123,7 @@
   )
  )
 
- ;; CHECK:      (func $get-nonnullable-in-unreachable-later-loop (type $9) (param $x i32) (param $y (ref any))
+ ;; CHECK:      (func $get-nonnullable-in-unreachable-later-loop (type $10) (param $x i32) (param $y (ref any))
  ;; CHECK-NEXT:  (local $0 (ref any))
  ;; CHECK-NEXT:  (if
  ;; CHECK-NEXT:   (local.get $x)
@@ -1230,6 +1230,26 @@
   )
  )
 
+ ;; CHECK:      (func $refinalize-refine-br_if-tuple (type $21) (param $nn-any (ref any)) (result anyref i32)
+ ;; CHECK-NEXT:  (nop)
+ ;; CHECK-NEXT:  (block $block (type $4) (result (ref any) i32)
+ ;; CHECK-NEXT:   (tuple.drop 2
+ ;; CHECK-NEXT:    (block (type $4) (result (ref any) i32)
+ ;; CHECK-NEXT:     (br_if $block
+ ;; CHECK-NEXT:      (tuple.make 2
+ ;; CHECK-NEXT:       (local.get $nn-any)
+ ;; CHECK-NEXT:       (i32.const 0)
+ ;; CHECK-NEXT:      )
+ ;; CHECK-NEXT:      (i32.const 1)
+ ;; CHECK-NEXT:     )
+ ;; CHECK-NEXT:    )
+ ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:   (tuple.make 2
+ ;; CHECK-NEXT:    (local.get $nn-any)
+ ;; CHECK-NEXT:    (i32.const 2)
+ ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT: )
  (func $refinalize-refine-br_if-tuple (param $nn-any (ref any)) (result anyref i32)
   ;; As above, but with a tuple containing a reference.
   (nop)

From caf070d8cae7b627b714323896408e559c861584 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 16:18:47 -0800
Subject: [PATCH 28/49] work

---
 src/wasm/wasm-validator.cpp | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp
index e260d7c5d27..a29f7e48742 100644
--- a/src/wasm/wasm-validator.cpp
+++ b/src/wasm/wasm-validator.cpp
@@ -914,6 +914,14 @@ void FunctionValidator::visitBreak(Break* curr) {
   if (curr->condition && curr->value) {
     labelBrIfs[curr->name].push_back(curr);
   }
+  // An unreachable br must have no condition, or it must have an unreachable
+  // child.
+  if (curr->type == Type::unreachable && curr->condition) {
+    shouldBeTrue(curr->condition->type == Type::unreachable ||
+                 (curr->value && curr->value->type == Type::unreachable),
+                 curr,
+                 "unreachable break with condition or no unreachable child");
+  }
 }
 
 void FunctionValidator::visitSwitch(Switch* curr) {

From 528ad9588491f9508346a58ad2fd9de17e95fae5 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 16:29:30 -0800
Subject: [PATCH 29/49] work

---
 src/passes/RemoveUnusedBrs.cpp | 50 ++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 23 deletions(-)

diff --git a/src/passes/RemoveUnusedBrs.cpp b/src/passes/RemoveUnusedBrs.cpp
index 0e639cb14c8..402f1309907 100644
--- a/src/passes/RemoveUnusedBrs.cpp
+++ b/src/passes/RemoveUnusedBrs.cpp
@@ -140,6 +140,26 @@ static bool tooCostlyToRunUnconditionally(const PassOptions& passOptions,
   return tooCostlyToRunUnconditionally(passOptions, max);
 }
 
+// Utility to update a br_if type based on its value and the type of the block
+// it branches to. Receives a map of block name to block types. The map does not
+// need to contain blocks with MVP types, as we can infer those from the value.
+// This also finalizes at the end.
+using BlockTypeMap = std::unordered_map<Name, Type>;
+void updateBrIfType(Break* br, const BlockTypeMap& blockTypeMap) {
+  assert(br->condition);
+  assert(br->condition);
+  if (br->value) {
+    if (br->value->type.containsRef()) {
+      assert(blockTypeMap.count(br->name));
+      br->type = blockTypeMap[br->name];
+    } else {
+      // A simple type we can infer from the value.
+      br->type = br->value->type;
+    }
+  }
+  br->finalize();
+}
+
 struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
   bool isFunctionParallel() override { return true; }
 
@@ -417,12 +437,7 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
             br->condition =
               builder.makeSelect(br->condition, curr->condition, zero);
           }
-          if (br->value && br->value->type.containsRef()) {
-            // Update the br_if's type based on the block.
-            assert(concreteBlockTypes.count(br->name));
-            br->type = concreteBlockTypes[br->name];
-          }
-          br->finalize();
+          updateBrIfType(br, blockTypes);
           replaceCurrent(Builder(*getModule()).dropIfConcretelyTyped(br));
           anotherCycle = true;
         }
@@ -488,15 +503,14 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
 
     if (auto* block = curr->dynCast<Block>()) {
       if (block->type.containsRef()) {
-        self->concreteBlockTypes[block->name] = block->type;
+        self->blockTypes[block->name] = block->type;
       }
     }
     super::scan(self, currp);
   }
 
-  // We track the types of blocks that have concrete types, as they may have
-  // br_ifs that target them, whose type depends on the block type.
-  std::unordered_map<Name, Type> concreteBlockTypes;
+  // We track the types of blocks that have relevant types for updateBrIfType.
+  std::unordered_map<Name, Type> blockTypes;
 
   // optimizes a loop. returns true if we made changes
   bool optimizeLoop(Loop* loop) {
@@ -1081,7 +1095,7 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
             // we are an if-else where the ifTrue is a break without a
             // condition, so we can do this
             ifTrueBreak->condition = iff->condition;
-            updateBrIfType(ifTrueBreak);
+            updateBrIfType(ifTrueBreak, blockTypes);
             list[i] = Builder(*getModule()).dropIfConcretelyTyped(ifTrueBreak);
             ExpressionManipulator::spliceIntoBlock(curr, i + 1, iff->ifFalse);
             continue;
@@ -1095,7 +1109,7 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
                                 *getModule())) {
             ifFalseBreak->condition =
               Builder(*getModule()).makeUnary(EqZInt32, iff->condition);
-            updateBrIfType(ifFalseBreak);
+            updateBrIfType(ifFalseBreak, blockTypes);
             list[i] = Builder(*getModule()).dropIfConcretelyTyped(ifFalseBreak);
             ExpressionManipulator::spliceIntoBlock(curr, i + 1, iff->ifTrue);
             continue;
@@ -1698,7 +1712,7 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
       static void scan(FinalOptimizer* self, Expression** currp) {
         if (auto* block = (*currp)->dynCast<Block>()) {
           if (block->type.containsRef()) {
-            self->concreteBlockTypes[block->name] = block->type;
+            self->blockTypes[block->name] = block->type;
           }
         }
 
@@ -1706,16 +1720,6 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
       }
 
       std::unordered_map<Name, Type> concreteBlockTypes;
-
-      // Update a br_if's type based on the block.
-      void updateBrIfType(Break* br) {
-        assert(br->condition);
-        if (br->value && br->value->type.containsRef()) {
-          assert(concreteBlockTypes.count(br->name));
-          br->type = concreteBlockTypes[br->name];
-        }
-        br->finalize();
-      }
     };
     FinalOptimizer finalOptimizer(getPassOptions());
     finalOptimizer.setModule(getModule());

From b7db67ed81aaad64717e84dbf0b1d887e6758a03 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 16:30:56 -0800
Subject: [PATCH 30/49] work

---
 src/passes/RemoveUnusedBrs.cpp | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/passes/RemoveUnusedBrs.cpp b/src/passes/RemoveUnusedBrs.cpp
index 402f1309907..521c721d1b9 100644
--- a/src/passes/RemoveUnusedBrs.cpp
+++ b/src/passes/RemoveUnusedBrs.cpp
@@ -145,13 +145,14 @@ static bool tooCostlyToRunUnconditionally(const PassOptions& passOptions,
 // need to contain blocks with MVP types, as we can infer those from the value.
 // This also finalizes at the end.
 using BlockTypeMap = std::unordered_map<Name, Type>;
-void updateBrIfType(Break* br, const BlockTypeMap& blockTypeMap) {
+static void updateBrIfType(Break* br, const BlockTypeMap& blockTypeMap) {
   assert(br->condition);
   assert(br->condition);
   if (br->value) {
     if (br->value->type.containsRef()) {
-      assert(blockTypeMap.count(br->name));
-      br->type = blockTypeMap[br->name];
+      auto iter = blockTypeMap.find(br->name);
+      assert(iter != blockTypeMap.end());
+      br->type = iter->second;
     } else {
       // A simple type we can infer from the value.
       br->type = br->value->type;
@@ -1719,7 +1720,7 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
         PostWalker<FinalOptimizer>::scan(self, currp);
       }
 
-      std::unordered_map<Name, Type> concreteBlockTypes;
+      std::unordered_map<Name, Type> blockTypes;
     };
     FinalOptimizer finalOptimizer(getPassOptions());
     finalOptimizer.setModule(getModule());

From 3818676ef8fdf3c9f2cea1264becf602cb19bb06 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 17:02:06 -0800
Subject: [PATCH 31/49] work

---
 src/passes/Heap2Local.cpp       | 10 ++++++++--
 test/lit/passes/heap2local.wast | 16 ++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/src/passes/Heap2Local.cpp b/src/passes/Heap2Local.cpp
index fcf0d88863b..d58bfdfe01b 100644
--- a/src/passes/Heap2Local.cpp
+++ b/src/passes/Heap2Local.cpp
@@ -343,8 +343,14 @@ struct Heap2LocalOptimizer {
       }
 
       // Breaks that our allocation flows through may change type, as we now
-      // have a nullable type there.
-      curr->finalize();
+      // have a nullable type there. This is simple to fix as we know this is a
+      // reachable br_if with a value, and the only possible change there is to
+      // become nullable.
+      assert(curr->type != Type::unreachable);
+      assert(curr->value);
+      if (curr->type.isNonNullable()) {
+        curr->type = Type(curr->type.getHeapType(), Nullable);
+      }
     }
 
     void visitStructNew(StructNew* curr) {
diff --git a/test/lit/passes/heap2local.wast b/test/lit/passes/heap2local.wast
index b693c49ee95..fc2e499dbd9 100644
--- a/test/lit/passes/heap2local.wast
+++ b/test/lit/passes/heap2local.wast
@@ -1952,6 +1952,22 @@
       )
     )
   )
+
+  (func $br_if
+    ;; The br_if here must change type after we optimize, as then a nullable
+    ;; type will be flowing through it.
+    (drop
+      (block $label$1 (result (ref $struct.A))
+        (drop
+          (br_if $label$1
+            (struct.new_default $struct.A)
+            (i32.const 0)
+           )
+         )
+        (unreachable)
+      )
+    )
+  )
 )
 
 (module

From 4300fe2fb74dcfc25ef16d84d885010d373b0b01 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Thu, 7 Mar 2024 17:02:27 -0800
Subject: [PATCH 32/49] fix

---
 test/lit/passes/heap2local.wast | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/test/lit/passes/heap2local.wast b/test/lit/passes/heap2local.wast
index fc2e499dbd9..bfb0df38f48 100644
--- a/test/lit/passes/heap2local.wast
+++ b/test/lit/passes/heap2local.wast
@@ -1953,6 +1953,29 @@
     )
   )
 
+  ;; CHECK:      (func $br_if (type $1)
+  ;; CHECK-NEXT:  (local $0 i32)
+  ;; CHECK-NEXT:  (local $1 f64)
+  ;; CHECK-NEXT:  (drop
+  ;; CHECK-NEXT:   (block $label$1 (result (ref null $struct.A))
+  ;; CHECK-NEXT:    (drop
+  ;; CHECK-NEXT:     (br_if $label$1
+  ;; CHECK-NEXT:      (block (result nullref)
+  ;; CHECK-NEXT:       (local.set $0
+  ;; CHECK-NEXT:        (i32.const 0)
+  ;; CHECK-NEXT:       )
+  ;; CHECK-NEXT:       (local.set $1
+  ;; CHECK-NEXT:        (f64.const 0)
+  ;; CHECK-NEXT:       )
+  ;; CHECK-NEXT:       (ref.null none)
+  ;; CHECK-NEXT:      )
+  ;; CHECK-NEXT:      (i32.const 0)
+  ;; CHECK-NEXT:     )
+  ;; CHECK-NEXT:    )
+  ;; CHECK-NEXT:    (unreachable)
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT: )
   (func $br_if
     ;; The br_if here must change type after we optimize, as then a nullable
     ;; type will be flowing through it.

From 3350f9e8156f47d69116ec7bed9302c84f7847e7 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Fri, 8 Mar 2024 08:54:34 -0800
Subject: [PATCH 33/49] fix

---
 src/passes/Heap2Local.cpp | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/passes/Heap2Local.cpp b/src/passes/Heap2Local.cpp
index d58bfdfe01b..04921f18236 100644
--- a/src/passes/Heap2Local.cpp
+++ b/src/passes/Heap2Local.cpp
@@ -344,9 +344,8 @@ struct Heap2LocalOptimizer {
 
       // Breaks that our allocation flows through may change type, as we now
       // have a nullable type there. This is simple to fix as we know this is a
-      // reachable br_if with a value, and the only possible change there is to
-      // become nullable.
-      assert(curr->type != Type::unreachable);
+      // break with a value, and the only possible change there is to become
+      // nullable.
       assert(curr->value);
       if (curr->type.isNonNullable()) {
         curr->type = Type(curr->type.getHeapType(), Nullable);

From e5719731c13e44220b6e10ff2caea9b7c5fb42b5 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Fri, 8 Mar 2024 10:59:31 -0800
Subject: [PATCH 34/49] fix

---
 src/passes/RemoveUnusedBrs.cpp | 43 +++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/src/passes/RemoveUnusedBrs.cpp b/src/passes/RemoveUnusedBrs.cpp
index 521c721d1b9..2a0a480833b 100644
--- a/src/passes/RemoveUnusedBrs.cpp
+++ b/src/passes/RemoveUnusedBrs.cpp
@@ -272,11 +272,14 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
           break;
         }
       }
+      // Otherwise it is ok for properly-typed values to flow out.
+      self->stopUnrefinedValueFlow(curr->type);
     } else if (curr->is<Nop>()) {
       // ignore (could be result of a previous cycle)
       self->stopValueFlow();
     } else if (curr->is<Loop>()) {
-      // do nothing - it's ok for values to flow out
+      // As with a block, it is ok for properly-typed values to flow out.
+      self->stopUnrefinedValueFlow(curr->type);
     } else if (auto* sw = curr->dynCast<Switch>()) {
       self->stopFlow();
       self->optimizeSwitch(sw);
@@ -303,6 +306,44 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
 
   void stopValueFlow() { removeValueFlow(flows); }
 
+  // Stop a value from flowing that is not as refined as a given type. For
+  // example:
+  //
+  //  (block (result $sub)
+  //    ..maybe other branches to the block..
+  //    (return (super))    ;; the function returns super; the block is sub
+  //  )
+  //
+  //  If we let the value flow through, we'd be forced to unrefine the block:
+  //
+  //  (block (result $super)
+  //    ..maybe other branches to the block..
+  //    (super)
+  //  )
+  //
+  // It is best to avoid unrefining like that, so keep the |return|.
+  void stopUnrefinedValueFlow(Type type) {
+    if (!getModule()->features.hasGC()) {
+      return;
+    }
+    flows.erase(std::remove_if(flows.begin(),
+                               flows.end(),
+                               [&](Expression** currp) {
+                                 auto* curr = *currp;
+                                 Expression* value;
+                                 if (auto* ret = curr->dynCast<Return>()) {
+                                   value = ret->value;
+                                 } else {
+                                   value = curr->cast<Break>()->value;
+                                 }
+                                 // Remove if there is a value, and it is
+                                 // unrefined.
+                                 return value &&
+                                        !Type::isSubType(value->type, type);
+                               }),
+                flows.end());
+  }
+
   static void clear(RemoveUnusedBrs* self, Expression** currp) {
     self->flows.clear();
   }

From 31d70a1adc6db3efff753b26aaa6ced3cb2f3e76 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Fri, 8 Mar 2024 11:02:55 -0800
Subject: [PATCH 35/49] test

---
 test/lit/metadce/sourcemap.wat            |  1 +
 test/lit/passes/global-effects-O.wast     | 24 ++++++++++++++--------
 test/lit/passes/remove-unused-brs-gc.wast | 25 +++++++++++++++++++----
 3 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/test/lit/metadce/sourcemap.wat b/test/lit/metadce/sourcemap.wat
index 8a73a01da57..c6274722414 100644
--- a/test/lit/metadce/sourcemap.wat
+++ b/test/lit/metadce/sourcemap.wat
@@ -32,3 +32,4 @@
 ;; BIN-NEXT:  (nop)
 ;; BIN-NEXT:  ;;@ a:3:1
 ;; BIN-NEXT: )
+
diff --git a/test/lit/passes/global-effects-O.wast b/test/lit/passes/global-effects-O.wast
index c24159eace4..d8016470f51 100644
--- a/test/lit/passes/global-effects-O.wast
+++ b/test/lit/passes/global-effects-O.wast
@@ -104,7 +104,7 @@
   ;; CHECK_0-NEXT: )
   ;; CHECK_1:      (func $pointless-work (type $1) (result i32)
   ;; CHECK_1-NEXT:  (local $0 i32)
-  ;; CHECK_1-NEXT:  (loop $loop (result i32)
+  ;; CHECK_1-NEXT:  (loop $loop
   ;; CHECK_1-NEXT:   (br_if $loop
   ;; CHECK_1-NEXT:    (i32.lt_u
   ;; CHECK_1-NEXT:     (local.tee $0
@@ -116,12 +116,14 @@
   ;; CHECK_1-NEXT:     (i32.const 12345678)
   ;; CHECK_1-NEXT:    )
   ;; CHECK_1-NEXT:   )
-  ;; CHECK_1-NEXT:   (local.get $0)
+  ;; CHECK_1-NEXT:   (return
+  ;; CHECK_1-NEXT:    (local.get $0)
+  ;; CHECK_1-NEXT:   )
   ;; CHECK_1-NEXT:  )
   ;; CHECK_1-NEXT: )
   ;; CHECK_3:      (func $pointless-work (type $1) (; has Stack IR ;) (result i32)
   ;; CHECK_3-NEXT:  (local $0 i32)
-  ;; CHECK_3-NEXT:  (loop $loop (result i32)
+  ;; CHECK_3-NEXT:  (loop $loop
   ;; CHECK_3-NEXT:   (br_if $loop
   ;; CHECK_3-NEXT:    (i32.lt_u
   ;; CHECK_3-NEXT:     (local.tee $0
@@ -133,12 +135,14 @@
   ;; CHECK_3-NEXT:     (i32.const 12345678)
   ;; CHECK_3-NEXT:    )
   ;; CHECK_3-NEXT:   )
-  ;; CHECK_3-NEXT:   (local.get $0)
+  ;; CHECK_3-NEXT:   (return
+  ;; CHECK_3-NEXT:    (local.get $0)
+  ;; CHECK_3-NEXT:   )
   ;; CHECK_3-NEXT:  )
   ;; CHECK_3-NEXT: )
   ;; CHECK_s:      (func $pointless-work (type $1) (; has Stack IR ;) (result i32)
   ;; CHECK_s-NEXT:  (local $0 i32)
-  ;; CHECK_s-NEXT:  (loop $loop (result i32)
+  ;; CHECK_s-NEXT:  (loop $loop
   ;; CHECK_s-NEXT:   (br_if $loop
   ;; CHECK_s-NEXT:    (i32.lt_u
   ;; CHECK_s-NEXT:     (local.tee $0
@@ -150,12 +154,14 @@
   ;; CHECK_s-NEXT:     (i32.const 12345678)
   ;; CHECK_s-NEXT:    )
   ;; CHECK_s-NEXT:   )
-  ;; CHECK_s-NEXT:   (local.get $0)
+  ;; CHECK_s-NEXT:   (return
+  ;; CHECK_s-NEXT:    (local.get $0)
+  ;; CHECK_s-NEXT:   )
   ;; CHECK_s-NEXT:  )
   ;; CHECK_s-NEXT: )
   ;; CHECK_O:      (func $pointless-work (type $1) (; has Stack IR ;) (result i32)
   ;; CHECK_O-NEXT:  (local $0 i32)
-  ;; CHECK_O-NEXT:  (loop $loop (result i32)
+  ;; CHECK_O-NEXT:  (loop $loop
   ;; CHECK_O-NEXT:   (br_if $loop
   ;; CHECK_O-NEXT:    (i32.lt_u
   ;; CHECK_O-NEXT:     (local.tee $0
@@ -167,7 +173,9 @@
   ;; CHECK_O-NEXT:     (i32.const 12345678)
   ;; CHECK_O-NEXT:    )
   ;; CHECK_O-NEXT:   )
-  ;; CHECK_O-NEXT:   (local.get $0)
+  ;; CHECK_O-NEXT:   (return
+  ;; CHECK_O-NEXT:    (local.get $0)
+  ;; CHECK_O-NEXT:   )
   ;; CHECK_O-NEXT:  )
   ;; CHECK_O-NEXT: )
   (func $pointless-work (result i32)
diff --git a/test/lit/passes/remove-unused-brs-gc.wast b/test/lit/passes/remove-unused-brs-gc.wast
index 9dc871e326b..9306925c8ca 100644
--- a/test/lit/passes/remove-unused-brs-gc.wast
+++ b/test/lit/passes/remove-unused-brs-gc.wast
@@ -48,7 +48,7 @@
   )
  )
 
- ;; CHECK:      (func $br_on_cast (type $4) (result (ref $struct))
+ ;; CHECK:      (func $br_on_cast (type $5) (result (ref $struct))
  ;; CHECK-NEXT:  (local $struct (ref null $struct))
  ;; CHECK-NEXT:  (block $block (result (ref $struct))
  ;; CHECK-NEXT:   (drop
@@ -99,7 +99,7 @@
   )
  )
 
- ;; CHECK:      (func $br_on_cast-fallthrough (type $4) (result (ref $struct))
+ ;; CHECK:      (func $br_on_cast-fallthrough (type $5) (result (ref $struct))
  ;; CHECK-NEXT:  (local $struct (ref null $struct))
  ;; CHECK-NEXT:  (local $any anyref)
  ;; CHECK-NEXT:  (block $block (result (ref $struct))
@@ -190,7 +190,7 @@
   )
  )
 
- ;; CHECK:      (func $br_on_cast_unrelated (type $5) (result (ref null $struct))
+ ;; CHECK:      (func $br_on_cast_unrelated (type $4) (result (ref null $struct))
  ;; CHECK-NEXT:  (local $nullable-struct2 (ref null $struct2))
  ;; CHECK-NEXT:  (block $block (result nullref)
  ;; CHECK-NEXT:   (drop
@@ -244,7 +244,7 @@
   )
  )
 
- ;; CHECK:      (func $br_on_cast_unrelated-fallthrough (type $5) (result (ref null $struct))
+ ;; CHECK:      (func $br_on_cast_unrelated-fallthrough (type $4) (result (ref null $struct))
  ;; CHECK-NEXT:  (local $any anyref)
  ;; CHECK-NEXT:  (local $nullable-struct2 (ref null $struct2))
  ;; CHECK-NEXT:  (block $block (result nullref)
@@ -782,4 +782,21 @@
    )
   )
  )
+
+ ;; CHECK:      (func $never-unrefine (type $4) (result (ref null $struct))
+ ;; CHECK-NEXT:  (block $block (result (ref $substruct))
+ ;; CHECK-NEXT:   (nop)
+ ;; CHECK-NEXT:   (return
+ ;; CHECK-NEXT:    (struct.new_default $struct)
+ ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT: )
+ (func $never-unrefine (result (ref null $struct))
+  (block $block (result (ref $substruct))
+   (nop) ;; avoid this being a trivial block
+   (return
+    (struct.new_default $struct)
+   )
+  )
+ )
 )

From 4377a8d3969f24fac0c179f8f620f0041b0da5b9 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Fri, 8 Mar 2024 11:06:32 -0800
Subject: [PATCH 36/49] fix

---
 test/lit/passes/remove-unused-brs-gc.wast | 43 +++++++++++++++++------
 1 file changed, 32 insertions(+), 11 deletions(-)

diff --git a/test/lit/passes/remove-unused-brs-gc.wast b/test/lit/passes/remove-unused-brs-gc.wast
index 9306925c8ca..d73a996cb4e 100644
--- a/test/lit/passes/remove-unused-brs-gc.wast
+++ b/test/lit/passes/remove-unused-brs-gc.wast
@@ -190,7 +190,7 @@
   )
  )
 
- ;; CHECK:      (func $br_on_cast_unrelated (type $4) (result (ref null $struct))
+ ;; CHECK:      (func $br_on_cast_unrelated (type $3) (result (ref null $struct))
  ;; CHECK-NEXT:  (local $nullable-struct2 (ref null $struct2))
  ;; CHECK-NEXT:  (block $block (result nullref)
  ;; CHECK-NEXT:   (drop
@@ -244,7 +244,7 @@
   )
  )
 
- ;; CHECK:      (func $br_on_cast_unrelated-fallthrough (type $4) (result (ref null $struct))
+ ;; CHECK:      (func $br_on_cast_unrelated-fallthrough (type $3) (result (ref null $struct))
  ;; CHECK-NEXT:  (local $any anyref)
  ;; CHECK-NEXT:  (local $nullable-struct2 (ref null $struct2))
  ;; CHECK-NEXT:  (block $block (result nullref)
@@ -307,7 +307,7 @@
   )
  )
 
- ;; CHECK:      (func $br_on_cast_fail (type $3) (result anyref)
+ ;; CHECK:      (func $br_on_cast_fail (type $4) (result anyref)
  ;; CHECK-NEXT:  (local $struct (ref null $struct))
  ;; CHECK-NEXT:  (block $block (result (ref null $struct))
  ;; CHECK-NEXT:   (drop
@@ -354,7 +354,7 @@
   )
  )
 
- ;; CHECK:      (func $br_on_cast_fail-fallthrough (type $3) (result anyref)
+ ;; CHECK:      (func $br_on_cast_fail-fallthrough (type $4) (result anyref)
  ;; CHECK-NEXT:  (local $any anyref)
  ;; CHECK-NEXT:  (local $struct (ref null $struct))
  ;; CHECK-NEXT:  (block $block (result anyref)
@@ -410,7 +410,7 @@
   )
  )
 
- ;; CHECK:      (func $br_on_cast_fail_unrelated (type $3) (result anyref)
+ ;; CHECK:      (func $br_on_cast_fail_unrelated (type $4) (result anyref)
  ;; CHECK-NEXT:  (local $nullable-struct2 (ref null $struct2))
  ;; CHECK-NEXT:  (block $block (result (ref null $struct2))
  ;; CHECK-NEXT:   (drop
@@ -472,7 +472,7 @@
   )
  )
 
- ;; CHECK:      (func $br_on_cast_fail_unrelated-fallthrough (type $3) (result anyref)
+ ;; CHECK:      (func $br_on_cast_fail_unrelated-fallthrough (type $4) (result anyref)
  ;; CHECK-NEXT:  (local $any anyref)
  ;; CHECK-NEXT:  (local $nullable-struct2 (ref null $struct2))
  ;; CHECK-NEXT:  (block $block (result anyref)
@@ -783,15 +783,19 @@
   )
  )
 
- ;; CHECK:      (func $never-unrefine (type $4) (result (ref null $struct))
- ;; CHECK-NEXT:  (block $block (result (ref $substruct))
+ ;; CHECK:      (func $never-unrefine (type $3) (result (ref null $struct))
+ ;; CHECK-NEXT:  (block $block (result (ref $struct))
  ;; CHECK-NEXT:   (nop)
- ;; CHECK-NEXT:   (return
- ;; CHECK-NEXT:    (struct.new_default $struct)
- ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:   (struct.new_default $struct)
  ;; CHECK-NEXT:  )
  ;; CHECK-NEXT: )
  (func $never-unrefine (result (ref null $struct))
+  ;; This block is more refined than the function's result, and we should not
+  ;; unrefine it (which would happen if we removed the |return|).
+  ;;
+  ;; In this trivial case the block would be removed by vacuum anyhow, but in
+  ;; other situations it can be harmful to unrefine; for example, if there are
+  ;; br_ifs targeting the block as well, then they would end up unrefined too.
   (block $block (result (ref $substruct))
    (nop) ;; avoid this being a trivial block
    (return
@@ -799,4 +803,21 @@
    )
   )
  )
+
+ ;; CHECK:      (func $never-unrefine-ok (type $3) (result (ref null $struct))
+ ;; CHECK-NEXT:  (block $block (result (ref $substruct))
+ ;; CHECK-NEXT:   (nop)
+ ;; CHECK-NEXT:   (struct.new_default $substruct)
+ ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT: )
+ (func $never-unrefine-ok (result (ref null $struct))
+  ;; As above, but now we send in a sufficiently-refined type, so we can
+  ;; optimize and remove the return.
+  (block $block (result (ref $substruct))
+   (nop)
+   (return
+    (struct.new_default $substruct) ;; this changed
+   )
+  )
+ )
 )

From 6d14b8ae1f0ec41407da5aba20fde50e9e8ad717 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Fri, 8 Mar 2024 11:09:02 -0800
Subject: [PATCH 37/49] test

---
 test/lit/passes/remove-unused-brs-gc.wast | 59 +++++++++++++++--------
 1 file changed, 40 insertions(+), 19 deletions(-)

diff --git a/test/lit/passes/remove-unused-brs-gc.wast b/test/lit/passes/remove-unused-brs-gc.wast
index d73a996cb4e..5691fd31063 100644
--- a/test/lit/passes/remove-unused-brs-gc.wast
+++ b/test/lit/passes/remove-unused-brs-gc.wast
@@ -7,13 +7,18 @@
   ;; CHECK:      (rec
   ;; CHECK-NEXT:  (type $struct (sub (struct )))
   (type $struct (sub (struct)))
+
   ;; CHECK:       (type $struct2 (struct ))
   (type $struct2 (struct))
+
   ;; CHECK:       (type $substruct (sub $struct (struct )))
   (type $substruct (sub $struct (struct)))
+
+  ;; CHECK:       (type $subsubstruct (sub $substruct (struct )))
+  (type $subsubstruct (sub $substruct (struct)))
  )
 
- ;; CHECK:      (func $br_on-if (type $7) (param $0 (ref struct))
+ ;; CHECK:      (func $br_on-if (type $8) (param $0 (ref struct))
  ;; CHECK-NEXT:  (block $label
  ;; CHECK-NEXT:   (drop
  ;; CHECK-NEXT:    (select (result (ref struct))
@@ -48,7 +53,7 @@
   )
  )
 
- ;; CHECK:      (func $br_on_cast (type $5) (result (ref $struct))
+ ;; CHECK:      (func $br_on_cast (type $6) (result (ref $struct))
  ;; CHECK-NEXT:  (local $struct (ref null $struct))
  ;; CHECK-NEXT:  (block $block (result (ref $struct))
  ;; CHECK-NEXT:   (drop
@@ -99,7 +104,7 @@
   )
  )
 
- ;; CHECK:      (func $br_on_cast-fallthrough (type $5) (result (ref $struct))
+ ;; CHECK:      (func $br_on_cast-fallthrough (type $6) (result (ref $struct))
  ;; CHECK-NEXT:  (local $struct (ref null $struct))
  ;; CHECK-NEXT:  (local $any anyref)
  ;; CHECK-NEXT:  (block $block (result (ref $struct))
@@ -162,7 +167,7 @@
   )
  )
 
- ;; CHECK:      (func $nested_br_on_cast (type $8) (result i31ref)
+ ;; CHECK:      (func $nested_br_on_cast (type $9) (result i31ref)
  ;; CHECK-NEXT:  (block $label$1 (result (ref i31))
  ;; CHECK-NEXT:   (drop
  ;; CHECK-NEXT:    (br $label$1
@@ -190,7 +195,7 @@
   )
  )
 
- ;; CHECK:      (func $br_on_cast_unrelated (type $3) (result (ref null $struct))
+ ;; CHECK:      (func $br_on_cast_unrelated (type $4) (result (ref null $struct))
  ;; CHECK-NEXT:  (local $nullable-struct2 (ref null $struct2))
  ;; CHECK-NEXT:  (block $block (result nullref)
  ;; CHECK-NEXT:   (drop
@@ -244,7 +249,7 @@
   )
  )
 
- ;; CHECK:      (func $br_on_cast_unrelated-fallthrough (type $3) (result (ref null $struct))
+ ;; CHECK:      (func $br_on_cast_unrelated-fallthrough (type $4) (result (ref null $struct))
  ;; CHECK-NEXT:  (local $any anyref)
  ;; CHECK-NEXT:  (local $nullable-struct2 (ref null $struct2))
  ;; CHECK-NEXT:  (block $block (result nullref)
@@ -307,7 +312,7 @@
   )
  )
 
- ;; CHECK:      (func $br_on_cast_fail (type $4) (result anyref)
+ ;; CHECK:      (func $br_on_cast_fail (type $5) (result anyref)
  ;; CHECK-NEXT:  (local $struct (ref null $struct))
  ;; CHECK-NEXT:  (block $block (result (ref null $struct))
  ;; CHECK-NEXT:   (drop
@@ -354,7 +359,7 @@
   )
  )
 
- ;; CHECK:      (func $br_on_cast_fail-fallthrough (type $4) (result anyref)
+ ;; CHECK:      (func $br_on_cast_fail-fallthrough (type $5) (result anyref)
  ;; CHECK-NEXT:  (local $any anyref)
  ;; CHECK-NEXT:  (local $struct (ref null $struct))
  ;; CHECK-NEXT:  (block $block (result anyref)
@@ -410,7 +415,7 @@
   )
  )
 
- ;; CHECK:      (func $br_on_cast_fail_unrelated (type $4) (result anyref)
+ ;; CHECK:      (func $br_on_cast_fail_unrelated (type $5) (result anyref)
  ;; CHECK-NEXT:  (local $nullable-struct2 (ref null $struct2))
  ;; CHECK-NEXT:  (block $block (result (ref null $struct2))
  ;; CHECK-NEXT:   (drop
@@ -472,7 +477,7 @@
   )
  )
 
- ;; CHECK:      (func $br_on_cast_fail_unrelated-fallthrough (type $4) (result anyref)
+ ;; CHECK:      (func $br_on_cast_fail_unrelated-fallthrough (type $5) (result anyref)
  ;; CHECK-NEXT:  (local $any anyref)
  ;; CHECK-NEXT:  (local $nullable-struct2 (ref null $struct2))
  ;; CHECK-NEXT:  (block $block (result anyref)
@@ -543,7 +548,7 @@
   )
  )
 
- ;; CHECK:      (func $br_on_cast-unreachable (type $6) (param $i31ref i31ref) (result anyref)
+ ;; CHECK:      (func $br_on_cast-unreachable (type $7) (param $i31ref i31ref) (result anyref)
  ;; CHECK-NEXT:  (block $block
  ;; CHECK-NEXT:   (drop
  ;; CHECK-NEXT:    (block
@@ -599,7 +604,7 @@
   )
  )
 
- ;; CHECK:      (func $fallthrough-unreachable (type $6) (param $0 i31ref) (result anyref)
+ ;; CHECK:      (func $fallthrough-unreachable (type $7) (param $0 i31ref) (result anyref)
  ;; CHECK-NEXT:  (block $outer
  ;; CHECK-NEXT:   (drop
  ;; CHECK-NEXT:    (block ;; (replaces unreachable RefCast we can't emit)
@@ -638,7 +643,7 @@
   )
  )
 
- ;; CHECK:      (func $casts-are-costly (type $9) (param $x i32)
+ ;; CHECK:      (func $casts-are-costly (type $10) (param $x i32)
  ;; CHECK-NEXT:  (local $struct (ref null $struct))
  ;; CHECK-NEXT:  (drop
  ;; CHECK-NEXT:   (if (result i32)
@@ -783,7 +788,7 @@
   )
  )
 
- ;; CHECK:      (func $never-unrefine (type $3) (result (ref null $struct))
+ ;; CHECK:      (func $never-unrefine (type $4) (result (ref null $struct))
  ;; CHECK-NEXT:  (block $block (result (ref $struct))
  ;; CHECK-NEXT:   (nop)
  ;; CHECK-NEXT:   (struct.new_default $struct)
@@ -791,7 +796,7 @@
  ;; CHECK-NEXT: )
  (func $never-unrefine (result (ref null $struct))
   ;; This block is more refined than the function's result, and we should not
-  ;; unrefine it (which would happen if we removed the |return|).
+  ;; unrefine it.
   ;;
   ;; In this trivial case the block would be removed by vacuum anyhow, but in
   ;; other situations it can be harmful to unrefine; for example, if there are
@@ -804,15 +809,15 @@
   )
  )
 
- ;; CHECK:      (func $never-unrefine-ok (type $3) (result (ref null $struct))
+ ;; CHECK:      (func $never-unrefine-ok (type $4) (result (ref null $struct))
  ;; CHECK-NEXT:  (block $block (result (ref $substruct))
  ;; CHECK-NEXT:   (nop)
  ;; CHECK-NEXT:   (struct.new_default $substruct)
  ;; CHECK-NEXT:  )
  ;; CHECK-NEXT: )
  (func $never-unrefine-ok (result (ref null $struct))
-  ;; As above, but now we send in a sufficiently-refined type, so we can
-  ;; optimize and remove the return.
+  ;; As above, but now we send in an equal type. Again, the block should remain
+  ;; as refined as it is (which is trivial in this case).
   (block $block (result (ref $substruct))
    (nop)
    (return
@@ -820,4 +825,20 @@
    )
   )
  )
-)
+
+ ;; CHECK:      (func $never-unrefine-ok-2 (type $4) (result (ref null $struct))
+ ;; CHECK-NEXT:  (block $block (result (ref $subsubstruct))
+ ;; CHECK-NEXT:   (nop)
+ ;; CHECK-NEXT:   (struct.new_default $subsubstruct)
+ ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT: )
+ (func $never-unrefine-ok-2 (result (ref null $struct))
+  ;; As above, but now we send in a strict subtype. In this case we end up
+  ;; refining the block.
+  (block $block (result (ref $substruct))
+   (nop)
+   (return
+    (struct.new_default $subsubstruct) ;; this changed
+   )
+  )
+ ))

From eda3d263e859728dbdbc3eb9e090843ba5585664 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Fri, 8 Mar 2024 11:51:36 -0800
Subject: [PATCH 38/49] fix

---
 src/binaryen-c.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/binaryen-c.cpp b/src/binaryen-c.cpp
index 43e3262f461..7da3cc6ad5e 100644
--- a/src/binaryen-c.cpp
+++ b/src/binaryen-c.cpp
@@ -1128,7 +1128,7 @@ BinaryenExpressionRef BinaryenBreak(BinaryenModuleRef module,
   } else {
     // As a convenience for users, if type is not provided but we can infer the
     // type - which is the case for MVP types - then infer it.
-    if (condition && value && !value->type.isRef()) {
+    if (condition && value && !value->type.containsRef()) {
       optType = value->type;
     }
   }

From 50f3dd2a2fa431214a861fbb0e908c9b515aac48 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Fri, 8 Mar 2024 11:54:18 -0800
Subject: [PATCH 39/49] fix

---
 src/ir/utils.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/ir/utils.h b/src/ir/utils.h
index b8fdb9f1bf2..a0668d10287 100644
--- a/src/ir/utils.h
+++ b/src/ir/utils.h
@@ -137,6 +137,7 @@ struct ReFinalize
   // type changes. We must also track block names to blocks for this, so we
   // track all that using the block name as the key.
   struct BlockBrInfo {
+    // The block and its relevant breaks.
     Block* block = nullptr;
     std::vector<Break*> brs;
 

From ea7b3fd7480f2e345767465a6b26efb48229d1e2 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Fri, 8 Mar 2024 11:57:00 -0800
Subject: [PATCH 40/49] fix

---
 src/passes/RemoveUnusedBrs.cpp | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/passes/RemoveUnusedBrs.cpp b/src/passes/RemoveUnusedBrs.cpp
index 2a0a480833b..d2b55bcf18e 100644
--- a/src/passes/RemoveUnusedBrs.cpp
+++ b/src/passes/RemoveUnusedBrs.cpp
@@ -140,10 +140,12 @@ static bool tooCostlyToRunUnconditionally(const PassOptions& passOptions,
   return tooCostlyToRunUnconditionally(passOptions, max);
 }
 
-// Utility to update a br_if type based on its value and the type of the block
-// it branches to. Receives a map of block name to block types. The map does not
-// need to contain blocks with MVP types, as we can infer those from the value.
-// This also finalizes at the end.
+// Utility to update a br_if type after a change. This receives the br and a
+// mapping of block names to their types, which it uses if it needs to (in
+// particular, we do not need the mapping to figure out the type of a br_if
+// when the value has MVP type, so the mapping can only contain things relevant
+// to GC). This also calls finalize as needed, so that after calling it the
+// br_if type is fully updated.
 using BlockTypeMap = std::unordered_map<Name, Type>;
 static void updateBrIfType(Break* br, const BlockTypeMap& blockTypeMap) {
   assert(br->condition);

From ee48896398e3ececdf5f166f5e0b021de2d3474e Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Fri, 8 Mar 2024 11:57:55 -0800
Subject: [PATCH 41/49] fix

---
 src/passes/RemoveUnusedBrs.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/passes/RemoveUnusedBrs.cpp b/src/passes/RemoveUnusedBrs.cpp
index d2b55bcf18e..4be8d888d75 100644
--- a/src/passes/RemoveUnusedBrs.cpp
+++ b/src/passes/RemoveUnusedBrs.cpp
@@ -323,7 +323,8 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
   //    (super)
   //  )
   //
-  // It is best to avoid unrefining like that, so keep the |return|.
+  // It is best to avoid unrefining like that, so block the flow of such values
+  // here.
   void stopUnrefinedValueFlow(Type type) {
     if (!getModule()->features.hasGC()) {
       return;

From d6ff014d88e5780c0ca689e6a63c373ae5b0a5dd Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Fri, 8 Mar 2024 11:59:22 -0800
Subject: [PATCH 42/49] fix

---
 src/passes/RemoveUnusedBrs.cpp | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/passes/RemoveUnusedBrs.cpp b/src/passes/RemoveUnusedBrs.cpp
index 4be8d888d75..01462a532be 100644
--- a/src/passes/RemoveUnusedBrs.cpp
+++ b/src/passes/RemoveUnusedBrs.cpp
@@ -547,10 +547,13 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
     }
 
     if (auto* block = curr->dynCast<Block>()) {
-      if (block->type.containsRef()) {
+      if (block->name.is() && block->type.containsRef()) {
+        // This block has a type that may be needed for updating br_ifs, so
+        // note it.
         self->blockTypes[block->name] = block->type;
       }
     }
+
     super::scan(self, currp);
   }
 

From 8b32aacfe68c6bb840cf92c2974d944d71b27f58 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Fri, 8 Mar 2024 12:00:10 -0800
Subject: [PATCH 43/49] fix

---
 src/passes/RemoveUnusedBrs.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/passes/RemoveUnusedBrs.cpp b/src/passes/RemoveUnusedBrs.cpp
index 01462a532be..f989bf5dc2c 100644
--- a/src/passes/RemoveUnusedBrs.cpp
+++ b/src/passes/RemoveUnusedBrs.cpp
@@ -1759,7 +1759,7 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
       // have changed since then.
       static void scan(FinalOptimizer* self, Expression** currp) {
         if (auto* block = (*currp)->dynCast<Block>()) {
-          if (block->type.containsRef()) {
+          if (block->name.is() && block->type.containsRef()) {
             self->blockTypes[block->name] = block->type;
           }
         }

From b274bbbde820f42728caf5496b1a256eaeb55c0d Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Fri, 8 Mar 2024 12:02:50 -0800
Subject: [PATCH 44/49] fix

---
 src/wasm/wasm-validator.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp
index a29f7e48742..cfaaa2b34fd 100644
--- a/src/wasm/wasm-validator.cpp
+++ b/src/wasm/wasm-validator.cpp
@@ -801,7 +801,7 @@ void FunctionValidator::visitLoop(Loop* curr) {
     // Validate there are no br_ifs with values targeting us.
     shouldBeTrue(!labelBrIfs.count(curr->name),
                  curr,
-                 "loops cannot have br_ifs ith values targeting them");
+                 "loops cannot have br_ifs with values targeting them");
   }
   if (curr->type == Type::none) {
     shouldBeFalse(curr->body->type.isConcrete(),

From b4ebae987fc4d1157d22ce746668aeeab73480a9 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Fri, 8 Mar 2024 12:06:39 -0800
Subject: [PATCH 45/49] fix

---
 test/lit/metadce/sourcemap.wat | 1 -
 1 file changed, 1 deletion(-)

diff --git a/test/lit/metadce/sourcemap.wat b/test/lit/metadce/sourcemap.wat
index c6274722414..8a73a01da57 100644
--- a/test/lit/metadce/sourcemap.wat
+++ b/test/lit/metadce/sourcemap.wat
@@ -32,4 +32,3 @@
 ;; BIN-NEXT:  (nop)
 ;; BIN-NEXT:  ;;@ a:3:1
 ;; BIN-NEXT: )
-

From b947d7ca1207adef0781adc1da1eaa3509d7c2b1 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Fri, 8 Mar 2024 13:06:34 -0800
Subject: [PATCH 46/49] fix

---
 test/lit/passes/remove-unused-brs-gc.wast | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/test/lit/passes/remove-unused-brs-gc.wast b/test/lit/passes/remove-unused-brs-gc.wast
index 5691fd31063..2588973a91d 100644
--- a/test/lit/passes/remove-unused-brs-gc.wast
+++ b/test/lit/passes/remove-unused-brs-gc.wast
@@ -789,14 +789,17 @@
  )
 
  ;; CHECK:      (func $never-unrefine (type $4) (result (ref null $struct))
- ;; CHECK-NEXT:  (block $block (result (ref $struct))
+ ;; CHECK-NEXT:  (block $block (result (ref $substruct))
  ;; CHECK-NEXT:   (nop)
- ;; CHECK-NEXT:   (struct.new_default $struct)
+ ;; CHECK-NEXT:   (return
+ ;; CHECK-NEXT:    (struct.new_default $struct)
+ ;; CHECK-NEXT:   )
  ;; CHECK-NEXT:  )
  ;; CHECK-NEXT: )
  (func $never-unrefine (result (ref null $struct))
   ;; This block is more refined than the function's result, and we should not
-  ;; unrefine it.
+  ;; unrefine it. Instead we should keep the |return| in place (other opts might
+  ;; end up optimizing such code, e.g. vacuum).
   ;;
   ;; In this trivial case the block would be removed by vacuum anyhow, but in
   ;; other situations it can be harmful to unrefine; for example, if there are
@@ -816,8 +819,8 @@
  ;; CHECK-NEXT:  )
  ;; CHECK-NEXT: )
  (func $never-unrefine-ok (result (ref null $struct))
-  ;; As above, but now we send in an equal type. Again, the block should remain
-  ;; as refined as it is (which is trivial in this case).
+  ;; As above, but now we send in an equal type. We can remove the |return|
+  ;; here.
   (block $block (result (ref $substruct))
    (nop)
    (return
@@ -834,7 +837,7 @@
  ;; CHECK-NEXT: )
  (func $never-unrefine-ok-2 (result (ref null $struct))
   ;; As above, but now we send in a strict subtype. In this case we end up
-  ;; refining the block.
+  ;; refining the block (and also removing the |return|).
   (block $block (result (ref $substruct))
    (nop)
    (return

From 84d98513115f1a37f8e540c20caca2d5590a4ca1 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Fri, 8 Mar 2024 13:20:39 -0800
Subject: [PATCH 47/49] fix

---
 src/passes/RemoveUnusedBrs.cpp        |  9 ++++++++-
 test/lit/passes/global-effects-O.wast | 24 ++++++++----------------
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/src/passes/RemoveUnusedBrs.cpp b/src/passes/RemoveUnusedBrs.cpp
index f989bf5dc2c..278d96590a1 100644
--- a/src/passes/RemoveUnusedBrs.cpp
+++ b/src/passes/RemoveUnusedBrs.cpp
@@ -326,7 +326,14 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
   // It is best to avoid unrefining like that, so block the flow of such values
   // here.
   void stopUnrefinedValueFlow(Type type) {
-    if (!getModule()->features.hasGC()) {
+    if (type == Type::unreachable) {
+      // We are flowing through something currently unreachable. Us flowing
+      // through it may turn it reachable, which is fine, and there are no
+      // subtyping risks here, so nothing to check.
+      return;
+    }
+    if (!type.containsRef()) {
+      // Subtyping cannot be an issue here.
       return;
     }
     flows.erase(std::remove_if(flows.begin(),
diff --git a/test/lit/passes/global-effects-O.wast b/test/lit/passes/global-effects-O.wast
index d8016470f51..c24159eace4 100644
--- a/test/lit/passes/global-effects-O.wast
+++ b/test/lit/passes/global-effects-O.wast
@@ -104,7 +104,7 @@
   ;; CHECK_0-NEXT: )
   ;; CHECK_1:      (func $pointless-work (type $1) (result i32)
   ;; CHECK_1-NEXT:  (local $0 i32)
-  ;; CHECK_1-NEXT:  (loop $loop
+  ;; CHECK_1-NEXT:  (loop $loop (result i32)
   ;; CHECK_1-NEXT:   (br_if $loop
   ;; CHECK_1-NEXT:    (i32.lt_u
   ;; CHECK_1-NEXT:     (local.tee $0
@@ -116,14 +116,12 @@
   ;; CHECK_1-NEXT:     (i32.const 12345678)
   ;; CHECK_1-NEXT:    )
   ;; CHECK_1-NEXT:   )
-  ;; CHECK_1-NEXT:   (return
-  ;; CHECK_1-NEXT:    (local.get $0)
-  ;; CHECK_1-NEXT:   )
+  ;; CHECK_1-NEXT:   (local.get $0)
   ;; CHECK_1-NEXT:  )
   ;; CHECK_1-NEXT: )
   ;; CHECK_3:      (func $pointless-work (type $1) (; has Stack IR ;) (result i32)
   ;; CHECK_3-NEXT:  (local $0 i32)
-  ;; CHECK_3-NEXT:  (loop $loop
+  ;; CHECK_3-NEXT:  (loop $loop (result i32)
   ;; CHECK_3-NEXT:   (br_if $loop
   ;; CHECK_3-NEXT:    (i32.lt_u
   ;; CHECK_3-NEXT:     (local.tee $0
@@ -135,14 +133,12 @@
   ;; CHECK_3-NEXT:     (i32.const 12345678)
   ;; CHECK_3-NEXT:    )
   ;; CHECK_3-NEXT:   )
-  ;; CHECK_3-NEXT:   (return
-  ;; CHECK_3-NEXT:    (local.get $0)
-  ;; CHECK_3-NEXT:   )
+  ;; CHECK_3-NEXT:   (local.get $0)
   ;; CHECK_3-NEXT:  )
   ;; CHECK_3-NEXT: )
   ;; CHECK_s:      (func $pointless-work (type $1) (; has Stack IR ;) (result i32)
   ;; CHECK_s-NEXT:  (local $0 i32)
-  ;; CHECK_s-NEXT:  (loop $loop
+  ;; CHECK_s-NEXT:  (loop $loop (result i32)
   ;; CHECK_s-NEXT:   (br_if $loop
   ;; CHECK_s-NEXT:    (i32.lt_u
   ;; CHECK_s-NEXT:     (local.tee $0
@@ -154,14 +150,12 @@
   ;; CHECK_s-NEXT:     (i32.const 12345678)
   ;; CHECK_s-NEXT:    )
   ;; CHECK_s-NEXT:   )
-  ;; CHECK_s-NEXT:   (return
-  ;; CHECK_s-NEXT:    (local.get $0)
-  ;; CHECK_s-NEXT:   )
+  ;; CHECK_s-NEXT:   (local.get $0)
   ;; CHECK_s-NEXT:  )
   ;; CHECK_s-NEXT: )
   ;; CHECK_O:      (func $pointless-work (type $1) (; has Stack IR ;) (result i32)
   ;; CHECK_O-NEXT:  (local $0 i32)
-  ;; CHECK_O-NEXT:  (loop $loop
+  ;; CHECK_O-NEXT:  (loop $loop (result i32)
   ;; CHECK_O-NEXT:   (br_if $loop
   ;; CHECK_O-NEXT:    (i32.lt_u
   ;; CHECK_O-NEXT:     (local.tee $0
@@ -173,9 +167,7 @@
   ;; CHECK_O-NEXT:     (i32.const 12345678)
   ;; CHECK_O-NEXT:    )
   ;; CHECK_O-NEXT:   )
-  ;; CHECK_O-NEXT:   (return
-  ;; CHECK_O-NEXT:    (local.get $0)
-  ;; CHECK_O-NEXT:   )
+  ;; CHECK_O-NEXT:   (local.get $0)
   ;; CHECK_O-NEXT:  )
   ;; CHECK_O-NEXT: )
   (func $pointless-work (result i32)

From 041dfd1082bf16b63b15cb6676bcc539269978ca Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Fri, 8 Mar 2024 13:22:57 -0800
Subject: [PATCH 48/49] format

---
 test/lit/passes/heap2local.wast | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/lit/passes/heap2local.wast b/test/lit/passes/heap2local.wast
index bfb0df38f48..acf624bfca1 100644
--- a/test/lit/passes/heap2local.wast
+++ b/test/lit/passes/heap2local.wast
@@ -1983,8 +1983,8 @@
       (block $label$1 (result (ref $struct.A))
         (drop
           (br_if $label$1
-            (struct.new_default $struct.A)
-            (i32.const 0)
+             (struct.new_default $struct.A)
+             (i32.const 0)
            )
          )
         (unreachable)

From ec3bb1740355589f1edc1b26cf818341f199c792 Mon Sep 17 00:00:00 2001
From: Alon Zakai <azakai@google.com>
Date: Fri, 8 Mar 2024 15:56:43 -0800
Subject: [PATCH 49/49] format

---
 src/wasm/wasm-validator.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp
index cfaaa2b34fd..cb32ad225d3 100644
--- a/src/wasm/wasm-validator.cpp
+++ b/src/wasm/wasm-validator.cpp
@@ -918,7 +918,7 @@ void FunctionValidator::visitBreak(Break* curr) {
   // child.
   if (curr->type == Type::unreachable && curr->condition) {
     shouldBeTrue(curr->condition->type == Type::unreachable ||
-                 (curr->value && curr->value->type == Type::unreachable),
+                   (curr->value && curr->value->type == Type::unreachable),
                  curr,
                  "unreachable break with condition or no unreachable child");
   }