Avoid emitting a block in the binary format when it has no name (#4912)

We already did this if the block was a child of a control flow structure, which is
the common case (see the new added comment around that code, which clarifies
why). This does the same for all other blocks. This is simple to do and a minor
optimization, but the main benefit from this is just to make our handling of blocks
uniform: after this, we never emit a block with no name. This will make 1a non-
nullable locals easier to handle (since they will be able to assume that property;
and not emitting such blocks avoids some work to handle non-nullable locals
in them).

Author: kripken

src/wasm-stack.h

@@ -200,10 +200,21 @@ template<typename SubType> void BinaryenIRWriter<SubType>::write() {
   emitFunctionEnd();
 }
 
-// emits a node, but if it is a block with no name, emit a list of its contents
+// Emits a node in a position that can contain a list of contents, like an if
+// arm. This will emit the node, but if it is a block with no name, just emit
+// its contents. This is ok to do because a list of contents is ok in the wasm
+// binary format in such positions anyhow. When we read such code in Binaryen
+// we will end up creating a block for it (note that while doing so we create a
+// block without a name, since nothing branches to it, which makes it easy to
+// handle in optimization passes and when writing the binary out again).
 template<typename SubType>
 void BinaryenIRWriter<SubType>::visitPossibleBlockContents(Expression* curr) {
   auto* block = curr->dynCast<Block>();
+  // Even if the block has a name, check if the name is necessary (if it has no
+  // uses, it is equivalent to not having one). Scanning the children of the
+  // block means that this takes quadratic time, but it will be N^2 for a fairly
+  // small N since the number of nested non-block control flow structures tends
+  // to be very reasonable.
   if (!block || BranchUtils::BranchSeeker::has(block, block->name)) {
     visit(curr);
     return;
@@ -264,6 +275,26 @@ void BinaryenIRWriter<SubType>::visitBlock(Block* curr) {
     }
   };
 
+  // A block with no name never needs to be emitted: we can just emit its
+  // contents. In some cases that will end up as "stacky" code, which is valid
+  // in wasm but not in Binaryen IR. This is similar to what we do in
+  // visitPossibleBlockContents(), and like there, when we reload such a binary
+  // we'll end up creating a block for it then.
+  //
+  // Note that in visitPossibleBlockContents() we also optimize the case of a
+  // block with a name but the name actually has no uses - that handles more
+  // cases, but it requires more work. It is reasonable to do it in
+  // visitPossibleBlockContents() which handles the common cases of blocks that
+  // are children of control flow structures (like an if arm); doing it here
+  // would affect every block, including highly-nested block stacks, which would
+  // end up as quadratic time. In optimized code the name will not exist if it's
+  // not used anyhow, so a minor optimization for the unoptimized case that
+  // leads to potential quadratic behavior is not worth it here.
+  if (!curr->name.is()) {
+    visitChildren(curr, 0);
+    return;
+  }
+
   auto afterChildren = [this](Block* curr) {
     emitScopeEnd(curr);
     if (curr->type == Type::unreachable) {

test/example/c-api-unused-mem.txt

@@ -36,7 +36,7 @@
   (call $main)
  )
 )
-145
+133
 (module
  (type $none_=>_none (func))
  (memory $0 1024 1024)
@@ -53,20 +53,13 @@
      (i32.const 0)
     )
    )
-   (block $label$2
-    (br $label$1)
-   )
+   (br $label$1)
   )
-  (block $label$3
-   (block $label$4
-    (i32.store
-     (i32.const 0)
-     (local.get $0)
-    )
-    (return)
-   )
-   (unreachable)
+  (i32.store
+   (i32.const 0)
+   (local.get $0)
   )
+  (return)
  )
  (func $__wasm_start
   (i32.store