[wasm] Binary 11: AST changes.

src/wasm/asm-wasm-builder.cc

Index: src/wasm/asm-wasm-builder.cc
diff --git a/src/wasm/asm-wasm-builder.cc b/src/wasm/asm-wasm-builder.cc
index 4c7410d7e004c612e809db7f3ccc06da5da3188e..df496dc1daab23ce26f31e1f3591bb3e55e3d2f6 100644
--- a/src/wasm/asm-wasm-builder.cc
+++ b/src/wasm/asm-wasm-builder.cc
@@ -30,6 +30,7 @@ namespace wasm {
     if (HasStackOverflow()) return; \
   } while (false)
 
+enum AsmScope { kModuleScope, kInitScope, kFuncScope, kExportScope };
 
 class AsmWasmBuilderImpl : public AstVisitor {
  public:
@@ -43,9 +44,7 @@ class AsmWasmBuilderImpl : public AstVisitor {
         global_variables_(HashMap::PointersMatch,
                           ZoneHashMap::kDefaultHashMapCapacity,
                           ZoneAllocationPolicy(zone)),
-        in_function_(false),
-        is_set_op_(false),
-        marking_exported(false),
+        scope_(kModuleScope),
         builder_(new (zone) WasmModuleBuilder(zone)),
         current_function_builder_(nullptr),
         literal_(literal),
@@ -55,7 +54,6 @@ class AsmWasmBuilderImpl : public AstVisitor {
         typer_(typer),
         cache_(TypeCache::Get()),
         breakable_blocks_(zone),
-        block_size_(0),
         init_function_index_(0),
         next_table_index_(0),
         function_tables_(HashMap::PointersMatch,
@@ -81,13 +79,13 @@ class AsmWasmBuilderImpl : public AstVisitor {
   void VisitVariableDeclaration(VariableDeclaration* decl) {}
 
   void VisitFunctionDeclaration(FunctionDeclaration* decl) {
-    DCHECK(!in_function_);
+    DCHECK_EQ(kModuleScope, scope_);
     DCHECK_NULL(current_function_builder_);
     uint16_t index = LookupOrInsertFunction(decl->proxy()->var());
     current_function_builder_ = builder_->FunctionAt(index);
-    in_function_ = true;
+    scope_ = kFuncScope;
     RECURSE(Visit(decl->fun()));
-    in_function_ = false;
+    scope_ = kModuleScope;
     current_function_builder_ = nullptr;
     local_variables_.Clear();
   }
@@ -115,12 +113,10 @@ class AsmWasmBuilderImpl : public AstVisitor {
         }
       }
     }
-    if (in_function_) {
+    if (scope_ == kFuncScope) {
       BlockVisitor visitor(this, stmt->AsBreakableStatement(), kExprBlock,
-                           false,
-                           static_cast<byte>(stmt->statements()->length()));
+                           false);
       RECURSE(VisitStatements(stmt->statements()));
-      DCHECK(block_size_ >= 0);
     } else {
       RECURSE(VisitStatements(stmt->statements()));
     }
@@ -128,25 +124,17 @@ class AsmWasmBuilderImpl : public AstVisitor {
 
   class BlockVisitor {
    private:
-    int prev_block_size_;
-    uint32_t index_;
     AsmWasmBuilderImpl* builder_;
 
    public:
     BlockVisitor(AsmWasmBuilderImpl* builder, BreakableStatement* stmt,
-                 WasmOpcode opcode, bool is_loop, int initial_block_size)
+                 WasmOpcode opcode, bool is_loop)
         : builder_(builder) {
       builder_->breakable_blocks_.push_back(std::make_pair(stmt, is_loop));
       builder_->current_function_builder_->Emit(opcode);
-      index_ =
-          builder_->current_function_builder_->EmitEditableVarIntImmediate();
-      prev_block_size_ = builder_->block_size_;
-      builder_->block_size_ = initial_block_size;
     }
     ~BlockVisitor() {
-      builder_->current_function_builder_->EditVarIntImmediate(
-          index_, builder_->block_size_);
-      builder_->block_size_ = prev_block_size_;
+      builder_->current_function_builder_->Emit(kExprEnd);
       builder_->breakable_blocks_.pop_back();
     }
   };
@@ -160,25 +148,21 @@ class AsmWasmBuilderImpl : public AstVisitor {
   void VisitEmptyParentheses(EmptyParentheses* paren) { UNREACHABLE(); }
 
   void VisitIfStatement(IfStatement* stmt) {
-    DCHECK(in_function_);
-    if (stmt->HasElseStatement()) {
-      current_function_builder_->Emit(kExprIfElse);
-    } else {
-      current_function_builder_->Emit(kExprIf);
-    }
+    DCHECK_EQ(kFuncScope, scope_);
     RECURSE(Visit(stmt->condition()));
+    current_function_builder_->Emit(kExprIf);
     if (stmt->HasThenStatement()) {
       RECURSE(Visit(stmt->then_statement()));
-    } else {
-      current_function_builder_->Emit(kExprNop);
     }
     if (stmt->HasElseStatement()) {
+      current_function_builder_->Emit(kExprElse);
       RECURSE(Visit(stmt->else_statement()));
     }
+    current_function_builder_->Emit(kExprEnd);
   }
 
   void VisitContinueStatement(ContinueStatement* stmt) {
-    DCHECK(in_function_);
+    DCHECK_EQ(kFuncScope, scope_);
     DCHECK_NOT_NULL(stmt->target());
     int i = static_cast<int>(breakable_blocks_.size()) - 1;
     int block_distance = 0;
@@ -194,12 +178,12 @@ class AsmWasmBuilderImpl : public AstVisitor {
       }
     }
     DCHECK(i >= 0);
-    current_function_builder_->EmitWithVarInt(kExprBr, block_distance);
     current_function_builder_->Emit(kExprNop);
+    current_function_builder_->EmitWithVarInt(kExprBr, block_distance);
   }
 
   void VisitBreakStatement(BreakStatement* stmt) {
-    DCHECK(in_function_);
+    DCHECK_EQ(kFuncScope, scope_);
     DCHECK_NOT_NULL(stmt->target());
     int i = static_cast<int>(breakable_blocks_.size()) - 1;
     int block_distance = 0;
@@ -217,57 +201,53 @@ class AsmWasmBuilderImpl : public AstVisitor {
       }
     }
     DCHECK(i >= 0);
-    current_function_builder_->EmitWithVarInt(kExprBr, block_distance);
     current_function_builder_->Emit(kExprNop);
+    current_function_builder_->EmitWithVarInt(kExprBr, block_distance);
   }
 
   void VisitReturnStatement(ReturnStatement* stmt) {
-    if (in_function_) {
+    if (scope_ == kModuleScope) {
+      scope_ = kExportScope;
+      RECURSE(Visit(stmt->expression()));
+      scope_ = kModuleScope;
+    } else if (scope_ == kFuncScope) {
+      RECURSE(Visit(stmt->expression()));
       current_function_builder_->Emit(kExprReturn);
     } else {
-      marking_exported = true;
-    }
-    RECURSE(Visit(stmt->expression()));
-    if (!in_function_) {
-      marking_exported = false;
+      UNREACHABLE();
     }
   }
 
   void VisitWithStatement(WithStatement* stmt) { UNREACHABLE(); }
 
-  void SetLocalTo(uint16_t index, int value) {
+  void SetLocalTo(uint16_t index, int32_t value) {
+    current_function_builder_->EmitI32Const(value);
     current_function_builder_->Emit(kExprSetLocal);
     AddLeb128(index, true);
-    // TODO(bradnelson): variable size
-    byte code[] = {WASM_I32V(value)};
-    current_function_builder_->EmitCode(code, sizeof(code));
-    block_size_++;
   }
 
   void CompileCase(CaseClause* clause, uint16_t fall_through,
                    VariableProxy* tag) {
     Literal* label = clause->label()->AsLiteral();
     DCHECK_NOT_NULL(label);
-    block_size_++;
-    current_function_builder_->Emit(kExprIf);
-    current_function_builder_->Emit(kExprI32Ior);
-    current_function_builder_->Emit(kExprI32Eq);
     VisitVariableProxy(tag);
     VisitLiteral(label);
+    current_function_builder_->Emit(kExprI32Eq);
     current_function_builder_->Emit(kExprGetLocal);
     AddLeb128(fall_through, true);
-    BlockVisitor visitor(this, nullptr, kExprBlock, false, 0);
+    current_function_builder_->Emit(kExprI32Ior);
+    current_function_builder_->Emit(kExprIf);
+    BlockVisitor visitor(this, nullptr, kExprBlock, false);
     SetLocalTo(fall_through, 1);
     ZoneList<Statement*>* stmts = clause->statements();
-    block_size_ += stmts->length();
     RECURSE(VisitStatements(stmts));
+    current_function_builder_->Emit(kExprEnd);
   }
 
   void VisitSwitchStatement(SwitchStatement* stmt) {
     VariableProxy* tag = stmt->tag()->AsVariableProxy();
     DCHECK_NOT_NULL(tag);
-    BlockVisitor visitor(this, stmt->AsBreakableStatement(), kExprBlock, false,
-                         0);
+    BlockVisitor visitor(this, stmt->AsBreakableStatement(), kExprBlock, false);
     uint16_t fall_through = current_function_builder_->AddLocal(kAstI32);
     SetLocalTo(fall_through, 0);
 
@@ -278,7 +258,6 @@ class AsmWasmBuilderImpl : public AstVisitor {
         CompileCase(clause, fall_through, tag);
       } else {
         ZoneList<Statement*>* stmts = clause->statements();
-        block_size_ += stmts->length();
         RECURSE(VisitStatements(stmts));
       }
     }
@@ -287,53 +266,47 @@ class AsmWasmBuilderImpl : public AstVisitor {
   void VisitCaseClause(CaseClause* clause) { UNREACHABLE(); }
 
   void VisitDoWhileStatement(DoWhileStatement* stmt) {
-    DCHECK(in_function_);
-    BlockVisitor visitor(this, stmt->AsBreakableStatement(), kExprLoop, true,
-                         2);
+    DCHECK_EQ(kFuncScope, scope_);
+    BlockVisitor visitor(this, stmt->AsBreakableStatement(), kExprLoop, true);
     RECURSE(Visit(stmt->body()));
-    current_function_builder_->Emit(kExprIf);
     RECURSE(Visit(stmt->cond()));
-    current_function_builder_->EmitWithVarInt(kExprBr, 0);
-    current_function_builder_->Emit(kExprNop);
+    current_function_builder_->Emit(kExprIf);
+    current_function_builder_->EmitWithU8(kExprBr, 0);
+    current_function_builder_->Emit(kExprEnd);
   }
 
   void VisitWhileStatement(WhileStatement* stmt) {
-    DCHECK(in_function_);
-    BlockVisitor visitor(this, stmt->AsBreakableStatement(), kExprLoop, true,
-                         1);
-    current_function_builder_->Emit(kExprIf);
+    DCHECK_EQ(kFuncScope, scope_);
+    BlockVisitor visitor(this, stmt->AsBreakableStatement(), kExprLoop, true);
     RECURSE(Visit(stmt->cond()));
-    current_function_builder_->EmitWithVarInt(kExprBr, 0);
+    current_function_builder_->Emit(kExprIf);
     RECURSE(Visit(stmt->body()));
+    current_function_builder_->EmitWithU8(kExprBr, 0);
+    current_function_builder_->Emit(kExprEnd);
   }
 
   void VisitForStatement(ForStatement* stmt) {
-    DCHECK(in_function_);
+    DCHECK_EQ(kFuncScope, scope_);
     if (stmt->init() != nullptr) {
-      block_size_++;
       RECURSE(Visit(stmt->init()));
     }
-    BlockVisitor visitor(this, stmt->AsBreakableStatement(), kExprLoop, true,
-                         0);
+    BlockVisitor visitor(this, stmt->AsBreakableStatement(), kExprLoop, true);
     if (stmt->cond() != nullptr) {
-      block_size_++;
-      current_function_builder_->Emit(kExprIf);
-      current_function_builder_->Emit(kExprI32Eqz);
       RECURSE(Visit(stmt->cond()));
-      current_function_builder_->EmitWithVarInt(kExprBr, 1);
+      current_function_builder_->Emit(kExprI32Eqz);
+      current_function_builder_->Emit(kExprIf);
       current_function_builder_->Emit(kExprNop);
+      current_function_builder_->EmitWithU8(kExprBr, 1);
+      current_function_builder_->Emit(kExprEnd);
     }
     if (stmt->body() != nullptr) {
-      block_size_++;
       RECURSE(Visit(stmt->body()));
     }
     if (stmt->next() != nullptr) {
-      block_size_++;
       RECURSE(Visit(stmt->next()));
     }
-    block_size_++;
-    current_function_builder_->EmitWithVarInt(kExprBr, 0);
     current_function_builder_->Emit(kExprNop);
+    current_function_builder_->EmitWithU8(kExprBr, 0);
   }
 
   void VisitForInStatement(ForInStatement* stmt) { UNREACHABLE(); }
@@ -348,7 +321,7 @@ class AsmWasmBuilderImpl : public AstVisitor {
 
   void VisitFunctionLiteral(FunctionLiteral* expr) {
     Scope* scope = expr->scope();
-    if (in_function_) {
+    if (scope_ == kFuncScope) {
       if (expr->bounds().lower->IsFunction()) {
         FunctionType* func_type = expr->bounds().lower->AsFunction();
         LocalType return_type = TypeFrom(func_type->Result());
@@ -371,11 +344,13 @@ class AsmWasmBuilderImpl : public AstVisitor {
   }
 
   void VisitConditional(Conditional* expr) {
-    DCHECK(in_function_);
-    current_function_builder_->Emit(kExprIfElse);
+    DCHECK_EQ(kFuncScope, scope_);
     RECURSE(Visit(expr->condition()));
+    current_function_builder_->Emit(kExprIf);
     RECURSE(Visit(expr->then_expression()));
+    current_function_builder_->Emit(kExprElse);
     RECURSE(Visit(expr->else_expression()));
+    current_function_builder_->Emit(kExprEnd);
   }
 
   bool VisitStdlibConstant(Variable* var) {
@@ -431,45 +406,31 @@ class AsmWasmBuilderImpl : public AstVisitor {
   }
 
   void VisitVariableProxy(VariableProxy* expr) {
-    if (in_function_) {
+    if (scope_ == kFuncScope || scope_ == kInitScope) {
       Variable* var = expr->var();
-      if (is_set_op_) {
-        if (var->IsContextSlot()) {
-          current_function_builder_->Emit(kExprStoreGlobal);
-        } else {
-          current_function_builder_->Emit(kExprSetLocal);
-        }
-        is_set_op_ = false;
-      } else {
-        if (VisitStdlibConstant(var)) {
-          return;
-        }
-        if (var->IsContextSlot()) {
-          current_function_builder_->Emit(kExprLoadGlobal);
-        } else {
-          current_function_builder_->Emit(kExprGetLocal);
-        }
+      if (VisitStdlibConstant(var)) {
+        return;
       }
       LocalType var_type = TypeOf(expr);
       DCHECK_NE(kAstStmt, var_type);
       if (var->IsContextSlot()) {
+        current_function_builder_->Emit(kExprLoadGlobal);
         AddLeb128(LookupOrInsertGlobal(var, var_type), false);
       } else {
+        current_function_builder_->Emit(kExprGetLocal);
         AddLeb128(LookupOrInsertLocal(var, var_type), true);
       }
     }
   }
 
   void VisitLiteral(Literal* expr) {
-    if (in_function_) {
+    if (scope_ == kFuncScope || scope_ == kInitScope) {
       if (expr->raw_value()->IsNumber()) {
         LocalType type = TypeOf(expr);
         switch (type) {
           case kAstI32: {
-            int val = static_cast<int>(expr->raw_value()->AsNumber());
-            // TODO(bradnelson): variable size
-            byte code[] = {WASM_I32V(val)};
-            current_function_builder_->EmitCode(code, sizeof(code));
+            int32_t val = static_cast<int32_t>(expr->raw_value()->AsNumber());
+            current_function_builder_->EmitI32Const(val);
             break;
           }
           case kAstF32: {
@@ -497,7 +458,7 @@ class AsmWasmBuilderImpl : public AstVisitor {
     ZoneList<ObjectLiteralProperty*>* props = expr->properties();
     for (int i = 0; i < props->length(); ++i) {
       ObjectLiteralProperty* prop = props->at(i);
-      DCHECK(marking_exported);
+      DCHECK_EQ(kExportScope, scope_);
       VariableProxy* expr = prop->value()->AsVariableProxy();
       DCHECK_NOT_NULL(expr);
       Variable* var = expr->var();
@@ -518,11 +479,11 @@ class AsmWasmBuilderImpl : public AstVisitor {
 
   void LoadInitFunction() {
     current_function_builder_ = builder_->FunctionAt(init_function_index_);
-    in_function_ = true;
+    scope_ = kInitScope;
   }
 
   void UnLoadInitFunction() {
-    in_function_ = false;
+    scope_ = kModuleScope;
     current_function_builder_ = nullptr;
   }
 
@@ -625,33 +586,98 @@ class AsmWasmBuilderImpl : public AstVisitor {
     }
   };
 
-  void VisitAssignment(Assignment* expr) {
-    bool in_init = false;
-    if (!in_function_) {
-      BinaryOperation* binop = expr->value()->AsBinaryOperation();
-      if (binop != nullptr) {
+  void EmitAssignmentLhs(Expression* target, MachineType* mtype) {
+    // Match the left hand side of the assignment.
+    VariableProxy* target_var = target->AsVariableProxy();
+    if (target_var != nullptr) {
+      // Left hand side is a local or a global variable, no code on LHS.
+      return;
+    }
+
+    Property* target_prop = target->AsProperty();
+    if (target_prop != nullptr) {
+      // Left hand side is a property access, i.e. the asm.js heap.
+      VisitPropertyAndEmitIndex(target_prop, mtype);
+      return;
+    }
+
+    if (target_var == nullptr && target_prop == nullptr) {

[bradnelson, 2016/03/23 19:14:25]

You return above, why guard this, as you return in each of the above?

[titzer, 2016/03/23 19:53:05]

Because paranoia :-)

[bradn, 2016/03/23 19:58:50]

Seems like leaving the UNREACHABLE unguarded would be more paranoid?

+      UNREACHABLE();  // invalid assignment.
+    }
+  }
+
+  void EmitAssignmentRhs(Expression* target, Expression* value, bool* is_nop) {
+    BinaryOperation* binop = value->AsBinaryOperation();
+    if (binop != nullptr) {
+      if (scope_ == kInitScope) {
+        // Handle foreign variables in the initialization scope.
         Property* prop = binop->left()->AsProperty();
-        DCHECK_NOT_NULL(prop);
-        LoadInitFunction();
-        is_set_op_ = true;
-        RECURSE(Visit(expr->target()));
-        DCHECK(!is_set_op_);
         if (binop->op() == Token::MUL) {
           DCHECK(binop->right()->IsLiteral());
           DCHECK_EQ(1.0, binop->right()->AsLiteral()->raw_value()->AsNumber());
           DCHECK(binop->right()->AsLiteral()->raw_value()->ContainsDot());
           VisitForeignVariable(true, prop);
+          return;
         } else if (binop->op() == Token::BIT_OR) {
           DCHECK(binop->right()->IsLiteral());
           DCHECK_EQ(0.0, binop->right()->AsLiteral()->raw_value()->AsNumber());
           DCHECK(!binop->right()->AsLiteral()->raw_value()->ContainsDot());
           VisitForeignVariable(false, prop);
+          return;
         } else {
           UNREACHABLE();
         }
-        UnLoadInitFunction();
-        return;
       }
+      if (MatchBinaryOperation(binop) == kAsIs) {
+        VariableProxy* target_var = target->AsVariableProxy();
+        VariableProxy* effective_value_var = GetLeft(binop)->AsVariableProxy();
+        if (target_var != nullptr && effective_value_var != nullptr &&
+            target_var->var() == effective_value_var->var()) {
+          *is_nop = true;
+          return;
+        }
+      }
+    }
+    RECURSE(Visit(value));
+  }
+
+  void EmitAssignment(Assignment* expr, MachineType mtype) {
+    // Match the left hand side of the assignment.
+    VariableProxy* target_var = expr->target()->AsVariableProxy();
+    if (target_var != nullptr) {
+      // Left hand side is a local or a global variable.
+      Variable* var = target_var->var();
+      LocalType var_type = TypeOf(expr);
+      DCHECK_NE(kAstStmt, var_type);
+      if (var->IsContextSlot()) {
+        current_function_builder_->Emit(kExprStoreGlobal);
+        AddLeb128(LookupOrInsertGlobal(var, var_type), false);
+      } else {
+        current_function_builder_->Emit(kExprSetLocal);
+        AddLeb128(LookupOrInsertLocal(var, var_type), true);
+      }
+    }
+
+    Property* target_prop = expr->target()->AsProperty();
+    if (target_prop != nullptr) {
+      // Left hand side is a property access, i.e. the asm.js heap.
+      if (TypeOf(expr->value()) == kAstF64 && expr->target()->IsProperty() &&
+          expr->target()->AsProperty()->obj()->bounds().lower->Is(
+              cache_.kFloat32Array)) {
+        current_function_builder_->Emit(kExprF32ConvertF64);
+      }
+      current_function_builder_->EmitWithU8U8(
+          WasmOpcodes::LoadStoreOpcodeOf(mtype, true), 0, 0);
+    }
+
+    if (target_var == nullptr && target_prop == nullptr) {

[bradnelson, 2016/03/23 19:14:25]

Add returns instead of guard here?

[titzer, 2016/03/23 19:53:05]

Don't want to silently forget to generate code.

+      UNREACHABLE();  // invalid assignment.
+    }
+  }
+
+  void VisitAssignment(Assignment* expr) {
+    bool as_init = false;
+    if (scope_ == kModuleScope) {
       Property* prop = expr->value()->AsProperty();
       if (prop != nullptr) {
         VariableProxy* vp = prop->obj()->AsVariableProxy();
@@ -681,32 +707,18 @@ class AsmWasmBuilderImpl : public AstVisitor {
         // No init code to emit for CallNew nodes.
         return;
       }
-      in_init = true;
-      LoadInitFunction();
-    }
-    BinaryOperation* value_op = expr->value()->AsBinaryOperation();
-    if (value_op != nullptr && MatchBinaryOperation(value_op) == kAsIs) {
-      VariableProxy* target_var = expr->target()->AsVariableProxy();
-      VariableProxy* effective_value_var = GetLeft(value_op)->AsVariableProxy();
-      if (target_var != nullptr && effective_value_var != nullptr &&
-          target_var->var() == effective_value_var->var()) {
-        block_size_--;
-        return;
-      }
-    }
-    is_set_op_ = true;
-    RECURSE(Visit(expr->target()));
-    DCHECK(!is_set_op_);
-    // Assignment to heapf32 from float64 converts.
-    if (TypeOf(expr->value()) == kAstF64 && expr->target()->IsProperty() &&
-        expr->target()->AsProperty()->obj()->bounds().lower->Is(
-            cache_.kFloat32Array)) {
-      current_function_builder_->Emit(kExprF32ConvertF64);
+      as_init = true;
     }
-    RECURSE(Visit(expr->value()));
-    if (in_init) {
-      UnLoadInitFunction();
+
+    if (as_init) LoadInitFunction();
+    MachineType mtype;
+    bool is_nop = false;
+    EmitAssignmentLhs(expr->target(), &mtype);
+    EmitAssignmentRhs(expr->target(), expr->value(), &is_nop);
+    if (!is_nop) {
+      EmitAssignment(expr, mtype);
     }
+    if (as_init) UnLoadInitFunction();
   }
 
   void VisitYield(Yield* expr) { UNREACHABLE(); }
@@ -745,9 +757,7 @@ class AsmWasmBuilderImpl : public AstVisitor {
             Handle<Object> nvalue = maybe_nvalue.ToHandleChecked();
             if (nvalue->IsNumber()) {
               int32_t val = static_cast<int32_t>(nvalue->Number());
-              // TODO(bradnelson): variable size
-              byte code[] = {WASM_I32V(val)};
-              current_function_builder_->EmitCode(code, sizeof(code));
+              current_function_builder_->EmitI32Const(val);
               return;
             }
           }
@@ -763,46 +773,41 @@ class AsmWasmBuilderImpl : public AstVisitor {
     }
   }
 
-  void VisitProperty(Property* expr) {
+  void VisitPropertyAndEmitIndex(Property* expr, MachineType* mtype) {
     Expression* obj = expr->obj();
     DCHECK_EQ(obj->bounds().lower, obj->bounds().upper);
     Type* type = obj->bounds().lower;
-    MachineType mtype;
     int size;
     if (type->Is(cache_.kUint8Array)) {
-      mtype = MachineType::Uint8();
+      *mtype = MachineType::Uint8();
       size = 1;
     } else if (type->Is(cache_.kInt8Array)) {
-      mtype = MachineType::Int8();
+      *mtype = MachineType::Int8();
       size = 1;
     } else if (type->Is(cache_.kUint16Array)) {
-      mtype = MachineType::Uint16();
+      *mtype = MachineType::Uint16();
       size = 2;
     } else if (type->Is(cache_.kInt16Array)) {
-      mtype = MachineType::Int16();
+      *mtype = MachineType::Int16();
       size = 2;
     } else if (type->Is(cache_.kUint32Array)) {
-      mtype = MachineType::Uint32();
+      *mtype = MachineType::Uint32();
       size = 4;
     } else if (type->Is(cache_.kInt32Array)) {
-      mtype = MachineType::Int32();
+      *mtype = MachineType::Int32();
       size = 4;
     } else if (type->Is(cache_.kUint32Array)) {
-      mtype = MachineType::Uint32();
+      *mtype = MachineType::Uint32();
       size = 4;
     } else if (type->Is(cache_.kFloat32Array)) {
-      mtype = MachineType::Float32();
+      *mtype = MachineType::Float32();
       size = 4;
     } else if (type->Is(cache_.kFloat64Array)) {
-      mtype = MachineType::Float64();
+      *mtype = MachineType::Float64();
       size = 8;
     } else {
       UNREACHABLE();
     }
-    // TODO(titzer): use special asm-compatibility opcodes?
-    current_function_builder_->EmitWithU8U8(
-        WasmOpcodes::LoadStoreOpcodeOf(mtype, is_set_op_), 0, 0);
-    is_set_op_ = false;
     if (size == 1) {
       // Allow more general expression in byte arrays than the spec
       // strictly permits.
@@ -810,87 +815,103 @@ class AsmWasmBuilderImpl : public AstVisitor {
       // places that strictly should be HEAP8[HEAP32[..]>>0].
       RECURSE(Visit(expr->key()));
       return;
-    } else {
-      Literal* value = expr->key()->AsLiteral();
-      if (value) {
-        DCHECK(value->raw_value()->IsNumber());
-        DCHECK_EQ(kAstI32, TypeOf(value));
-        int val = static_cast<int>(value->raw_value()->AsNumber());
-        // TODO(bradnelson): variable size
-        byte code[] = {WASM_I32V(val * size)};
-        current_function_builder_->EmitCode(code, sizeof(code));
-        return;
-      }
-      BinaryOperation* binop = expr->key()->AsBinaryOperation();
-      if (binop) {
-        DCHECK_EQ(Token::SAR, binop->op());
-        DCHECK(binop->right()->AsLiteral()->raw_value()->IsNumber());
-        DCHECK(kAstI32 == TypeOf(binop->right()->AsLiteral()));
-        DCHECK_EQ(size,
-                  1 << static_cast<int>(
-                      binop->right()->AsLiteral()->raw_value()->AsNumber()));
-        // Mask bottom bits to match asm.js behavior.
-        current_function_builder_->Emit(kExprI32And);
-        byte code[] = {WASM_I8(~(size - 1))};
-        current_function_builder_->EmitCode(code, sizeof(code));
-        RECURSE(Visit(binop->left()));
-        return;
-      }
+    }
+
+    Literal* value = expr->key()->AsLiteral();
+    if (value) {
+      DCHECK(value->raw_value()->IsNumber());
+      DCHECK_EQ(kAstI32, TypeOf(value));
+      int32_t val = static_cast<int32_t>(value->raw_value()->AsNumber());
+      // TODO(titzer): handle overflow here.

[bradnelson, 2016/03/23 19:14:25]

Spec allows 0 - 2^32 -1 here. What did you mean?

[titzer, 2016/03/23 19:53:05]

Right. If it is 2^31, e.g., and then we multiply by {size}, that will overflow.

[bradn, 2016/03/23 19:58:50]

Ah, yes.

+      current_function_builder_->EmitI32Const(val * size);
+      return;
+    }
+    BinaryOperation* binop = expr->key()->AsBinaryOperation();
+    if (binop) {
+      DCHECK_EQ(Token::SAR, binop->op());
+      DCHECK(binop->right()->AsLiteral()->raw_value()->IsNumber());
+      DCHECK(kAstI32 == TypeOf(binop->right()->AsLiteral()));
+      DCHECK_EQ(size,
+                1 << static_cast<int>(
+                    binop->right()->AsLiteral()->raw_value()->AsNumber()));
+      // Mask bottom bits to match asm.js behavior.
+      byte mask = static_cast<byte>(~(size - 1));
+      RECURSE(Visit(binop->left()));
+      current_function_builder_->EmitWithU8(kExprI8Const, mask);
+      current_function_builder_->Emit(kExprI32And);
+      return;
     }
     UNREACHABLE();
   }
 
+  void VisitProperty(Property* expr) {
+    MachineType mtype;
+    VisitPropertyAndEmitIndex(expr, &mtype);
+    current_function_builder_->EmitWithU8U8(
+        WasmOpcodes::LoadStoreOpcodeOf(mtype, false), 0, 0);
+  }
+
   bool VisitStdlibFunction(Call* call, VariableProxy* expr) {
     Variable* var = expr->var();
     AsmTyper::StandardMember standard_object =
         typer_->VariableAsStandardMember(var);
     ZoneList<Expression*>* args = call->arguments();
     LocalType call_type = TypeOf(call);
+
     switch (standard_object) {
       case AsmTyper::kNone: {
         return false;
       }
       case AsmTyper::kMathAcos: {
+        VisitCallArgs(call);
         DCHECK_EQ(kAstF64, call_type);
         current_function_builder_->Emit(kExprF64Acos);
         break;
       }
       case AsmTyper::kMathAsin: {
+        VisitCallArgs(call);
         DCHECK_EQ(kAstF64, call_type);
         current_function_builder_->Emit(kExprF64Asin);
         break;
       }
       case AsmTyper::kMathAtan: {
+        VisitCallArgs(call);
         DCHECK_EQ(kAstF64, call_type);
         current_function_builder_->Emit(kExprF64Atan);
         break;
       }
       case AsmTyper::kMathCos: {
+        VisitCallArgs(call);
         DCHECK_EQ(kAstF64, call_type);
         current_function_builder_->Emit(kExprF64Cos);
         break;
       }
       case AsmTyper::kMathSin: {
+        VisitCallArgs(call);
         DCHECK_EQ(kAstF64, call_type);
         current_function_builder_->Emit(kExprF64Sin);
         break;
       }
       case AsmTyper::kMathTan: {
+        VisitCallArgs(call);
         DCHECK_EQ(kAstF64, call_type);
         current_function_builder_->Emit(kExprF64Tan);
         break;
       }
       case AsmTyper::kMathExp: {
+        VisitCallArgs(call);
         DCHECK_EQ(kAstF64, call_type);
         current_function_builder_->Emit(kExprF64Exp);
         break;
       }
       case AsmTyper::kMathLog: {
+        VisitCallArgs(call);
         DCHECK_EQ(kAstF64, call_type);
         current_function_builder_->Emit(kExprF64Log);
         break;
       }
       case AsmTyper::kMathCeil: {
+        VisitCallArgs(call);
         if (call_type == kAstF32) {
           current_function_builder_->Emit(kExprF32Ceil);
         } else if (call_type == kAstF64) {
@@ -901,6 +922,7 @@ class AsmWasmBuilderImpl : public AstVisitor {
         break;
       }
       case AsmTyper::kMathFloor: {
+        VisitCallArgs(call);
         if (call_type == kAstF32) {
           current_function_builder_->Emit(kExprF32Floor);
         } else if (call_type == kAstF64) {
@@ -911,6 +933,7 @@ class AsmWasmBuilderImpl : public AstVisitor {
         break;
       }
       case AsmTyper::kMathSqrt: {
+        VisitCallArgs(call);
         if (call_type == kAstF32) {
           current_function_builder_->Emit(kExprF32Sqrt);
         } else if (call_type == kAstF64) {
@@ -921,19 +944,36 @@ class AsmWasmBuilderImpl : public AstVisitor {
         break;
       }
       case AsmTyper::kMathAbs: {
-        // TODO(bradnelson): Should this be cast to float?
         if (call_type == kAstI32) {
-          current_function_builder_->Emit(kExprIfElse);
-          current_function_builder_->Emit(kExprI32LtS);
-          Visit(args->at(0));
+          uint16_t tmp = current_function_builder_->AddLocal(kAstI32);
+
+          // if set_local(tmp, x) < 0
+          Visit(call->arguments()->at(0));
+          current_function_builder_->Emit(kExprSetLocal);
+          AddLeb128(tmp, true);
           byte code[] = {WASM_I8(0)};
           current_function_builder_->EmitCode(code, sizeof(code));
-          current_function_builder_->Emit(kExprI32Sub);
+          current_function_builder_->Emit(kExprI32LtS);
+          current_function_builder_->Emit(kExprIf);
+
+          // then (0 - tmp)
           current_function_builder_->EmitCode(code, sizeof(code));
-          Visit(args->at(0));
+          current_function_builder_->Emit(kExprGetLocal);
+          AddLeb128(tmp, true);
+          current_function_builder_->Emit(kExprI32Sub);
+
+          // else tmp
+          current_function_builder_->Emit(kExprElse);
+          current_function_builder_->Emit(kExprGetLocal);
+          AddLeb128(tmp, true);
+          // end
+          current_function_builder_->Emit(kExprEnd);
+
         } else if (call_type == kAstF32) {
+          VisitCallArgs(call);
           current_function_builder_->Emit(kExprF32Abs);
         } else if (call_type == kAstF64) {
+          VisitCallArgs(call);
           current_function_builder_->Emit(kExprF64Abs);
         } else {
           UNREACHABLE();
@@ -943,13 +983,36 @@ class AsmWasmBuilderImpl : public AstVisitor {
       case AsmTyper::kMathMin: {
         // TODO(bradnelson): Change wasm to match Math.min in asm.js mode.
         if (call_type == kAstI32) {
-          current_function_builder_->Emit(kExprIfElse);
+          uint16_t tmp_x = current_function_builder_->AddLocal(kAstI32);
+          uint16_t tmp_y = current_function_builder_->AddLocal(kAstI32);

[bradnelson, 2016/03/23 19:14:25]

Did we want to turn these into asm specific opcodes (mozilla does FWIW)?
(not in this change)

I had started down that direction, but found that TF Calls seem to turn into
separate invocations if you reference them in multiple branches in some cases.
Is there a correct way to express a call + multiple uses of the return value in
the TF graph without loading/storing to the stack?

[titzer, 2016/03/23 19:53:05]

It's probably about the same amount of work either way.

[bradn, 2016/03/23 19:58:50]

K, although I still want to understand why tf call node results can't be reused,
but that's orthogonal to this review.

+
+          // if set_local(tmp_x, x) < set_local(tmp_y, y)
+          Visit(call->arguments()->at(0));
+          current_function_builder_->Emit(kExprSetLocal);
+          AddLeb128(tmp_x, true);
+
+          Visit(call->arguments()->at(1));
+          current_function_builder_->Emit(kExprSetLocal);
+          AddLeb128(tmp_y, true);
+
           current_function_builder_->Emit(kExprI32LeS);
-          Visit(args->at(0));
-          Visit(args->at(1));
+          current_function_builder_->Emit(kExprIf);
+
+          // then tmp_x
+          current_function_builder_->Emit(kExprGetLocal);
+          AddLeb128(tmp_x, true);
+
+          // else tmp_y
+          current_function_builder_->Emit(kExprElse);
+          current_function_builder_->Emit(kExprGetLocal);
+          AddLeb128(tmp_y, true);
+          current_function_builder_->Emit(kExprEnd);
+
         } else if (call_type == kAstF32) {
+          VisitCallArgs(call);
           current_function_builder_->Emit(kExprF32Min);
         } else if (call_type == kAstF64) {
+          VisitCallArgs(call);
           current_function_builder_->Emit(kExprF64Min);
         } else {
           UNREACHABLE();
@@ -959,13 +1022,36 @@ class AsmWasmBuilderImpl : public AstVisitor {
       case AsmTyper::kMathMax: {
         // TODO(bradnelson): Change wasm to match Math.max in asm.js mode.
         if (call_type == kAstI32) {
-          current_function_builder_->Emit(kExprIfElse);
-          current_function_builder_->Emit(kExprI32GtS);
-          Visit(args->at(0));
-          Visit(args->at(1));
+          uint16_t tmp_x = current_function_builder_->AddLocal(kAstI32);
+          uint16_t tmp_y = current_function_builder_->AddLocal(kAstI32);
+
+          // if set_local(tmp_x, x) < set_local(tmp_y, y)
+          Visit(call->arguments()->at(0));
+          current_function_builder_->Emit(kExprSetLocal);
+          AddLeb128(tmp_x, true);
+
+          Visit(call->arguments()->at(1));
+          current_function_builder_->Emit(kExprSetLocal);
+          AddLeb128(tmp_y, true);
+
+          current_function_builder_->Emit(kExprI32LeS);
+          current_function_builder_->Emit(kExprIf);
+
+          // then tmp_y
+          current_function_builder_->Emit(kExprGetLocal);
+          AddLeb128(tmp_y, true);
+
+          // else tmp_x
+          current_function_builder_->Emit(kExprElse);
+          current_function_builder_->Emit(kExprGetLocal);
+          AddLeb128(tmp_x, true);
+          current_function_builder_->Emit(kExprEnd);
+
         } else if (call_type == kAstF32) {
+          VisitCallArgs(call);
           current_function_builder_->Emit(kExprF32Max);
         } else if (call_type == kAstF64) {
+          VisitCallArgs(call);
           current_function_builder_->Emit(kExprF64Max);
         } else {
           UNREACHABLE();
@@ -973,16 +1059,19 @@ class AsmWasmBuilderImpl : public AstVisitor {
         break;
       }
       case AsmTyper::kMathAtan2: {
+        VisitCallArgs(call);
         DCHECK_EQ(kAstF64, call_type);
         current_function_builder_->Emit(kExprF64Atan2);
         break;
       }
       case AsmTyper::kMathPow: {
+        VisitCallArgs(call);
         DCHECK_EQ(kAstF64, call_type);
         current_function_builder_->Emit(kExprF64Pow);
         break;
       }
       case AsmTyper::kMathImul: {
+        VisitCallArgs(call);
         current_function_builder_->Emit(kExprI32Mul);
         break;
       }
@@ -990,6 +1079,7 @@ class AsmWasmBuilderImpl : public AstVisitor {
         DCHECK(args->length() == 1);
         Literal* literal = args->at(0)->AsLiteral();
         if (literal != nullptr) {
+          // constant fold Math.fround(#const);
           if (literal->raw_value()->IsNumber()) {
             float val = static_cast<float>(literal->raw_value()->AsNumber());
             byte code[] = {WASM_F32(val)};
@@ -997,6 +1087,7 @@ class AsmWasmBuilderImpl : public AstVisitor {
             return true;
           }
         }
+        VisitCallArgs(call);
         switch (TypeIndexOf(args->at(0))) {
           case kInt32:
           case kFixnum:
@@ -1020,7 +1111,6 @@ class AsmWasmBuilderImpl : public AstVisitor {
         break;
       }
     }
-    VisitCallArgs(call);
     return true;
   }
 
@@ -1034,9 +1124,10 @@ class AsmWasmBuilderImpl : public AstVisitor {
 
   void VisitCall(Call* expr) {
     Call::CallType call_type = expr->GetCallType(isolate_);
+    std::vector<byte> call_code;
     switch (call_type) {
       case Call::OTHER_CALL: {
-        DCHECK(in_function_);
+        DCHECK_EQ(kFuncScope, scope_);
         VariableProxy* proxy = expr->expression()->AsVariableProxy();
         if (proxy != nullptr) {
           if (VisitStdlibFunction(expr, proxy)) {
@@ -1062,32 +1153,35 @@ class AsmWasmBuilderImpl : public AstVisitor {
         } else {
           index = LookupOrInsertFunction(vp->var());
         }
-        current_function_builder_->Emit(kExprCallFunction);
-        std::vector<uint8_t> index_arr = UnsignedLEB128From(index);
-        current_function_builder_->EmitCode(
-            &index_arr[0], static_cast<uint32_t>(index_arr.size()));
+        call_code.push_back(kExprCallFunction);
+        // TODO(titzer): inefficient and clunky
+        std::vector<byte> leb = UnsignedLEB128From(index);
+        call_code.insert(call_code.end(), leb.begin(), leb.end());
         break;
       }
       case Call::KEYED_PROPERTY_CALL: {
-        DCHECK(in_function_);
+        DCHECK_EQ(kFuncScope, scope_);
         Property* p = expr->expression()->AsProperty();
         DCHECK_NOT_NULL(p);
         VariableProxy* var = p->obj()->AsVariableProxy();
         DCHECK_NOT_NULL(var);
         FunctionTableIndices* indices = LookupFunctionTable(var->var());
-        current_function_builder_->EmitWithVarInt(kExprCallIndirect,
-                                                  indices->signature_index);
-        current_function_builder_->Emit(kExprI32Add);
-        // TODO(bradnelson): variable size
-        byte code[] = {WASM_I32V(indices->start_index)};
-        current_function_builder_->EmitCode(code, sizeof(code));
         RECURSE(Visit(p->key()));
+        current_function_builder_->EmitI32Const(indices->start_index);
+        current_function_builder_->Emit(kExprI32Add);
+        call_code.push_back(kExprCallIndirect);
+        // TODO(titzer): inefficient and clunky
+        std::vector<byte> leb = UnsignedLEB128From(indices->signature_index);
+        call_code.insert(call_code.end(), leb.begin(), leb.end());
         break;
       }
       default:
         UNREACHABLE();
     }
     VisitCallArgs(expr);
+
+    current_function_builder_->EmitCode(
+        &call_code[0], static_cast<uint32_t>(call_code.size()));
   }
 
   void VisitCallNew(CallNew* expr) { UNREACHABLE(); }
@@ -1095,6 +1189,7 @@ class AsmWasmBuilderImpl : public AstVisitor {
   void VisitCallRuntime(CallRuntime* expr) { UNREACHABLE(); }
 
   void VisitUnaryOperation(UnaryOperation* expr) {
+    RECURSE(Visit(expr->expression()));
     switch (expr->op()) {
       case Token::NOT: {
         DCHECK_EQ(kAstI32, TypeOf(expr->expression()));
@@ -1104,7 +1199,6 @@ class AsmWasmBuilderImpl : public AstVisitor {
       default:
         UNREACHABLE();
     }
-    RECURSE(Visit(expr->expression()));
   }
 
   void VisitCountOperation(CountOperation* expr) { UNREACHABLE(); }
@@ -1250,6 +1344,7 @@ class AsmWasmBuilderImpl : public AstVisitor {
   void VisitBinaryOperation(BinaryOperation* expr) {
     ConvertOperation convertOperation = MatchBinaryOperation(expr);
     if (convertOperation == kToDouble) {
+      RECURSE(Visit(expr->left()));
       TypeIndex type = TypeIndexOf(expr->left());
       if (type == kInt32 || type == kFixnum) {
         current_function_builder_->Emit(kExprF64SConvertI32);
@@ -1260,8 +1355,8 @@ class AsmWasmBuilderImpl : public AstVisitor {
       } else {
         UNREACHABLE();
       }
-      RECURSE(Visit(expr->left()));
     } else if (convertOperation == kToInt) {
+      RECURSE(Visit(GetLeft(expr)));
       TypeIndex type = TypeIndexOf(GetLeft(expr));
       if (type == kFloat32) {
         current_function_builder_->Emit(kExprI32SConvertF32);
@@ -1270,10 +1365,20 @@ class AsmWasmBuilderImpl : public AstVisitor {
       } else {
         UNREACHABLE();
       }
-      RECURSE(Visit(GetLeft(expr)));
     } else if (convertOperation == kAsIs) {
       RECURSE(Visit(GetLeft(expr)));
     } else {
+      if (expr->op() == Token::COMMA) {
+        current_function_builder_->Emit(kExprBlock);
+      }
+
+      RECURSE(Visit(expr->left()));
+      RECURSE(Visit(expr->right()));
+
+      if (expr->op() == Token::COMMA) {
+        current_function_builder_->Emit(kExprEnd);

[bradnelson, 2016/03/23 19:14:25]

return here vs the break down below, might be more obvious. (optional)

+      }
+
       switch (expr->op()) {
         BINOP_CASE(Token::ADD, Add, NON_SIGNED_BINOP, true);
         BINOP_CASE(Token::SUB, Sub, NON_SIGNED_BINOP, true);
@@ -1300,14 +1405,11 @@ class AsmWasmBuilderImpl : public AstVisitor {
           break;
         }
         case Token::COMMA: {
-          current_function_builder_->EmitWithVarInt(kExprBlock, 2);
           break;
         }
         default:
           UNREACHABLE();
       }
-      RECURSE(Visit(expr->left()));
-      RECURSE(Visit(expr->right()));
     }
   }
 
@@ -1325,6 +1427,8 @@ class AsmWasmBuilderImpl : public AstVisitor {
   }
 
   void VisitCompareOperation(CompareOperation* expr) {
+    RECURSE(Visit(expr->left()));
+    RECURSE(Visit(expr->right()));
     switch (expr->op()) {
       BINOP_CASE(Token::EQ, Eq, NON_SIGNED_BINOP, false);
       BINOP_CASE(Token::LT, Lt, SIGNED_BINOP, false);
@@ -1334,8 +1438,6 @@ class AsmWasmBuilderImpl : public AstVisitor {
       default:
         UNREACHABLE();
     }
-    RECURSE(Visit(expr->left()));
-    RECURSE(Visit(expr->right()));
   }
 
 #undef BINOP_CASE
@@ -1497,9 +1599,7 @@ class AsmWasmBuilderImpl : public AstVisitor {
   ZoneHashMap local_variables_;
   ZoneHashMap functions_;
   ZoneHashMap global_variables_;
-  bool in_function_;
-  bool is_set_op_;
-  bool marking_exported;
+  AsmScope scope_;
   WasmModuleBuilder* builder_;
   WasmFunctionBuilder* current_function_builder_;
   FunctionLiteral* literal_;
@@ -1509,7 +1609,6 @@ class AsmWasmBuilderImpl : public AstVisitor {
   AsmTyper* typer_;
   TypeCache const& cache_;
   ZoneVector<std::pair<BreakableStatement*, bool>> breakable_blocks_;
-  int block_size_;
   uint16_t init_function_index_;
   uint32_t next_table_index_;
   ZoneHashMap function_tables_;