Bug 1595825 - Text-to-binary branch instructions with multiple values. r=wingo
authorLars T Hansen <lhansen@mozilla.com>
Mon, 18 Nov 2019 12:05:21 +0000
changeset 502406 71bcb50a0a8c3298af53e98cd741700f5c3392d2
parent 502405 7d5484728cf0e34482efccdcadb16482ee05f8f0
child 502407 9e0759e69df2c9b51bb7e2f26960f1f44d0a4274
push id36815
push usershindli@mozilla.com
push dateMon, 18 Nov 2019 16:17:29 +0000
treeherdermozilla-central@edad97097819 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerswingo
bugs1595825
milestone72.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1595825 - Text-to-binary branch instructions with multiple values. r=wingo Allow br, br_if, and br_table to pass multiple results to their target blocks. This turned out to be easy: there was what appeared to be arbitrary complexity in handling both br_if and br_table, this is now removed. Differential Revision: https://phabricator.services.mozilla.com/D53027
js/src/jit-test/tests/wasm/multi-value/block-validate.js
js/src/wasm/WasmAST.h
js/src/wasm/WasmTextToBinary.cpp
--- a/js/src/jit-test/tests/wasm/multi-value/block-validate.js
+++ b/js/src/jit-test/tests/wasm/multi-value/block-validate.js
@@ -186,8 +186,81 @@ wasmValidateText(`
         }
         return s;
        })()}
     (func (result i32)
       (i32.add
         (block (result i32 i32)
           (i32.const 32)
           (i32.const 10)))))`);
+
+wasmValidateText(`
+  (module
+    (func (result i32)
+      (block $B (result i32 i32)
+        (br $B (i32.const 1) (i32.const 2))
+        (i32.const 0x1337))
+      (drop)))`);
+
+wasmValidateText(`
+  (module
+    (func (result i32)
+      (block $B (result i32 i32)
+        (i32.const 1)
+        (br $B (i32.const 2))
+        (i32.const 0x1337))
+      (drop)))`);
+
+wasmValidateText(`
+  (module
+    (func (result i32)
+      (block $B (result i32 i32)
+        (i32.const 1)
+        (i32.const 2)
+        (br $B)
+        (i32.const 0x1337))
+      (drop)))`);
+
+wasmValidateText(`
+  (module
+    (func (param $cond i32) (result i32)
+      (block $B (result i32 i32)
+        (br_if $B (i32.const 1) (i32.const 2) (local.get $cond)))
+      (drop)))`);
+
+wasmValidateText(`
+  (module
+    (func (param $cond i32) (result i32)
+      (block $B (result i32 i32)
+        (i32.const 1)
+        (br_if $B (i32.const 2) (local.get $cond)))
+      (drop)))`);
+
+wasmValidateText(`
+  (module
+    (func (param $cond i32) (result i32)
+      (block $B (result i32 i32)
+        (i32.const 1)
+        (i32.const 2)
+        (br_if $B (local.get $cond)))
+      (drop)))`);
+
+wasmValidateText(`
+  (module
+    (func (param $cond i32) (result i32)
+      (block $B (result i32 i32)
+        (i32.const 1)
+        (i32.const 2)
+        (local.get $cond)
+        (br_if $B))
+      (drop)))`);
+
+wasmValidateText(`
+  (module
+    (func (param $index i32) (result i32)
+      (block $OUT (result i32)
+        (block $B1 (result i32 i32)
+          (block $B2 (result i32 i32)
+            (block $B3 (result i32 i32)
+              (br_table $B1 $B2 $B3 (i32.const 1) (i32.const 2) (local.get $index)))
+            (br $OUT (i32.add)))
+          (br $OUT (i32.sub)))
+        (br $OUT (i32.mul)))))`);
--- a/js/src/wasm/WasmAST.h
+++ b/js/src/wasm/WasmAST.h
@@ -544,32 +544,27 @@ class AstBlock : public AstExpr {
   Op op() const { return op_; }
   AstName name() const { return name_; }
   const AstExprVector& exprs() const { return exprs_; }
   AstBlockType& type() { return type_; }
 };
 
 class AstBranch : public AstExpr {
   Op op_;
-  AstExpr* cond_;
   AstRef target_;
-  AstExpr* value_;
+  AstExprVector values_;  // Includes the condition, for br_if
 
  public:
   static const AstExprKind Kind = AstExprKind::Branch;
-  explicit AstBranch(Op op, AstExpr* cond, AstRef target, AstExpr* value)
-      : AstExpr(Kind), op_(op), cond_(cond), target_(target), value_(value) {}
+  explicit AstBranch(Op op, AstRef target, AstExprVector&& values)
+      : AstExpr(Kind), op_(op), target_(target), values_(std::move(values)) {}
 
   Op op() const { return op_; }
   AstRef& target() { return target_; }
-  AstExpr& cond() const {
-    MOZ_ASSERT(cond_);
-    return *cond_;
-  }
-  AstExpr* maybeValue() const { return value_; }
+  AstExprVector& values() { return values_; }
 };
 
 class AstCall : public AstExpr {
   Op op_;
   AstRef func_;
   AstExprVector args_;
 
  public:
@@ -1056,34 +1051,31 @@ class AstMemoryGrow final : public AstEx
  public:
   static const AstExprKind Kind = AstExprKind::MemoryGrow;
   explicit AstMemoryGrow(AstExpr* operand) : AstExpr(Kind), operand_(operand) {}
 
   AstExpr* operand() const { return operand_; }
 };
 
 class AstBranchTable : public AstExpr {
-  AstExpr& index_;
   AstRef default_;
   AstRefVector table_;
-  AstExpr* value_;
+  AstExprVector values_;
 
  public:
   static const AstExprKind Kind = AstExprKind::BranchTable;
-  explicit AstBranchTable(AstExpr& index, AstRef def, AstRefVector&& table,
-                          AstExpr* maybeValue)
+  explicit AstBranchTable(AstRef def, AstRefVector&& table,
+                          AstExprVector&& values)
       : AstExpr(Kind),
-        index_(index),
         default_(def),
         table_(std::move(table)),
-        value_(maybeValue) {}
-  AstExpr& index() const { return index_; }
+        values_(std::move(values)) {}
   AstRef& def() { return default_; }
   AstRefVector& table() { return table_; }
-  AstExpr* maybeValue() { return value_; }
+  AstExprVector& values() { return values_; }
 };
 
 class AstFunc : public AstNode {
   AstName name_;
   AstRef funcType_;
   AstValTypeVector vars_;
   AstNameVector localNames_;
   AstExprVector body_;
--- a/js/src/wasm/WasmTextToBinary.cpp
+++ b/js/src/wasm/WasmTextToBinary.cpp
@@ -2604,81 +2604,57 @@ static AstBlock* ParseBlock(WasmParseCon
     }
     result =
         new (c.lifo) AstBlock(Op::Block, type, otherName, std::move(exprs));
   }
 
   return result;
 }
 
