[wasm] Binary 11: AST changes.

src/wasm/asm-wasm-builder.cc

 // Copyright 2015 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/v8.h"
 
 // Required to get M_E etc. in MSVC.
 #if defined(_WIN32)
 #define _USE_MATH_DEFINES
 #endif
@@ skipping 12 matching lines @@
 namespace internal {
 namespace wasm {
 
 #define RECURSE(call)               \
   do {                              \
     DCHECK(!HasStackOverflow());    \
     call;                           \
     if (HasStackOverflow()) return; \
   } while (false)
 
+enum AsmScope { kModuleScope, kInitScope, kFuncScope, kExportScope };
 
 class AsmWasmBuilderImpl : public AstVisitor {
  public:
   AsmWasmBuilderImpl(Isolate* isolate, Zone* zone, FunctionLiteral* literal,
                      Handle<Object> foreign, AsmTyper* typer)
       : local_variables_(HashMap::PointersMatch,
                          ZoneHashMap::kDefaultHashMapCapacity,
                          ZoneAllocationPolicy(zone)),
         functions_(HashMap::PointersMatch, ZoneHashMap::kDefaultHashMapCapacity,
                    ZoneAllocationPolicy(zone)),
         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),
         isolate_(isolate),
         zone_(zone),
         foreign_(foreign),
         typer_(typer),
         cache_(TypeCache::Get()),
         breakable_blocks_(zone),
-        block_size_(0),
         init_function_index_(0),
         next_table_index_(0),
         function_tables_(HashMap::PointersMatch,
                          ZoneHashMap::kDefaultHashMapCapacity,
                          ZoneAllocationPolicy(zone)),
         imported_function_table_(this) {
     InitializeAstVisitor(isolate);
   }
 
   void InitializeInitFunction() {
     init_function_index_ = builder_->AddFunction();
     current_function_builder_ = builder_->FunctionAt(init_function_index_);
     current_function_builder_->ReturnType(kAstStmt);
     builder_->MarkStartFunction(init_function_index_);
     current_function_builder_ = nullptr;
   }
 
   void Compile() {
     InitializeInitFunction();
     RECURSE(VisitFunctionLiteral(literal_));
   }
 
   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();
   }
 
   void VisitImportDeclaration(ImportDeclaration* decl) {}
 
   void VisitExportDeclaration(ExportDeclaration* decl) {}
 
   void VisitStatements(ZoneList<Statement*>* stmts) {
     for (int i = 0; i < stmts->length(); ++i) {
       Statement* stmt = stmts->at(i);
       RECURSE(Visit(stmt));
       if (stmt->IsJump()) break;
     }
   }
 
   void VisitBlock(Block* stmt) {
     if (stmt->statements()->length() == 1) {
       ExpressionStatement* expr =
           stmt->statements()->at(0)->AsExpressionStatement();
       if (expr != nullptr) {
         if (expr->expression()->IsAssignment()) {
           RECURSE(VisitExpressionStatement(expr));
           return;
         }
       }
     }
-    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()));
     }
   }
 
   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();
     }
   };
 
   void VisitExpressionStatement(ExpressionStatement* stmt) {
     RECURSE(Visit(stmt->expression()));
   }
 
   void VisitEmptyStatement(EmptyStatement* stmt) {}
 
   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;
     for (; i >= 0; i--) {
       auto elem = breakable_blocks_.at(i);
       if (elem.first == stmt->target()) {
         DCHECK(elem.second);
         break;
       } else if (elem.second) {
         block_distance += 2;
       } else {
         block_distance += 1;
       }
     }
     DCHECK(i >= 0);
+    current_function_builder_->Emit(kExprNop);
     current_function_builder_->EmitWithVarInt(kExprBr, block_distance);
-    current_function_builder_->Emit(kExprNop);
   }
 
   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;
     for (; i >= 0; i--) {
       auto elem = breakable_blocks_.at(i);
       if (elem.first == stmt->target()) {
         if (elem.second) {
           block_distance++;
         }
         break;
       } else if (elem.second) {
         block_distance += 2;
       } else {
         block_distance += 1;
       }
     }
     DCHECK(i >= 0);
