Return to more structured type rules for block and if

auto_update_tests.py

@@ -5,8 +5,10 @@
 from scripts.test.support import run_command, split_wast
 from scripts.test.shared import (
     ASM2WASM, MOZJS, S2WASM, WASM_SHELL, WASM_OPT, WASM_AS, WASM_DIS,
-    WASM_CTOR_EVAL, WASM_MERGE, WASM_REDUCE,
+    WASM_CTOR_EVAL, WASM_MERGE, WASM_REDUCE, WASM2ASM,
     BINARYEN_INSTALL_DIR, has_shell_timeout)
+from scripts.test.wasm2asm import tests, spec_tests, extra_tests
+
 
 print '[ processing and updating testcases... ]\n'
 
@@ -261,6 +263,24 @@
     out = t + '.out'
     with open(out, 'w') as o: o.write(actual)
 
+print '\n[ checking wasm2asm ]\n'
+
+for wasm in tests + spec_tests + extra_tests:
+  if not wasm.endswith('.wast'):
+    continue
+
+  asm = os.path.basename(wasm).replace('.wast', '.2asm.js')
+  expected_file = os.path.join('test', asm)
+
+  if not os.path.exists(expected_file):
+    continue
+
+  print '..', wasm
+
+  cmd = WASM2ASM + [os.path.join('test', wasm)]
+  out = run_command(cmd)
+  with open(expected_file, 'w') as o: o.write(out)
+
 if has_shell_timeout():
   print '\n[ checking wasm-reduce ]\n'
 

src/ast/branch-utils.h