-static AstBranch* ParseBranch(WasmParseContext& c, Op op, bool inParens) {
-  MOZ_ASSERT(op == Op::Br || op == Op::BrIf);
-
-  AstRef target;
-  if (!c.ts.matchRef(&target, c.error)) {
-    return nullptr;
-  }
-
-  AstExpr* value = nullptr;
-  if (inParens) {
-    if (c.ts.getIf(WasmToken::OpenParen)) {
-      value = ParseExprInsideParens(c);
-      if (!value) {
-        return nullptr;
-      }
-      if (!c.ts.match(WasmToken::CloseParen, c.error)) {
-        return nullptr;
-      }
-    }
-  }
-
-  AstExpr* cond = nullptr;
-  if (op == Op::BrIf) {
-    if (inParens && c.ts.getIf(WasmToken::OpenParen)) {
-      cond = ParseExprInsideParens(c);
-      if (!cond) {
-        return nullptr;
-      }
-      if (!c.ts.match(WasmToken::CloseParen, c.error)) {
-        return nullptr;
-      }
-    } else {
-      cond = new (c.lifo) AstPop();
-      if (!cond) {
-        return nullptr;
-      }
-    }
-  }
-
-  return new (c.lifo) AstBranch(op, cond, target, value);
-}
-
-static bool ParseArgs(WasmParseContext& c, AstExprVector* args) {
+static bool ParseExprs(WasmParseContext& c, AstExprVector* exprs) {
   while (c.ts.getIf(WasmToken::OpenParen)) {
-    AstExpr* arg = ParseExprInsideParens(c);
-    if (!arg || !args->append(arg)) {
+    AstExpr* expr = ParseExprInsideParens(c);
+    if (!expr || !exprs->append(expr)) {
       return false;
     }
     if (!c.ts.match(WasmToken::CloseParen, c.error)) {
       return false;
     }
   }
 
   return true;
 }
 
+static AstBranch* ParseBranch(WasmParseContext& c, Op op, bool inParens) {
+  MOZ_ASSERT(op == Op::Br || op == Op::BrIf);
+
+  AstRef target;
+  if (!c.ts.matchRef(&target, c.error)) {
+    return nullptr;
+  }
+
+  AstExprVector values(c.lifo);
+  if (inParens) {
+    if (!ParseExprs(c, &values)) {
+      return nullptr;
+    }
+  }
+
+  return new (c.lifo) AstBranch(op, target, std::move(values));
+}
+
 static AstCall* ParseCall(WasmParseContext& c, bool inParens) {
   AstRef func;
   if (!c.ts.matchRef(&func, c.error)) {
     return nullptr;
   }
 
   AstExprVector args(c.lifo);
   if (inParens) {
-    if (!ParseArgs(c, &args)) {
+    if (!ParseExprs(c, &args)) {
       return nullptr;
     }
   }
 
   return new (c.lifo) AstCall(Op::Call, func, std::move(args));
 }
 
 static AstCallIndirect* ParseCallIndirect(WasmParseContext& c, bool inParens) {
@@ -2698,17 +2674,17 @@ static AstCallIndirect* ParseCallIndirec
     funcType = secondRef;
   } else {
     funcType = firstRef;
   }
 
   AstExprVector args(c.lifo);
   AstExpr* index;
   if (inParens) {
-    if (!ParseArgs(c, &args)) {
+    if (!ParseExprs(c, &args)) {
       return nullptr;
     }
 
     if (args.empty()) {
       index = new (c.lifo) AstPop();
     } else {
       index = args.popCopy();
     }
@@ -3726,36 +3702,24 @@ static AstBranchTable* ParseBranchTable(
 
   if (table.empty()) {
     c.ts.generateError(c.ts.get(), c.error);
     return nullptr;
   }
 
   AstRef def = table.popCopy();
 
-  AstExpr* index = ParseExpr(c, inParens);
-  if (!index) {
-    return nullptr;
-  }
-
-  AstExpr* value = nullptr;
+  AstExprVector values(c.lifo);
   if (inParens) {
-    if (c.ts.getIf(WasmToken::OpenParen)) {
-      value = index;
-      index = ParseExprInsideParens(c);
-      if (!index) {
-        return nullptr;
-      }
-      if (!c.ts.match(WasmToken::CloseParen, c.error)) {
-        return nullptr;
-      }
-    }
-  }
-
-  return new (c.lifo) AstBranchTable(*index, def, std::move(table), value);
+    if (!ParseExprs(c, &values)) {
+      return nullptr;
+    }
+  }
+
+  return new (c.lifo) AstBranchTable(def, std::move(table), std::move(values));
 }
 
 static AstMemoryGrow* ParseMemoryGrow(WasmParseContext& c, bool inParens) {
   AstExpr* operand = ParseExpr(c, inParens);
   if (!operand) {
     return nullptr;
   }
 
@@ -3986,17 +3950,17 @@ static AstTableSize* ParseTableSize(Wasm
 static AstExpr* ParseStructNew(WasmParseContext& c, bool inParens) {
   AstRef typeDef;
   if (!c.ts.matchRef(&typeDef, c.error)) {
     return nullptr;
   }
 
   AstExprVector args(c.lifo);
   if (inParens) {
-    if (!ParseArgs(c, &args)) {
+    if (!ParseExprs(c, &args)) {
       return nullptr;
     }
   }
 
   // An AstRef cast to AstValType turns into a Ref type, which is exactly what
   // we need here.
 
   return new (c.lifo) AstStructNew(typeDef, std::move(args));
@@ -5613,55 +5577,39 @@ static bool ResolveSelect(Resolver& r, A
          ResolveExpr(r, *b.op2());
 }
 
 static bool ResolveBranch(Resolver& r, AstBranch& br) {
   if (!r.resolveBranchTarget(br.target())) {
     return false;
   }
 
-  if (br.maybeValue() && !ResolveExpr(r, *br.maybeValue())) {
-    return false;
-  }
-
-  if (br.op() == Op::BrIf) {
-    if (!ResolveExpr(r, br.cond())) {
-      return false;
-    }
-  }
-
-  return true;
-}
-
-static bool ResolveArgs(Resolver& r, const AstExprVector& args) {
-  for (AstExpr* arg : args) {
-    if (!ResolveExpr(r, *arg)) {
-      return false;
-    }
+  if (!ResolveExprList(r, br.values())) {
+    return false;
   }
 
   return true;
 }
 
 static bool ResolveCall(Resolver& r, AstCall& c) {
   MOZ_ASSERT(c.op() == Op::Call);
 
-  if (!ResolveArgs(r, c.args())) {
+  if (!ResolveExprList(r, c.args())) {
     return false;
   }
 
   if (!r.resolveFunction(c.func())) {
     return false;
   }
 
   return true;
 }
 
 static bool ResolveCallIndirect(Resolver& r, AstCallIndirect& c) {
-  if (!ResolveArgs(r, c.args())) {
+  if (!ResolveExprList(r, c.args())) {
     return false;
   }
 
   if (!ResolveExpr(r, *c.index())) {
     return false;
   }
 
   if (!r.resolveSignature(c.funcType())) {
@@ -5793,21 +5741,17 @@ static bool ResolveBranchTable(Resolver&
   }
 
   for (AstRef& elem : bt.table()) {
     if (!r.resolveBranchTarget(elem)) {
       return false;
     }
   }
 
-  if (bt.maybeValue() && !ResolveExpr(r, *bt.maybeValue())) {
-    return false;
-  }
-
-  return ResolveExpr(r, bt.index());
+  return ResolveExprList(r, bt.values());
 }
 
 static bool ResolveAtomicCmpXchg(Resolver& r, AstAtomicCmpXchg& s) {
   return ResolveLoadStoreAddress(r, s.address()) &&
          ResolveExpr(r, s.expected()) && ResolveExpr(r, s.replacement());
 }
 
 static bool ResolveAtomicLoad(Resolver& r, AstAtomicLoad& l) {
@@ -5872,17 +5816,17 @@ static bool ResolveTableSet(Resolver& r,
 
 static bool ResolveTableSize(Resolver& r, AstTableSize& s) {
   return r.resolveTable(s.targetTable());
 }
 #endif
 
 #ifdef ENABLE_WASM_GC
 static bool ResolveStructNew(Resolver& r, AstStructNew& s) {
-  if (!ResolveArgs(r, s.fieldValues())) {
+  if (!ResolveExprList(r, s.fieldValues())) {
     return false;
   }
 
   if (!r.resolveType(s.structType())) {
     return false;
   }
 
   return true;
@@ -6323,26 +6267,18 @@ static bool EncodeBlock(Encoder& e, AstB
   }
 
   return true;
 }
 
 static bool EncodeBranch(Encoder& e, AstBranch& br) {
   MOZ_ASSERT(br.op() == Op::Br || br.op() == Op::BrIf);
 
-  if (br.maybeValue()) {
-    if (!EncodeExpr(e, *br.maybeValue())) {
-      return false;
-    }
-  }
-
-  if (br.op() == Op::BrIf) {
-    if (!EncodeExpr(e, br.cond())) {
-      return false;
-    }
+  if (!EncodeExprList(e, br.values())) {
+    return false;
   }
 
   if (!e.writeOp(br.op())) {
     return false;
   }
 
   if (!e.writeVarU32(br.target().index())) {
     return false;
@@ -6350,44 +6286,34 @@ static bool EncodeBranch(Encoder& e, Ast
 
   return true;
 }
 
 static bool EncodeFirst(Encoder& e, AstFirst& f) {
   return EncodeExprList(e, f.exprs());
 }
 
-static bool EncodeArgs(Encoder& e, const AstExprVector& args) {
-  for (AstExpr* arg : args) {
-    if (!EncodeExpr(e, *arg)) {
-      return false;
-    }
-  }
-
-  return true;
-}
-
 static bool EncodeCall(Encoder& e, AstCall& c) {
-  if (!EncodeArgs(e, c.args())) {
+  if (!EncodeExprList(e, c.args())) {
     return false;
   }
 
   if (!e.writeOp(c.op())) {
     return false;
   }
 
   if (!e.writeVarU32(c.func().index())) {
     return false;
   }
 
   return true;
 }
 
 static bool EncodeCallIndirect(Encoder& e, AstCallIndirect& c) {
-  if (!EncodeArgs(e, c.args())) {
+  if (!EncodeExprList(e, c.args())) {
     return false;
   }
 
   if (!EncodeExpr(e, *c.index())) {
     return false;
   }
 
   if (!e.writeOp(Op::CallIndirect)) {
@@ -6545,23 +6471,17 @@ static bool EncodeReturn(Encoder& e, Ast
   if (!e.writeOp(Op::Return)) {
     return false;
   }
 
   return true;
 }
 
 static bool EncodeBranchTable(Encoder& e, AstBranchTable& bt) {
-  if (bt.maybeValue()) {
-    if (!EncodeExpr(e, *bt.maybeValue())) {
-      return false;
-    }
-  }
-
-  if (!EncodeExpr(e, bt.index())) {
+  if (!EncodeExprList(e, bt.values())) {
     return false;
   }
 
   if (!e.writeOp(Op::BrTable)) {
     return false;
   }
 
   if (!e.writeVarU32(bt.table().length())) {
@@ -6695,17 +6615,17 @@ static bool EncodeTableSet(Encoder& e, A
 
 static bool EncodeTableSize(Encoder& e, AstTableSize& s) {
   return e.writeOp(MiscOp::TableSize) && e.writeVarU32(s.targetTable().index());
 }
 #endif
 
 #ifdef ENABLE_WASM_GC
 static bool EncodeStructNew(Encoder& e, AstStructNew& s) {
-  if (!EncodeArgs(e, s.fieldValues())) {
+  if (!EncodeExprList(e, s.fieldValues())) {
     return false;
   }
 
   if (!e.writeOp(MiscOp::StructNew)) {
     return false;
   }
 
   if (!e.writeVarU32(s.structType().index())) {