+    current_function_builder_->Emit(kExprNop);
     current_function_builder_->EmitWithVarInt(kExprBr, block_distance);
-    current_function_builder_->Emit(kExprNop);
   }
 
   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);
 
     ZoneList<CaseClause*>* clauses = stmt->cases();
     for (int i = 0; i < clauses->length(); ++i) {
       CaseClause* clause = clauses->at(i);
       if (!clause->is_default()) {
         CompileCase(clause, fall_through, tag);
       } else {
         ZoneList<Statement*>* stmts = clause->statements();
-        block_size_ += stmts->length();
         RECURSE(VisitStatements(stmts));
       }
     }
   }
 
   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()));
+    RECURSE(Visit(stmt->cond()));
     current_function_builder_->Emit(kExprIf);
-    RECURSE(Visit(stmt->cond()));
-    current_function_builder_->EmitWithVarInt(kExprBr, 0);
-    current_function_builder_->Emit(kExprNop);
+    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);
+    DCHECK_EQ(kFuncScope, scope_);
+    BlockVisitor visitor(this, stmt->AsBreakableStatement(), kExprLoop, true);
+    RECURSE(Visit(stmt->cond()));
     current_function_builder_->Emit(kExprIf);
-    RECURSE(Visit(stmt->cond()));
-    current_function_builder_->EmitWithVarInt(kExprBr, 0);
     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_++;
+      RECURSE(Visit(stmt->cond()));
+      current_function_builder_->Emit(kExprI32Eqz);
       current_function_builder_->Emit(kExprIf);
-      current_function_builder_->Emit(kExprI32Eqz);
-      RECURSE(Visit(stmt->cond()));
-      current_function_builder_->EmitWithVarInt(kExprBr, 1);
       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(); }
 
   void VisitForOfStatement(ForOfStatement* stmt) { UNREACHABLE(); }
 
   void VisitTryCatchStatement(TryCatchStatement* stmt) { UNREACHABLE(); }
 
   void VisitTryFinallyStatement(TryFinallyStatement* stmt) { UNREACHABLE(); }
 
   void VisitDebuggerStatement(DebuggerStatement* stmt) { UNREACHABLE(); }
 
   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());
         current_function_builder_->ReturnType(return_type);
         for (int i = 0; i < expr->parameter_count(); i++) {
           LocalType type = TypeFrom(func_type->Parameter(i));
           DCHECK_NE(kAstStmt, type);
           LookupOrInsertLocal(scope->parameter(i), type);
         }
       } else {
         UNREACHABLE();
       }
     }
     RECURSE(VisitStatements(expr->body()));
     RECURSE(VisitDeclarations(scope->declarations()));
   }
 
   void VisitNativeFunctionLiteral(NativeFunctionLiteral* expr) {
     UNREACHABLE();
   }
 
   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) {
     AsmTyper::StandardMember standard_object =
         typer_->VariableAsStandardMember(var);
     double value;
     switch (standard_object) {
       case AsmTyper::kInfinity: {
         value = std::numeric_limits<double>::infinity();
         break;
@@ skipping 35 matching lines @@
         break;
       }
       default: { return false; }
     }
     byte code[] = {WASM_F64(value)};
     current_function_builder_->EmitCode(code, sizeof(code));
     return true;
   }
 
   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: {
             float val = static_cast<float>(expr->raw_value()->AsNumber());
             byte code[] = {WASM_F32(val)};
             current_function_builder_->EmitCode(code, sizeof(code));
             break;
           }
           case kAstF64: {
             double val = static_cast<double>(expr->raw_value()->AsNumber());
             byte code[] = {WASM_F64(val)};
             current_function_builder_->EmitCode(code, sizeof(code));
             break;
           }
           default:
             UNREACHABLE();
         }
       }
     }
   }
 
   void VisitRegExpLiteral(RegExpLiteral* expr) { UNREACHABLE(); }
 
   void VisitObjectLiteral(ObjectLiteral* expr) {
     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();
       Literal* name = prop->key()->AsLiteral();
       DCHECK_NOT_NULL(name);
       DCHECK(name->IsPropertyName());
       const AstRawString* raw_name = name->AsRawPropertyName();
       if (var->is_function()) {
         uint16_t index = LookupOrInsertFunction(var);
         builder_->FunctionAt(index)->Exported(1);
         builder_->FunctionAt(index)
             ->SetName(raw_name->raw_data(), raw_name->length());
       }
     }
   }
 
   void VisitArrayLiteral(ArrayLiteral* expr) { UNREACHABLE(); }
 
   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;
   }
 
   void AddFunctionTable(VariableProxy* table, ArrayLiteral* funcs) {
     FunctionType* func_type =
         funcs->bounds().lower->AsArray()->Element()->AsFunction();
     LocalType return_type = TypeFrom(func_type->Result());
     FunctionSig::Builder sig(zone(), return_type == kAstStmt ? 0 : 1,
                              func_type->Arity());
     if (return_type != kAstStmt) {
@@ skipping 82 matching lines @@
           function->ReturnType(sig->GetReturn());
         }
         for (size_t i = 0; i < sig->parameter_count(); i++) {
           function->AddParam(sig->GetParam(i));
         }
         return index;
       }
     }
   };
 
