fix blockifyMerge logic - it needs to not skip code in the block we merge to. since that's a fairly specific functionality needed in removeUnusedBrs, move it to there

Author: kripken

src/passes/RemoveUnusedBrs.cpp

@@ -232,11 +232,39 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
           // this is already an if-else. if one side is a dead end, we can append to the other, if
           // there is no returned value to concern us
           assert(!isConcreteWasmType(iff->type)); // can't be, since in the middle of a block
+
+          // ensures the first node is a block, if it isn't already, and merges in the second,
+          // either as a single element or, if a block, by appending to the first block. this
+          // keeps the order of operations in place, that is, the appended element will be
+          // executed after the first node's elements
+          auto blockifyMerge = [&](Expression* any, Expression* append) -> Block* {
+            Block* block = nullptr;
+            if (any) block = any->dynCast<Block>();
+            // if the first isn't a block, or it's a block with a name (so we might
+            // branch to the end, and so can't append to it, we might skip that code!)
+            // then make a new block
+            if (!block || block->name.is()) {
+              block = builder.makeBlock(any);
+            } else {
+              assert(!isConcreteWasmType(block->type));
+            }
+            auto* other = append->dynCast<Block>();
+            if (!other) {
+              block->list.push_back(append);
+            } else {
+              for (auto* item : other->list) {
+                block->list.push_back(item);
+              }
+            }
+            block->finalize();
+            return block;
+          };
+
           if (ExpressionAnalyzer::obviouslyDoesNotFlowOut(iff->ifTrue)) {
-            iff->ifFalse = builder.blockifyMerge(iff->ifFalse, builder.stealSlice(block, i + 1, list.size()));
+            iff->ifFalse = blockifyMerge(iff->ifFalse, builder.stealSlice(block, i + 1, list.size()));
             return true;
           } else if (ExpressionAnalyzer::obviouslyDoesNotFlowOut(iff->ifFalse)) {
-            iff->ifTrue = builder.blockifyMerge(iff->ifTrue, builder.stealSlice(block, i + 1, list.size()));
+            iff->ifTrue = blockifyMerge(iff->ifTrue, builder.stealSlice(block, i + 1, list.size()));
             return true;
           }
         }

src/wasm-builder.h

@@ -315,28 +315,6 @@ class Builder {
     return block;
   }
 
-  // ensures the first node is a block, if it isn't already, and merges in the second,
-  // either as a single element or, if a block, by appending to the first block
-  Block* blockifyMerge(Expression* any, Expression* append) {
-    Block* block = nullptr;
-    if (any) block = any->dynCast<Block>();
-    if (!block) {
-      block = makeBlock(any);
-    } else {
-      assert(!isConcreteWasmType(block->type));
-    }
-    auto* other = append->dynCast<Block>();
-    if (!other) {
-      block->list.push_back(append);
-    } else {
-      for (auto* item : other->list) {
-        block->list.push_back(item);
-      }
-    }
-    block->finalize();
-    return block;
-  }
-
   // a helper for the common pattern of a sequence of two expressions. Similar to
   // blockify, but does *not* reuse a block if the first is one.
   Block* makeSequence(Expression* left, Expression* right) {

test/passes/remove-unused-brs.txt

@@ -5,6 +5,7 @@
  (type $3 (func (param i32 i32) (result i32)))
  (type $4 (func (param i32 i32)))
  (type $5 (func (param f32 i32 f32 i32 i32 f64 f32) (result i32)))
+ (type $6 (func (param i32) (result i64)))
  (memory $0 256 256)
  (func $b0-yes (type $0) (param $i1 i32)
   (block $topmost
@@ -539,11 +540,13 @@
    (block $out59
     (if
      (i32.const 0)
-     (block $block61
-      (drop
-       (i32.const 1)
+     (block
+      (block $block61
+       (drop
+        (i32.const 1)
+       )
+       (call $loops)
       )
-      (call $loops)
       (br $in58)
      )
      (nop)
@@ -584,11 +587,13 @@
    (block $out69
     (if
      (i32.const 0)
-     (block $block71
-      (drop
-       (i32.const 1)
+     (block
+      (block $block71
+       (drop
+        (i32.const 1)
+       )
+       (call $loops)
       )
-      (call $loops)
       (drop
        (i32.const 102)
       )
@@ -1011,4 +1016,23 @@
    (i32.const 1935947830)
   )
  )
+ (func $unexitable-loops-result (type $6) (param $0 i32) (result i64)
+  (loop $label$0
+   (loop $label$1
+    (br_if $label$0
+     (i32.load8_s
+      (i32.const 201460482)
+     )
+    )
+    (block
+     (block $label$3
+      (br_if $label$1
+       (get_local $0)
+      )
+     )
+     (br $label$1)
+    )
+   )
+  )
+ )
 )

test/passes/remove-unused-brs.wast

@@ -901,5 +901,23 @@
     (i32.const 1935947830)
    )
   )
+  (func $unexitable-loops-result (param $0 i32) (result i64)
+   (loop $label$0
+    (loop $label$1
+     (if
+      (i32.load8_s
+       (i32.const 201460482)
+      )
+      (br $label$0)
+      (block $label$3
+       (br_if $label$3
+        (get_local $0)
+       )
+      )
+     )
+     (br $label$1)
+    )
+   )
+  )
 )
 

test/passes/remove-unused-names_remove-unused-brs_vacuum.txt

@@ -22,6 +22,7 @@
  (type $FUNCSIG$vii (func (param i32 i32)))
  (type $FUNCSIG$iiii (func (param i32 i32 i32) (result i32)))
  (type $FUNCSIG$id (func (param f64) (result i32)))
+ (type $23 (func (param i32) (result i64)))
  (import "env" "DYNAMICTOP_PTR" (global $import$0 i32))
  (import "env" "tempDoublePtr" (global $import$1 i32))
  (import "env" "ABORT" (global $import$2 i32))
@@ -113,4 +114,21 @@
  (func $23 (type $0) (param $0 i32) (param $1 i32) (param $2 i32) (result i32)
   (unreachable)
  )
+ (func $unexitable-loops-result (type $23) (param $0 i32) (result i64)
+  (loop $label$0
+   (loop $label$1
+    (br_if $label$0
+     (i32.load8_s
+      (i32.const 201460482)
+     )
+    )
+    (block
+     (br_if $label$1
+      (get_local $0)
+     )
+     (br $label$1)
+    )
+   )
+  )
+ )
 )

test/passes/remove-unused-names_remove-unused-brs_vacuum.wast

@@ -127,5 +127,23 @@
   (func $23 (param i32) (param i32) (param i32) (result i32)
     (unreachable)
   )
+  (func $unexitable-loops-result (param $0 i32) (result i64)
+   (loop $label$0
+    (loop $label$1
+     (if
+      (i32.load8_s
+       (i32.const 201460482)
+      )
+      (br $label$0)
+      (block $label$3
+       (br_if $label$3
+        (get_local $0)
+       )
+      )
+     )
+     (br $label$1)
+    )
+   )
+  )
 )