[Interpreter] Add support for for..in.

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 8c791db5e3d9ddbab25e65145c45bccffcb413e1..7c57b84501a8a1b99fdaea6a1e23ab651422a1e7 100644
--- a/src/interpreter/bytecode-generator.cc
+++ b/src/interpreter/bytecode-generator.cc
@@ -593,8 +593,116 @@ void BytecodeGenerator::VisitForStatement(ForStatement* stmt) {
 }
 
 
+void BytecodeGenerator::VisitForInAssignment(Expression* expr,
+                                             FeedbackVectorSlot slot) {
+  DCHECK(expr->IsValidReferenceExpression());
+
+  // Evaluate assignment starting with the value to be stored in the
+  // accumulator.
+  Property* property = expr->AsProperty();
+  LhsKind assign_type = Property::GetAssignType(property);
+  switch (assign_type) {
+    case VARIABLE: {
+      Variable* variable = expr->AsVariableProxy()->var();
+      VisitVariableAssignment(variable, slot);
+      break;
+    }
+    case NAMED_PROPERTY: {
+      TemporaryRegisterScope temporary_register_scope(builder());
+      Register value = temporary_register_scope.NewRegister();

[rmcilroy, 2015/10/26 14:04:01]

nit - use expression_result()->NewRegister() ?

[oth, 2015/10/28 11:53:12]

This would cause us to allocate an extra register in VisitForIn, whereas if this
is local it doesn't increase the frame size.

+      builder()->StoreAccumulatorInRegister(value);
+      Register object = VisitForRegisterValue(property->obj());
+      size_t name_index = builder()->GetConstantPoolEntry(
+          property->key()->AsLiteral()->AsPropertyName());
+      builder()->StoreNamedProperty(object, name_index, feedback_index(slot),
+                                    language_mode());
+      break;
+    }
+    case KEYED_PROPERTY: {
+      TemporaryRegisterScope temporary_register_scope(builder());
+      Register value = temporary_register_scope.NewRegister();

[rmcilroy, 2015/10/26 14:04:01]

ditto

[oth, 2015/10/28 11:53:12]

This would cause us to allocate an extra register in VisitForIn, whereas if this
is local it doesn't increase the frame size.

+      builder()->StoreAccumulatorInRegister(value);
+      Register object = VisitForRegisterValue(property->obj());
+      Register key = VisitForRegisterValue(property->key());
+      builder()->LoadAccumulatorWithRegister(value);
+      builder()->StoreKeyedProperty(object, key, feedback_index(slot),
+                                    language_mode());
+      break;
+    }
+    case NAMED_SUPER_PROPERTY:
+    case KEYED_SUPER_PROPERTY:
+      UNIMPLEMENTED();
+  }
+}
+
+
 void BytecodeGenerator::VisitForInStatement(ForInStatement* stmt) {
-  UNIMPLEMENTED();
+  EffectResultScope result_scope(this);

[rmcilroy, 2015/10/26 14:04:01]

Is this needed? Shouldn't we just be using VisitForEffect where necessary below?

[oth, 2015/10/28 11:53:12]

This is needed. RegisterResultScope can allocate a register in the outer()
execution result which is null without this at present. Added a comment about
the StatementResultScope which'd obviate the need for this.

+
+  if (stmt->subject()->IsNullLiteral() ||
+      stmt->subject()->IsUndefinedLiteral(isolate())) {
+    // ForIn generates lots of code, skip if it wouldn't produce any effects.
+    return;
+  }
+
+  TemporaryRegisterScope temporary_register_scope(builder());
+  LoopBuilder loop_builder(builder());
+  ControlScopeForIteration control_scope(this, stmt, &loop_builder);
+
+  // Prepare the state for executing ForIn.
+  builder()->EnterBlock();
+  VisitForAccumulatorValue(stmt->subject());
+  builder()->ForInPrepare();
+  Register for_in_state = temporary_register_scope.NewRegister();

[rmcilroy, 2015/10/26 14:04:01]

nit - move this up (and use execution_result()->NewRegister ? 

[oth, 2015/10/28 11:53:11]

Done.

+  builder()->StoreAccumulatorInRegister(for_in_state);

[rmcilroy, 2015/10/26 14:04:01]

nit - use builder chaining?

[oth, 2015/10/28 11:53:11]

Done-ish. Changed to chain in blocks that are logical units.

+  builder()->LoadUndefined();
+  builder()->CompareOperation(Token::Value::EQ_STRICT, for_in_state,
+                              Strength::WEAK);

[rmcilroy, 2015/10/26 14:04:01]

/s/Strength::WEAK/language_mode_strength()

[oth, 2015/10/28 11:53:12]

Done.

+  loop_builder.BreakIfTrue();
+
+  // The loop

[rmcilroy, 2015/10/26 14:04:01]

nit -full stop 

[oth, 2015/10/28 11:53:11]

Done.

+  BytecodeLabel condition_label, break_label, continue_label;
+
+  Register index = temporary_register_scope.NewRegister();
+  builder()->LoadLiteral(Smi::FromInt(0));
+  builder()->StoreAccumulatorInRegister(index);
+
+  // Check loop termination (accumulator holds index).
+  builder()->Bind(&condition_label);
+  builder()->ForInDone(for_in_state);
+  loop_builder.BreakIfTrue();
+
+  // Get next item

[rmcilroy, 2015/10/26 14:04:01]

nit - full stop

[oth, 2015/10/28 11:53:11]

Done.

+  builder()->LoadAccumulatorWithRegister(index);
+  builder()->ForInNext(for_in_state);
+
+  // Start again if item is undefined.
+  Register value = temporary_register_scope.NewRegister();
+  builder()->StoreAccumulatorInRegister(value);
+  builder()->LoadUndefined();

[rmcilroy, 2015/10/26 14:04:01]

Would it be worth maintaining a temprorary register with Undefined in it for the
duration of the loop - that would allow this to become: 

builder()->ForInNext(for_in_state);
   .CompareOperation(Token::Value::EQ_STRICT, undefined,
language_mode_strength())
loop_builder.ContinueIfTrue();
...

In fact, if we add a new JumpIfUndefined bytecode this could all roll into:
builder()->ForInNext(for_in_state);
loop_builder.ContinueIfUndefined();
VisitForInAssignment(stmt->each(), stmt->EachFeedbackSlot());

With no need to move the value in / out of registers. WDYT?

[oth, 2015/10/28 11:53:11]

This is superb! Done.

+  builder()->CompareOperation(Token::Value::EQ_STRICT, value, Strength::WEAK);
+  loop_builder.ContinueIfTrue();
+  builder()->LoadAccumulatorWithRegister(value);
+
+  // Store the value in the each variable.
+  VisitForInAssignment(stmt->each(), stmt->EachFeedbackSlot());
+  // NB the user's loop variable will be assigned the value of each so
+  // even an empty body will have this assignment.
+  Visit(stmt->body());
+
+  // Increment index and start loop again.
+  builder()->Bind(&continue_label);
+  builder()->LoadLiteral(Smi::FromInt(1));
+  builder()->BinaryOperation(Token::Value::ADD, index, Strength::WEAK);

[rmcilroy, 2015/10/26 14:04:01]

You could use builder()->CountOperation(Token::Value::ADD, index, ...), which
will emit Inc and avoid the LoadLiteral()

Also /s/Strength::WEAK/language_mode_strength();

[oth, 2015/10/28 11:53:12]

Done.

+  builder()->StoreAccumulatorInRegister(index);
+  builder()->Jump(&condition_label);
+
+  // End of loop
+  builder()->Bind(&break_label);
+  builder()->LeaveBlock();
+
+  loop_builder.SetBreakTarget(break_label);
+  loop_builder.SetContinueTarget(continue_label);
 }