-  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();
         if (vp != nullptr && vp->var()->IsParameter() &&
             vp->var()->index() == 1) {
           VariableProxy* target = expr->target()->AsVariableProxy();
           if (target->bounds().lower->Is(Type::Function())) {
             const AstRawString* name =
                 prop->key()->AsLiteral()->AsRawPropertyName();
             imported_function_table_.AddImport(target->var(), name->raw_data(),
                                                name->length());
           }
         }
         // Property values in module scope don't emit code, so return.
         return;
       }
       ArrayLiteral* funcs = expr->value()->AsArrayLiteral();
       if (funcs != nullptr &&
           funcs->bounds().lower->AsArray()->Element()->IsFunction()) {
         VariableProxy* target = expr->target()->AsVariableProxy();
         DCHECK_NOT_NULL(target);
         AddFunctionTable(target, funcs);
         // Only add to the function table. No init needed.
         return;
       }
       if (expr->value()->IsCallNew()) {
         // No init code to emit for CallNew nodes.
         return;
       }
-      in_init = true;
-      LoadInitFunction();
+      as_init = true;
     }
-    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;
-      }
+
+    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);
     }
-    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);
-    }
-    RECURSE(Visit(expr->value()));
-    if (in_init) {
-      UnLoadInitFunction();
-    }
+    if (as_init) UnLoadInitFunction();
   }
 
   void VisitYield(Yield* expr) { UNREACHABLE(); }
 
   void VisitThrow(Throw* expr) { UNREACHABLE(); }
 
   void VisitForeignVariable(bool is_float, Property* expr) {
     DCHECK(expr->obj()->AsVariableProxy());
     DCHECK(VariableLocation::PARAMETER ==
            expr->obj()->AsVariableProxy()->var()->location());
@@ skipping 18 matching lines @@
               return;
             }
           }
         } else {
           MaybeHandle<Object> maybe_nvalue =
               i::Object::ToInt32(isolate_, value);
           if (!maybe_nvalue.is_null()) {
             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;
             }
           }
         }
       }
     }
     if (is_float) {
       byte code[] = {WASM_F64(std::numeric_limits<double>::quiet_NaN())};
       current_function_builder_->EmitCode(code, sizeof(code));
     } else {
       byte code[] = {WASM_I32V_1(0)};
       current_function_builder_->EmitCode(code, sizeof(code));
     }
   }
 
