[generators] Perform state dispatch in loop header.

src/interpreter/control-flow-builders.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/interpreter/control-flow-builders.h"
 
 namespace v8 {
 namespace internal {
 namespace interpreter {
 
@@ skipping 79 matching lines @@
 LoopBuilder::~LoopBuilder() { DCHECK(continue_sites_.empty()); }
 
 
 void LoopBuilder::LoopHeader() {
   // Jumps from before the loop header into the loop violate ordering
   // requirements of bytecode basic blocks. The only entry into a loop
   // must be the loop header. Surely breaks is okay? Not if nested
   // 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_);
+}
+
+
 void LoopBuilder::EndLoop() {
   // Loop must have closed form, i.e. all loop elements are within the loop,
   // the loop header precedes the body and next elements in the loop.
   DCHECK(loop_header_.is_bound());
   builder()->Bind(&loop_end_);
   SetBreakTarget(loop_end_);
   if (next_.is_bound()) {
     DCHECK(!condition_.is_bound() || next_.offset() >= condition_.offset());
     SetContinueTarget(next_);
   } else {
@@ skipping 71 matching lines @@
 }
 
 
 void TryFinallyBuilder::EndFinally() {
   // Nothing to be done here.
 }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8