@@ -103,11 +103,11 @@ inline std::set<Name> getBranchTargets(Expression* ast) {
 // Finds if there are branches targeting a name. Note that since names are
 // unique in our IR, we just need to look for the name, and do not need
 // to analyze scoping.
-// By default we ignore untaken branches. You can set named to
-// note those as well, then any named branch is noted, even if untaken
+// By default we consider untaken branches (so any named use). You can unset named to
+// avoid that (and only note branches that are not obviously unreachable)
 struct BranchSeeker : public PostWalker<BranchSeeker> {
   Name target;
-  bool named = false;
+  bool named = true;
 
   Index found;
   WasmType valueType;
@@ -144,32 +144,32 @@ struct BranchSeeker : public PostWalker<BranchSeeker> {
     if (curr->default_ == target) noteFound(curr->value);
   }
 
-  static bool has(Expression* tree, Name target) {
+  static bool hasTaken(Expression* tree, Name target) {
     if (!target.is()) return false;
     BranchSeeker seeker(target);
+    seeker.named = false;
     seeker.walk(tree);
     return seeker.found > 0;
   }
 
-  static Index count(Expression* tree, Name target) {
+  static Index countTaken(Expression* tree, Name target) {
     if (!target.is()) return 0;
     BranchSeeker seeker(target);
+    seeker.named = false;
     seeker.walk(tree);
     return seeker.found;
   }
 
   static bool hasNamed(Expression* tree, Name target) {
     if (!target.is()) return false;
     BranchSeeker seeker(target);
-    seeker.named = true;
     seeker.walk(tree);
     return seeker.found > 0;
   }
 
   static Index countNamed(Expression* tree, Name target) {
     if (!target.is()) return 0;
     BranchSeeker seeker(target);
-    seeker.named = true;
     seeker.walk(tree);
     return seeker.found;
   }

src/ast/type-updating.h

@@ -132,13 +132,9 @@ struct TypeUpdater : public ExpressionStackWalker<TypeUpdater, UnifiedExpression
   // adds (or removes) breaks depending on break/switch contents
   void discoverBreaks(Expression* curr, int change) {
     if (auto* br = curr->dynCast<Break>()) {
-      if (BranchUtils::isBranchTaken(br)) {
-        noteBreakChange(br->name, change, br->value);
-      }
+      noteBreakChange(br->name, change, br->value);
     } else if (auto* sw = curr->dynCast<Switch>()) {
-      if (BranchUtils::isBranchTaken(sw)) {
-        applySwitchChanges(sw, change);
-      }
+      applySwitchChanges(sw, change);
     }
   }
 
@@ -200,34 +196,17 @@ struct TypeUpdater : public ExpressionStackWalker<TypeUpdater, UnifiedExpression
       auto* child = curr;
       curr = parents[child];
       if (!curr) return;
-      // if a child of a break/switch is now unreachable, the
-      // break may no longer be taken. note that if we get here,
-      // this is an actually new unreachable child of the
-      // node, so if there is just 1 such child, it is us, and
-      // we are newly unreachable
-      if (auto* br = curr->dynCast<Break>()) {
-        int unreachableChildren = 0;
-        if (br->value && br->value->type == unreachable) unreachableChildren++;
-        if (br->condition && br->condition->type == unreachable) unreachableChildren++;
-        if (unreachableChildren == 1) {
-          // the break is no longer taken
-          noteBreakChange(br->name, -1, br->value);
-        }
-      } else if (auto* sw = curr->dynCast<Switch>()) {
-        int unreachableChildren = 0;
-        if (sw->value && sw->value->type == unreachable) unreachableChildren++;
-        if (sw->condition->type == unreachable) unreachableChildren++;
-        if (unreachableChildren == 1) {
-          applySwitchChanges(sw, -1);
-        }
-      }
       // get ready to apply unreachability to this node
       if (curr->type == unreachable) {
         return; // already unreachable, stop here
       }
       // most nodes become unreachable if a child is unreachable,
-      // but exceptions exists
+      // but exceptions exist
       if (auto* block = curr->dynCast<Block>()) {
+        // if the block has a fallthrough, it can keep its type
+        if (isConcreteWasmType(block->list.back()->type)) {
+          return; // did not turn
+        }
         // if the block has breaks, it can keep its type
         if (!block->name.is() || blockInfos[block->name].numBreaks == 0) {
           curr->type = unreachable;
@@ -255,7 +234,7 @@ struct TypeUpdater : public ExpressionStackWalker<TypeUpdater, UnifiedExpression
       return; // nothing concrete to change to unreachable
     }
     if (curr->name.is() && blockInfos[curr->name].numBreaks > 0) {
-      return;// has a break, not unreachable
+      return; // has a break, not unreachable
     }
     // look for a fallthrough
     makeBlockUnreachableIfNoFallThrough(curr);
@@ -265,9 +244,13 @@ struct TypeUpdater : public ExpressionStackWalker<TypeUpdater, UnifiedExpression
     if (curr->type == unreachable) {
       return; // no change possible
     }
+    if (!curr->list.empty() &&
+        isConcreteWasmType(curr->list.back()->type)) {
+      return; // should keep type due to fallthrough, even if has an unreachable child
+    }
     for (auto* child : curr->list) {
       if (child->type == unreachable) {
-        // no fallthrough, this block is now unreachable
+        // no fallthrough, and an unreachable, => this block is now unreachable
         changeTypeTo(curr, unreachable);
         return;
       }

src/ast_utils.h

@@ -62,7 +62,7 @@ struct ExpressionAnalyzer {
     if (auto* br = curr->dynCast<Break>()) {
       if (!br->condition) return true;
     } else if (auto* block = curr->dynCast<Block>()) {
-      if (block->list.size() > 0 && obviouslyDoesNotFlowOut(block->list.back()) && !BranchUtils::BranchSeeker::has(block, block->name)) return true;
+      if (block->list.size() > 0 && obviouslyDoesNotFlowOut(block->list.back()) && !BranchUtils::BranchSeeker::hasTaken(block, block->name)) return true;
     }
     return false;
   }
@@ -101,49 +101,59 @@ struct ReFinalize : public WalkerPass<PostWalker<ReFinalize>> {
   std::map<Name, WasmType> breakValues;
 
   void visitBlock(Block *curr) {
+    if (curr->list.size() == 0) {
+      curr->type = none;
+      return;
+    }
     // do this quickly, without any validation
+    auto old = curr->type;
+    // last element determines type
+    curr->type = curr->list.back()->type;
+    // if concrete, it doesn't matter if we have an unreachable child, and we
+    // don't need to look at breaks
+    if (isConcreteWasmType(curr->type)) return;
+    // otherwise, we have no final fallthrough element to determine the type,
+    // could be determined by breaks
     if (curr->name.is()) {
       auto iter = breakValues.find(curr->name);
       if (iter != breakValues.end()) {
         // there is a break to here
-        curr->type = iter->second;
+        auto type = iter->second;
+        if (type == unreachable) {
+          // all we have are breaks with values of type unreachable, and no
+          // concrete fallthrough either. we must have had an existing type, then
+          curr->type = old;
+          assert(isConcreteWasmType(curr->type));
+        } else {
+          curr->type = type;
+        }
         return;
       }
     }
-    // nothing branches here
-    if (curr->list.size() > 0) {
-      // if we have an unreachable child, we are unreachable
-      // (we don't need to recurse into children, they can't
-      // break to us)
+    if (curr->type == unreachable) return;
+    // type is none, but we might be unreachable
+    if (curr->type == none) {
       for (auto* child : curr->list) {
         if (child->type == unreachable) {
           curr->type = unreachable;
-          return;
+          break;
         }
       }
-      // children are reachable, so last element determines type
-      curr->type = curr->list.back()->type;
-    } else {
-      curr->type = none;
     }
   }
   void visitIf(If *curr) { curr->finalize(); }
   void visitLoop(Loop *curr) { curr->finalize(); }
   void visitBreak(Break *curr) {
     curr->finalize();
-    if (BranchUtils::isBranchTaken(curr)) {
-      breakValues[curr->name] = getValueType(curr->value);
-    }
+    updateBreakValueType(curr->name, getValueType(curr->value));
   }
   void visitSwitch(Switch *curr) {
     curr->finalize();
-    if (BranchUtils::isBranchTaken(curr)) {
-      auto valueType = getValueType(curr->value);
-      for (auto target : curr->targets) {
-        breakValues[target] = valueType;
-      }
-      breakValues[curr->default_] = valueType;
+    auto valueType = getValueType(curr->value);
+    for (auto target : curr->targets) {
+      updateBreakValueType(target, valueType);
     }
+    updateBreakValueType(curr->default_, valueType);
   }
   void visitCall(Call *curr) { curr->finalize(); }
   void visitCallImport(CallImport *curr) { curr->finalize(); }
@@ -167,7 +177,13 @@ struct ReFinalize : public WalkerPass<PostWalker<ReFinalize>> {
   void visitUnreachable(Unreachable *curr) { curr->finalize(); }
 
   WasmType getValueType(Expression* value) {
-    return value && value->type != unreachable ? value->type : none;
+    return value ? value->type : none;
+  }
+
+  void updateBreakValueType(Name name, WasmType type) {
+    if (type != unreachable || breakValues.count(name) == 0) {
+      breakValues[name] = type;
+    }
   }
 };
 

src/passes/FlattenControlFlow.cpp

@@ -61,7 +61,7 @@
 #include <wasm.h>
 #include <pass.h>
 #include <wasm-builder.h>
-
+#include <ast_utils.h>
 
 namespace wasm {
 
@@ -461,6 +461,11 @@ struct FlattenControlFlow : public WalkerPass<PostWalker<FlattenControlFlow>> {
       splitter.note(operand);
     }
   }
+
+  void visitFunction(Function* curr) {
+    // removing breaks can alter types
+    ReFinalize().walkFunctionInModule(curr, getModule());
+  }
 };
 
 Pass *createFlattenControlFlowPass() {

src/passes/OptimizeInstructions.cpp

@@ -397,6 +397,14 @@ struct OptimizeInstructions : public WalkerPass<PostWalker<OptimizeInstructions,
 
   // Optimizations that don't yet fit in the pattern DSL, but could be eventually maybe
   Expression* handOptimize(Expression* curr) {
+    // if this contains dead code, don't bother trying to optimize it, the type
+    // might change (if might not be unreachable if just one arm is, for example).
+    // this optimization pass focuses on actually executing code. the only
+    // exceptions are control flow changes
+    if (curr->type == unreachable &&
+        !curr->is<Break>() && !curr->is<Switch>() && !curr->is<If>()) {
+      return nullptr;
+    }
     if (auto* binary = curr->dynCast<Binary>()) {
       if (Properties::isSymmetric(binary)) {
         // canonicalize a const to the second position

src/passes/RemoveUnusedBrs.cpp

@@ -394,7 +394,9 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
         if (list.size() == 1 && curr->name.is()) {
           // if this block has just one child, a sub-block, then jumps to the former are jumps to us, really
           if (auto* child = list[0]->dynCast<Block>()) {
-            if (child->name.is() && child->name != curr->name) {
+            // the two blocks must have the same type for us to update the branch, as otherwise
+            // one block may be unreachable and the other concrete, so one might lack a value
+            if (child->name.is() && child->name != curr->name && child->type == curr->type) {
               auto& breaks = breaksToBlock[child];
               for (auto* br : breaks) {
                 newNames[br] = curr->name;

src/passes/SimplifyLocals.cpp

@@ -372,8 +372,9 @@ struct SimplifyLocals : public WalkerPass<LinearExecutionWalker<SimplifyLocals>>
   // optimize set_locals from both sides of an if into a return value
   void optimizeIfReturn(If* iff, Expression** currp, Sinkables& ifTrue) {
     assert(iff->ifFalse);
-    // if this if already has a result, we can't do anything
-    if (isConcreteWasmType(iff->type)) return;
+    // if this if already has a result, or is unreachable code, we have
+    // nothing to do
+    if (iff->type != none) return;
     // We now have the sinkables from both sides of the if.
     Sinkables& ifFalse = sinkables;
     Index sharedIndex = -1;

src/wasm/wasm-validator.cpp

@@ -70,9 +70,7 @@ void WasmValidator::visitBlock(Block *curr) {
   if (curr->list.size() > 0) {
     auto backType = curr->list.back()->type;
     if (!isConcreteWasmType(curr->type)) {
-      if (isConcreteWasmType(backType)) {
-        shouldBeTrue(curr->type == unreachable, curr, "block with no value and a last element with a value must be unreachable");
-      }
+      shouldBeFalse(isConcreteWasmType(backType), curr, "if block is not returning a value, final element should not flow out a value");
     } else {
       if (isConcreteWasmType(backType)) {
         shouldBeEqual(curr->type, backType, curr, "block with value and last element with value must match types");
@@ -118,15 +116,19 @@ void WasmValidator::visitIf(If *curr) {
         shouldBeEqual(curr->ifFalse->type, unreachable, curr, "unreachable if-else must have unreachable false");
       }
     }
+    if (isConcreteWasmType(curr->ifTrue->type)) {
+      shouldBeEqual(curr->type, curr->ifTrue->type, curr, "if type must match concrete ifTrue");
+      shouldBeEqualOrFirstIsUnreachable(curr->ifFalse->type, curr->ifTrue->type, curr, "other arm must match concrete ifTrue");
+    }
+    if (isConcreteWasmType(curr->ifFalse->type)) {
+      shouldBeEqual(curr->type, curr->ifFalse->type, curr, "if type must match concrete ifFalse");
+      shouldBeEqualOrFirstIsUnreachable(curr->ifTrue->type, curr->ifFalse->type, curr, "other arm must match concrete ifFalse");
+    }
   }
 }
 
 void WasmValidator::noteBreak(Name name, Expression* value, Expression* curr) {
-  if (!BranchUtils::isBranchTaken(curr)) {
-    // if not actually taken, just note the name
-    namedBreakTargets.insert(name);
-    return;
-  }
+  namedBreakTargets.insert(name);
   WasmType valueType = none;
   Index arity = 0;
   if (value) {
@@ -548,7 +550,7 @@ void WasmValidator::visitFunction(Function *curr) {
   if (returnType != unreachable) {
     shouldBeEqual(curr->result, returnType, curr->body, "function result must match, if function has returns");
   }
-  if (!shouldBeTrue(namedBreakTargets.empty(), curr->body, "all named break targets must exist (even if not taken)") && !quiet) {
+  if (!shouldBeTrue(namedBreakTargets.empty(), curr->body, "all named break targets must exist") && !quiet) {
     std::cerr << "(on label " << *namedBreakTargets.begin() << ")\n";
   }
   returnType = unreachable;