-  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.
       // Early versions of Emscripten emit HEAP8[HEAP32[..]|0] in
       // 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) {
           current_function_builder_->Emit(kExprF64Ceil);
         } else {
           UNREACHABLE();
         }
         break;
       }
       case AsmTyper::kMathFloor: {
+        VisitCallArgs(call);
         if (call_type == kAstF32) {
           current_function_builder_->Emit(kExprF32Floor);
         } else if (call_type == kAstF64) {
           current_function_builder_->Emit(kExprF64Floor);
         } else {
           UNREACHABLE();
         }
         break;
       }
       case AsmTyper::kMathSqrt: {
+        VisitCallArgs(call);
         if (call_type == kAstF32) {
           current_function_builder_->Emit(kExprF32Sqrt);
         } else if (call_type == kAstF64) {
           current_function_builder_->Emit(kExprF64Sqrt);
         } else {
           UNREACHABLE();
         }
         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(kExprI32LtS);
+          current_function_builder_->Emit(kExprIf);
+
+          // then (0 - tmp)
+          current_function_builder_->EmitCode(code, sizeof(code));
+          current_function_builder_->Emit(kExprGetLocal);
+          AddLeb128(tmp, true);
           current_function_builder_->Emit(kExprI32Sub);
-          current_function_builder_->EmitCode(code, sizeof(code));
-          Visit(args->at(0));
+
+          // 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();
         }
         break;
       }
       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();
         }
         break;
       }
       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();
         }
         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;
       }
       case AsmTyper::kMathFround: {
         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)};
             current_function_builder_->EmitCode(code, sizeof(code));
             return true;
           }
         }
+        VisitCallArgs(call);
         switch (TypeIndexOf(args->at(0))) {
           case kInt32:
           case kFixnum:
             current_function_builder_->Emit(kExprF32SConvertI32);
             break;
           case kUint32:
             current_function_builder_->Emit(kExprF32UConvertI32);
             break;
           case kFloat32:
             break;
           case kFloat64:
             current_function_builder_->Emit(kExprF32ConvertF64);
             break;
           default:
             UNREACHABLE();
         }
         break;
       }
       default: {
         UNREACHABLE();
         break;
       }
     }
-    VisitCallArgs(call);
     return true;
   }
 
   void VisitCallArgs(Call* expr) {
     ZoneList<Expression*>* args = expr->arguments();
     for (int i = 0; i < args->length(); ++i) {
       Expression* arg = args->at(i);
       RECURSE(Visit(arg));
     }
   }
 
   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)) {
             return;
           }
         }
         uint16_t index;
         VariableProxy* vp = expr->expression()->AsVariableProxy();
         if (vp != nullptr &&
             Type::Any()->Is(vp->bounds().lower->AsFunction()->Result())) {
           LocalType return_type = TypeOf(expr);
           ZoneList<Expression*>* args = expr->arguments();
           FunctionSig::Builder sig(zone(), return_type == kAstStmt ? 0 : 1,
                                    args->length());
           if (return_type != kAstStmt) {
             sig.AddReturn(return_type);
           }
           for (int i = 0; i < args->length(); i++) {
             sig.AddParam(TypeOf(args->at(i)));
           }
           index =
               imported_function_table_.GetFunctionIndex(vp->var(), sig.Build());
         } 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);
+        RECURSE(Visit(p->key()));
+        current_function_builder_->EmitI32Const(indices->start_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()));
+        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(); }
 
   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()));
         current_function_builder_->Emit(kExprI32Eqz);
         break;
       }
       default:
         UNREACHABLE();
     }
