[Interpreter] Adds support for const/let variables to interpreter.

src/interpreter/bytecode-generator.cc

Index: src/interpreter/bytecode-generator.cc
diff --git a/src/interpreter/bytecode-generator.cc b/src/interpreter/bytecode-generator.cc
index ebfdf33c581585628e491e43102b366e18f6ea77..05037e0eaa44a7577259a32dac8d38072819ae48 100644
--- a/src/interpreter/bytecode-generator.cc
+++ b/src/interpreter/bytecode-generator.cc
@@ -718,7 +718,9 @@ void BytecodeGenerator::VisitFunctionDeclaration(FunctionDeclaration* decl) {
     case VariableLocation::PARAMETER:
     case VariableLocation::LOCAL: {
       VisitForAccumulatorValue(decl->fun());
-      VisitVariableAssignment(variable, FeedbackVectorSlot::Invalid());
+      DCHECK(variable->mode() == LET || variable->mode() == VAR);
+      VisitVariableAssignment(variable, Token::INIT,
+                              FeedbackVectorSlot::Invalid());
       break;
     }
     case VariableLocation::CONTEXT: {
@@ -1000,7 +1002,7 @@ void BytecodeGenerator::VisitForInAssignment(Expression* expr,
   switch (assign_type) {
     case VARIABLE: {
       Variable* variable = expr->AsVariableProxy()->var();
-      VisitVariableAssignment(variable, slot);
+      VisitVariableAssignment(variable, Token::ASSIGN, slot);
       break;
     }
     case NAMED_PROPERTY: {
@@ -1504,14 +1506,25 @@ void BytecodeGenerator::VisitVariableProxy(VariableProxy* proxy) {
   VisitVariableLoad(proxy->var(), proxy->VariableFeedbackSlot());
 }
 
+void BytecodeGenerator::BuildHoleCheckForVariableLoad(VariableMode mode,
+                                                      Handle<String> name) {
+  if (mode == CONST_LEGACY) {
+    BytecodeLabel end_label;
+    builder()->JumpIfNotHole(&end_label).LoadUndefined().Bind(&end_label);

[rmcilroy, 2016/01/26 14:37:28]

Newlines between builder operations (throughout).

[mythria, 2016/02/01 10:02:18]

I added newlines. cl format removes them. Shall I override cl format?

[rmcilroy, 2016/02/03 12:28:25]

I think we should override this if possible, it is pretty difficult to read
otherwise (another option would be to just call builder() at the start of every
line here)

+  } else if (mode == LET || mode == CONST) {
+    BuildThrowIfHole(name);
+  }
+}
 
 void BytecodeGenerator::VisitVariableLoad(Variable* variable,
                                           FeedbackVectorSlot slot,
                                           TypeofMode typeof_mode) {
+  VariableMode mode = variable->mode();
   switch (variable->location()) {
     case VariableLocation::LOCAL: {
       Register source(Register(variable->index()));
       builder()->LoadAccumulatorWithRegister(source);
+      BuildHoleCheckForVariableLoad(mode, variable->name());
       execution_result()->SetResultInAccumulator();
       break;
     }
@@ -1520,6 +1533,7 @@ void BytecodeGenerator::VisitVariableLoad(Variable* variable,
       // index -1 but is parameter index 0 in BytecodeArrayBuilder).
       Register source = builder()->Parameter(variable->index() + 1);
       builder()->LoadAccumulatorWithRegister(source);
+      BuildHoleCheckForVariableLoad(mode, variable->name());
       execution_result()->SetResultInAccumulator();
       break;
     }
@@ -1552,10 +1566,10 @@ void BytecodeGenerator::VisitVariableLoad(Variable* variable,
               .StoreAccumulatorInRegister(context_reg);
         }
       }
+
       builder()->LoadContextSlot(context_reg, variable->index());
+      BuildHoleCheckForVariableLoad(mode, variable->name());
       execution_result()->SetResultInAccumulator();
-      // TODO(rmcilroy): Perform check for uninitialized legacy const, const and
-      // let variables.
       break;
     }
     case VariableLocation::LOOKUP: {
@@ -1581,21 +1595,119 @@ Register BytecodeGenerator::VisitVariableLoadForRegisterValue(
   return register_scope.ResultRegister();
 }
 
+void BytecodeGenerator::BuildThrowIfHole(Handle<String> name) {
+  Register name_reg = register_allocator()->NewRegister();
+  BytecodeLabel end_label;
+  builder()
+      ->JumpIfNotHole(&end_label)
+      .LoadLiteral(name)
+      .StoreAccumulatorInRegister(name_reg)
+      .CallRuntime(Runtime::kThrowReferenceError, name_reg, 1)
+      .Bind(&end_label);
+}
+
+void BytecodeGenerator::BuildThrowIfNotHole(Handle<String> name) {
+  Register name_reg = register_allocator()->NewRegister();
+  BytecodeLabel end_label;
+  builder()
+      ->JumpIfHole(&end_label)
+      .LoadLiteral(name)
+      .StoreAccumulatorInRegister(name_reg)
+      .CallRuntime(Runtime::kThrowReferenceError, name_reg, 1)
+      .Bind(&end_label);
+}
+
+void BytecodeGenerator::BuildThrowReassignConstant(Register destination,
+                                                   Handle<String> name) {
+  Register name_reg = register_allocator()->NewRegister();
+  BytecodeLabel else_label;
+  builder()
+      ->LoadLiteral(name)
+      .StoreAccumulatorInRegister(name_reg)
+      .LoadAccumulatorWithRegister(destination)
+      .JumpIfNotHole(&else_label)
+      .CallRuntime(Runtime::kThrowReferenceError, name_reg, 1)
+      .Bind(&else_label)
+      .CallRuntime(Runtime::kThrowConstAssignError, name_reg, 1);
+}
+
+void BytecodeGenerator::BuildThrowReassignConstant(Handle<String> name) {

[rmcilroy, 2016/01/26 14:37:28]

Could we merge both BuildThrowReassignConstant (and just always have the value
in a register)?

[mythria, 2016/02/01 10:02:18]

Done.

+  Register name_reg = register_allocator()->NewRegister();
+  BytecodeLabel else_label;
+  builder()
+      ->JumpIfNotHole(&else_label)
+      .LoadLiteral(name)

[rmcilroy, 2016/01/26 14:37:28]

Move LoadLiteral / StoreAccumulatorInRegister(name_reg) before jump (to avoid
doing it twice) ?

[mythria, 2016/02/01 10:02:18]

For throwing const reassign error, we don't need the variable name. It was a bug
and I fixed it. So we need these only in one part of the if. 

Done.

+      .StoreAccumulatorInRegister(name_reg)
+      .CallRuntime(Runtime::kThrowReferenceError, name_reg, 1)
+      .Bind(&else_label)
+      .LoadLiteral(name)
+      .StoreAccumulatorInRegister(name_reg)
+      .CallRuntime(Runtime::kThrowConstAssignError, name_reg, 1);
+}
 
 void BytecodeGenerator::VisitVariableAssignment(Variable* variable,
+                                                Token::Value op,
                                                 FeedbackVectorSlot slot) {
+  VariableMode mode = variable->mode();
+  RegisterAllocationScope assignment_register_scope(this);
+  BytecodeLabel end_label;
+  bool hole_check_required =
+      (mode == CONST_LEGACY) || (mode == LET && op != Token::INIT) ||
+      (mode == CONST && op != Token::INIT) ||
+      (mode == CONST && op == Token::INIT && variable->is_this());
   switch (variable->location()) {
+    case VariableLocation::PARAMETER:
     case VariableLocation::LOCAL: {
-      // TODO(rmcilroy): support const mode initialization.
-      Register destination(variable->index());
-      builder()->StoreAccumulatorInRegister(destination);
-      break;
-    }
-    case VariableLocation::PARAMETER: {
-      // The parameter indices are shifted by 1 (receiver is variable
-      // index -1 but is parameter index 0 in BytecodeArrayBuilder).
-      Register destination(builder()->Parameter(variable->index() + 1));
-      builder()->StoreAccumulatorInRegister(destination);
+      Register destination;
+      if (VariableLocation::PARAMETER == variable->location()) {
+        destination = Register(builder()->Parameter(variable->index() + 1));
+      } else {
+        destination = Register(variable->index());
+      }
+
+      if (!hole_check_required) {
+        builder()->StoreAccumulatorInRegister(destination);
+      } else if (mode == CONST && op != Token::INIT) {

[rmcilroy, 2016/01/26 14:37:28]

As discussed, could you merge the code to do the hole check for this and the
context slot into a common helper function.

[mythria, 2016/02/01 10:02:18]

Done. I did not pull const_legacy && Token::Init to helper function. Because, we
need to store to destination conditionally. We have to pass cotext_reg and
destination registers and only one of them would be valid. Let me know if you
have any suggestions. 

+        // Assigning to constant is not allowed. Throw an appropriate
+        // exception. Accumulator is not restored back.
+        BuildThrowReassignConstant(destination, variable->name());
+      } else if (mode == CONST_LEGACY && op != Token::INIT) {
+        // TODO(mythria): Find a test case for strict mode legacy constants.

[rmcilroy, 2016/01/26 14:37:28]

Legacy const doesn't exist for strict mode, right?

[mythria, 2016/02/01 10:02:18]

Done. Yes, it was a old comment. Retained the DCHECK because we are not totally
sure and have to check with andreas.

+        DCHECK(!is_strict(language_mode()));
+      } else {
+        // Store accumulator to a temporary register and load destination to
+        // perform hole check.
+        BytecodeLabel end_label;
+        Register value_temp = register_allocator()->NewRegister();
+        builder()
+            ->StoreAccumulatorInRegister(value_temp)
+            .LoadAccumulatorWithRegister(destination);
+        if (mode == CONST_LEGACY && op == Token::INIT) {
+          // If the destination is already initialized silently ignore this
+          // assignment. Restore the accumulator.
+          builder()
+              ->JumpIfNotHole(&end_label)
+              .MoveRegister(value_temp, destination)
+              .Bind(&end_label)
+              .LoadAccumulatorWithRegister(value_temp);
+        } else if (mode == LET && op != Token::INIT) {
+          BuildThrowIfHole(variable->name());
+          // Move does not help here, because accumulator should be restored to
+          // value_temp as well.
+          builder()
+              ->LoadAccumulatorWithRegister(value_temp)
+              .StoreAccumulatorInRegister(destination);
+        } else {
+          DCHECK(variable->is_this() && mode == CONST && op == Token::INIT);
+          // Perform an initialization check for 'this'. 'this' variable is the
+          // only variable being able to trigger bind operations outside the TDZ
+          // via 'super' calls.
+          BuildThrowIfNotHole(variable->name());
+          builder()
+              ->LoadAccumulatorWithRegister(value_temp)
+              .StoreAccumulatorInRegister(destination);
+        }
+      }
       break;
     }
     case VariableLocation::GLOBAL:
@@ -1605,14 +1717,16 @@ void BytecodeGenerator::VisitVariableAssignment(Variable* variable,
       break;
     }
     case VariableLocation::CONTEXT: {
-      // TODO(rmcilroy): support const mode initialization.
       int depth = execution_context()->ContextChainDepth(variable->scope());
       ContextScope* context = execution_context()->Previous(depth);
       Register context_reg;
+      Register value_temp;
+      bool value_in_accumulator = true;

[rmcilroy, 2016/01/26 14:37:28]

Let's not try to be smart with "value_in_accumulator" for now (particularly
since we will replace these by special bytecodes), let's just assume that it is
always in the accumulator (maintaining the LoadAccumulatorWithRegister in the
else branch) and have a new temporary register for the hole check which is
stored from the accumulator.

[mythria, 2016/02/01 10:02:18]

Done.

+
       if (context) {
         context_reg = context->reg();
       } else {
-        Register value_temp = register_allocator()->NewRegister();
+        value_temp = register_allocator()->NewRegister();
         context_reg = register_allocator()->NewRegister();
         // Walk the context chain to find the context at the given depth.
         // TODO(rmcilroy): Perform this work in a bytecode handler once we have
@@ -1628,15 +1742,85 @@ void BytecodeGenerator::VisitVariableAssignment(Variable* variable,
               ->LoadContextSlot(context_reg, Context::PREVIOUS_INDEX)
               .StoreAccumulatorInRegister(context_reg);
         }
-        builder()->LoadAccumulatorWithRegister(value_temp);
+        value_in_accumulator = false;
+      }
+
+      if (!hole_check_required) {
+        if (!value_in_accumulator) {
+          builder()->LoadAccumulatorWithRegister(value_temp);
+        }
+        builder()->StoreContextSlot(context_reg, variable->index());
+      } else if (mode == CONST && op != Token::INIT) {
+        // Assigning to constant is not allowed. Throw an appropriate
+        // exception. Accumulator is not restored back.
+        builder()->LoadContextSlot(context_reg, variable->index());
+        BuildThrowReassignConstant(variable->name());
+      } else if (mode == CONST_LEGACY && op != Token::INIT) {
+        // TODO(mythria): Find a test case for strict mode legacy constants.

[rmcilroy, 2016/01/26 14:37:28]

ditto.

[mythria, 2016/02/01 10:02:18]

Done.

+        DCHECK(!is_strict(language_mode()));
+        // Accumulator has to be restored back.
+        if (!value_in_accumulator) {
+          builder()->LoadAccumulatorWithRegister(value_temp);
+        }
+      } else {
+        // Store accumulator to a temporary register and load destination to
+        // perform hole check.
+        if (value_in_accumulator) {
+          value_temp = register_allocator()->NewRegister();
+          builder()->StoreAccumulatorInRegister(value_temp);
+        }
+        builder()->LoadContextSlot(context_reg, variable->index());
+        BytecodeLabel end_label;
+        if (mode == CONST_LEGACY && op == Token::INIT) {
+          // If the destination is already initialized silently ignore this
+          // assignment and restore back the accumulator.

[rmcilroy, 2016/01/26 14:37:28]

Not sure what "restore back the accumulator" means here?

[mythria, 2016/02/01 10:02:18]

Done.

+          builder()
+              ->JumpIfNotHole(&end_label)
+              .LoadAccumulatorWithRegister(value_temp)
+              .StoreContextSlot(context_reg, variable->index())
+              .Bind(&end_label)
+              .LoadAccumulatorWithRegister(value_temp);
+        } else if (mode == LET && op != Token::INIT) {
+          BuildThrowIfHole(variable->name());
+          builder()
+              ->LoadAccumulatorWithRegister(value_temp)
+              .StoreContextSlot(context_reg, variable->index());
+        } else {
+          DCHECK(variable->is_this() && mode == CONST && op == Token::INIT);
+          // Perform an initialization check for 'this'. 'this' variable is the
+          // only variable being able to trigger bind operations outside the TDZ

[rmcilroy, 2016/01/26 14:37:28]

/s/variable being able/variable able

[mythria, 2016/02/01 10:02:18]

Done.

+          // via 'super' calls.
+          BuildThrowIfNotHole(variable->name());
+          builder()
+              ->LoadAccumulatorWithRegister(value_temp)
+              .StoreContextSlot(context_reg, variable->index());
+        }
       }
-      builder()->StoreContextSlot(context_reg, variable->index());
       break;
     }
     case VariableLocation::LOOKUP: {
-      // TODO(mythria): Use Runtime::kInitializeLegacyConstLookupSlot for
-      // initializations of const declarations.
-      builder()->StoreLookupSlot(variable->name(), language_mode());
+      if (mode == CONST_LEGACY && op == Token::INIT) {
+        register_allocator()->PrepareForConsecutiveAllocations(3);
+        Register value = register_allocator()->NextConsecutiveRegister();
+        Register context = register_allocator()->NextConsecutiveRegister();
+        Register name = register_allocator()->NextConsecutiveRegister();
+
+        // InitializeLegacyConstLookupSlot runtime call returns the 'value'
+        // passed to it. So, accumulator will have its original contents when
+        // runtime call returns.
+        builder()
+            ->StoreAccumulatorInRegister(value)
+            .MoveRegister(execution_context()->reg(), context)
+            .LoadLiteral(variable->name())
+            .StoreAccumulatorInRegister(name)
+            .CallRuntime(Runtime::kInitializeLegacyConstLookupSlot, value, 3);
+      } else if (mode == CONST_LEGACY && op != Token::INIT) {
+        // TODO(mythria): Find a test case for strict mode. We should throw in
+        // strict mode.
+        DCHECK(!is_strict(language_mode()));
+      } else {
+        builder()->StoreLookupSlot(variable->name(), language_mode());
+      }
       break;
     }
   }
@@ -1729,7 +1913,7 @@ void BytecodeGenerator::VisitAssignment(Assignment* expr) {
       // TODO(oth): The VisitVariableAssignment() call is hard to reason about.
       // Is the value in the accumulator safe? Yes, but scary.
       Variable* variable = expr->target()->AsVariableProxy()->var();
-      VisitVariableAssignment(variable, slot);
+      VisitVariableAssignment(variable, expr->op(), slot);
       break;
     }
     case NAMED_PROPERTY:
@@ -2155,7 +2339,7 @@ void BytecodeGenerator::VisitCountOperation(CountOperation* expr) {
   switch (assign_type) {
     case VARIABLE: {
       Variable* variable = expr->expression()->AsVariableProxy()->var();
-      VisitVariableAssignment(variable, feedback_slot);
+      VisitVariableAssignment(variable, expr->op(), feedback_slot);
       break;
     }
     case NAMED_PROPERTY: {
@@ -2419,7 +2603,8 @@ void BytecodeGenerator::VisitArgumentsObject(Variable* variable) {
           ? CreateArgumentsType::kUnmappedArguments
           : CreateArgumentsType::kMappedArguments;
   builder()->CreateArguments(type);
-  VisitVariableAssignment(variable, FeedbackVectorSlot::Invalid());
+  VisitVariableAssignment(variable, Token::ASSIGN,
+                          FeedbackVectorSlot::Invalid());
 }
 
 
@@ -2431,7 +2616,7 @@ void BytecodeGenerator::VisitThisFunctionVariable(Variable* variable) {
 
   // Store the closure we were called with in the given variable.
   builder()->LoadAccumulatorWithRegister(Register::function_closure());
-  VisitVariableAssignment(variable, FeedbackVectorSlot::Invalid());
+  VisitVariableAssignment(variable, Token::INIT, FeedbackVectorSlot::Invalid());
 }
 
 
@@ -2440,7 +2625,7 @@ void BytecodeGenerator::VisitNewTargetVariable(Variable* variable) {
 
   // Store the new target we were called with in the given variable.
   builder()->LoadAccumulatorWithRegister(Register::new_target());
-  VisitVariableAssignment(variable, FeedbackVectorSlot::Invalid());
+  VisitVariableAssignment(variable, Token::INIT, FeedbackVectorSlot::Invalid());
 }