[generators] Perform state dispatch in loop header.

src/interpreter/control-flow-builders.cc

Index: src/interpreter/control-flow-builders.cc
diff --git a/src/interpreter/control-flow-builders.cc b/src/interpreter/control-flow-builders.cc
index 6510aa443aaa2a605989865596cc8227406d3a48..2cd2ba790fb343b5bd4e831d2d150246d349b4ee 100644
--- a/src/interpreter/control-flow-builders.cc
+++ b/src/interpreter/control-flow-builders.cc
@@ -97,6 +97,48 @@ void LoopBuilder::LoopHeader() {
   // and misplaced between the headers.
   DCHECK(break_sites_.empty() && continue_sites_.empty());
   builder()->Bind(&loop_header_);
+
+  if (yield_count > 0) {
+    // Special treatment for loops containing yields (in generator functions).
+
+    // Bind all labels for resume points within the loop to the loop header.
+    // Also create fresh labels for these resume points, to be used inside the
+    // loop.
+    auto& resume_points = builder()->generator_resume_points;
+    int yields_seen = builder()->generator_yields_seen();
+    for (int id = yields_seen; id < yields_seen + yield_count; id++) {
+      DCHECK(0 <= id && id < resume_points.size());
+      builder()->Bind(loop_header_, &(resume_points[id]));
+      resume_points[id] = BytecodeLabel();
+    }
+
+    Register state = Register::new_target();  // HACK
+
+    // If we are not resuming, fall through to loop body.
+    // If we are resuming, perform state dispatch.
+    BytecodeLabel not_resuming;
+    builder()
+        ->LoadLiteral(Smi::FromInt(yields_seen))
+        .CompareOperation(Token::Value::LT, state)
+        .JumpIfTrue(&not_resuming)
+        .IndexedJump(state, yields_seen, yield_count, resume_points)
+        .Bind(&not_resuming);

[rmcilroy, 2016/04/19 09:30:03]

I really think this logic should live in the bytecode-generator. How about
rather than passing the yield_count to the builder, we just pass a list of
additional labels to bind at the loop header (i.e., the yield resume points
between yields_seen and yields_seen + yield_count), and then we add a function
in BytecodeGenerator for VisitIterationHeader(IterationStatement* stmt,
LoopBuilder* loop_builder) to do the logic about jumping to the inner yield
points and updating the labels in the resume_points list to be new
BytecodeLabels pointing to the new inner resume points. WDYT?

[neis, 2016/04/19 11:02:15]

See my comments below.

+  }
+}
+
+
+void LoopBuilder::JumpToHeader() {
+  if (yield_count > 0) {
+    // Special treatment for loops containing yields (in generator functions).
+    // Before jumping back to the loop header, set state to something that
+    // will cause the next iteration to "not resume", i.e. to start from the
+    // beginning.
+    Register state = Register::new_target();  // HACK
+    builder()
+        ->LoadLiteral(Smi::FromInt(-1))
+        .StoreAccumulatorInRegister(state);

[rmcilroy, 2016/04/19 09:30:03]

Could we instead set the resume value to -1 at the final resume point (i.e., in
VisitYield after the resume)?

[neis, 2016/04/19 11:02:15]

There is no "final" yield.  Eg.:
while (...) {
  ...
  if (...) { yield } else { yield }
}
Semantically, JumpToHeader is the right place where this internal state change
should happen.

I was actually quite happy with how my changes were encapsulated in LoopHeader
and JumpToHeader and it makes a lot of sense to me that those are the places to
modify. What don't you like about it?

[rmcilroy, 2016/04/19 11:14:26]

Both of these are different yield points though, right - and both will have
their own VisitYield(). The resume state should only be set to something other
than -1 if we are eventually going to resume in one of these VisitYield points.
If we set the state register to -1 immediately after the
builder()->Bind(&(generator_resume_points_[id])) in each of the Yield points
then we will never end up jumping back to a header with the something other than
"not resume".
I don't agree, I think it should happen once we have fully resumed, after that
it should never be set to something other than -1.
The bytecode array builder shouldn't know about generators or yield points.
Doing it in a VisitIterationHeader (similar to the 
VisitIterationBody function) in Bytecode generator also keeps changes
encapsulated, and keeps all the logic for generators in BytecodeGenerator.

[Jarin, 2016/04/25 09:50:37]

Indeed, I thought we set the state to -1 after the bind in VisitYield, so we
should always see the state -1 when jumping back to loop header. (We should only
jump forward when resuming.)

+  }
+  builder()->Jump(&loop_header_);
 }