-    RECURSE(Visit(expr->expression()));
   }
 
   void VisitCountOperation(CountOperation* expr) { UNREACHABLE(); }
 
   bool MatchIntBinaryOperation(BinaryOperation* expr, Token::Value op,
                                int32_t val) {
     DCHECK_NOT_NULL(expr->right());
     if (expr->op() == op && expr->right()->IsLiteral() &&
         TypeOf(expr) == kAstI32) {
       Literal* right = expr->right()->AsLiteral();
@@ skipping 125 matching lines @@
     if (expr->op() == Token::BIT_XOR) {
       return expr->left()->AsBinaryOperation()->left();
     } else {
       return expr->left();
     }
   }
 
   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);
       } else if (type == kUint32) {
         current_function_builder_->Emit(kExprF64UConvertI32);
       } else if (type == kFloat32) {
         current_function_builder_->Emit(kExprF64ConvertF32);
       } 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);
       } else if (type == kFloat64) {
         current_function_builder_->Emit(kExprI32SConvertF64);
       } 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);
         BINOP_CASE(Token::MUL, Mul, NON_SIGNED_BINOP, true);
         BINOP_CASE(Token::DIV, Div, SIGNED_BINOP, false);
         BINOP_CASE(Token::BIT_OR, Ior, NON_SIGNED_INT_BINOP, true);
         BINOP_CASE(Token::BIT_AND, And, NON_SIGNED_INT_BINOP, true);
         BINOP_CASE(Token::BIT_XOR, Xor, NON_SIGNED_INT_BINOP, true);
         BINOP_CASE(Token::SHL, Shl, NON_SIGNED_INT_BINOP, true);
         BINOP_CASE(Token::SAR, ShrS, NON_SIGNED_INT_BINOP, true);
         BINOP_CASE(Token::SHR, ShrU, NON_SIGNED_INT_BINOP, true);
         case Token::MOD: {
           TypeIndex type = TypeIndexOf(expr->left(), expr->right(), false);
           if (type == kInt32) {
             current_function_builder_->Emit(kExprI32RemS);
           } else if (type == kUint32) {
             current_function_builder_->Emit(kExprI32RemU);
           } else if (type == kFloat64) {
             current_function_builder_->Emit(kExprF64Mod);
             return;
           } else {
             UNREACHABLE();
           }
           break;
         }
         case Token::COMMA: {
-          current_function_builder_->EmitWithVarInt(kExprBlock, 2);
           break;
         }
         default:
           UNREACHABLE();
       }
-      RECURSE(Visit(expr->left()));
-      RECURSE(Visit(expr->right()));
     }
   }
 
   void AddLeb128(uint32_t index, bool is_local) {
     std::vector<uint8_t> index_vec = UnsignedLEB128From(index);
     if (is_local) {
       uint32_t pos_of_index[1] = {0};
       current_function_builder_->EmitCode(
           &index_vec[0], static_cast<uint32_t>(index_vec.size()), pos_of_index,
           1);
     } else {
       current_function_builder_->EmitCode(
           &index_vec[0], static_cast<uint32_t>(index_vec.size()));
     }
   }
 
   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);
       BINOP_CASE(Token::LTE, Le, SIGNED_BINOP, false);
       BINOP_CASE(Token::GT, Gt, SIGNED_BINOP, false);
       BINOP_CASE(Token::GTE, Ge, SIGNED_BINOP, false);
       default:
         UNREACHABLE();
     }
-    RECURSE(Visit(expr->left()));
-    RECURSE(Visit(expr->right()));
   }
 
 #undef BINOP_CASE
 #undef NON_SIGNED_INT_BINOP
 #undef SIGNED_BINOP
 #undef NON_SIGNED_BINOP
 
   enum TypeIndex {
     kInt32 = 0,
     kUint32 = 1,
@@ skipping 141 matching lines @@
     } else {
       return kAstStmt;
     }
   }
 
   Zone* zone() { return zone_; }
 
   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_;
   Isolate* isolate_;
   Zone* zone_;
   Handle<Object> foreign_;
   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_;
   ImportedFunctionTable imported_function_table_;
 
   DEFINE_AST_VISITOR_SUBCLASS_MEMBERS();
 
  private:
   DISALLOW_COPY_AND_ASSIGN(AsmWasmBuilderImpl);
 };
@@ skipping 11 matching lines @@
 // that zone in constructor may be thrown away once wasm module is written.
 WasmModuleIndex* AsmWasmBuilder::Run() {
   AsmWasmBuilderImpl impl(isolate_, zone_, literal_, foreign_, typer_);
   impl.Compile();
   WasmModuleWriter* writer = impl.builder_->Build(zone_);
   return writer->WriteTo(zone_);
 }
 }  // namespace wasm
 }  // namespace internal
 }  // namespace v8