yield.md

I think we need better primitives for yield/yield*/await in v8.

Here's the current stuff:

void BytecodeGenerator::BuildGeneratorSuspend(Suspend* expr, Register value,
                                              RegisterList registers_to_save) {
  RegisterAllocationScope register_scope(this);

  // Save context, registers, and state. Then return.
  builder()
      ->LoadLiteral(Smi::FromInt(expr->suspend_id()))
      .SuspendGenerator(generator_object_, registers_to_save, expr->flags());

  if (expr->IsNonInitialGeneratorYield()) {
    // GeneratorYield: Wrap the value into IteratorResult.
    RegisterList args = register_allocator()->NewRegisterList(2);
    builder()
        ->MoveRegister(value, args[0])
        .LoadFalse()
        .StoreAccumulatorInRegister(args[1])
        .CallRuntime(Runtime::kInlineCreateIterResultObject, args);
  } else if (expr->IsNonInitialAsyncGeneratorYield()) {
    // AsyncGenerator yields (with the exception of the initial yield) delegate
    // to AsyncGeneratorResolve(), implemented via the runtime call below.
    RegisterList args = register_allocator()->NewRegisterList(3);

    // AsyncGeneratorYield:
    // perform AsyncGeneratorResolve(<generator>, <value>, false).
    builder()
        ->MoveRegister(generator_object_, args[0])
        .MoveRegister(value, args[1])
        .LoadFalse()
        .StoreAccumulatorInRegister(args[2])
        .CallRuntime(Runtime::kInlineAsyncGeneratorResolve, args);
  } else {
    builder()->LoadAccumulatorWithRegister(value);
  }
  builder()->Return();  // Hard return (ignore any finally blocks).
}

void BytecodeGenerator::BuildGeneratorResume(
    Suspend* expr, RegisterList registers_to_restore) {
  RegisterAllocationScope register_scope(this);

  // Clobbers all registers.
  builder()->RestoreGeneratorRegisters(generator_object_, registers_to_restore);

  // Update state to indicate that we have finished resuming. Loop headers
  // rely on this.
  builder()
      ->LoadLiteral(Smi::FromInt(JSGeneratorObject::kGeneratorExecuting))
      .StoreAccumulatorInRegister(generator_state_);

  // When resuming an Async Generator from an Await expression, the sent
  // value is in the [[await_input_or_debug_pos]] slot. Otherwise, the sent
  // value is in the [[input_or_debug_pos]] slot.
  Runtime::FunctionId get_generator_input =
      expr->is_async_generator() && expr->is_await()
          ? Runtime::kInlineAsyncGeneratorGetAwaitInputOrDebugPos
          : Runtime::kInlineGeneratorGetInputOrDebugPos;

  builder()->CallRuntime(get_generator_input, generator_object_);
}

This splits the suspend and resume steps into 2 different helper functions. Quite a lot of code. Registers which need to be saved need to be known by the callers, which complicates any caller of these methods.

Furthermore, since these take a Suspend* operand, it requires an AST node be present, which is problematic for desugaring.

Consider the following updated primitive:

void BytecodeGenerator::BuildYieldPoint(int suspend_id, SuspendFlags flags,
                                        Register operand) {
  // Suspend:
  // `live` is an approximation at this point.
  int live_count = register_allocator()->next_register_index();
  if (operand.index() + 1 == live_count) live_count = operand.index();
  RegisterList live_registers(0, live_count);

  builder()
      ->LoadLiteral(Smi::FromInt(suspend_id))
      .SuspendGenerator(generator_object_, live_registers, flags)
      .LoadAccumulatorWithRegister(operand)
      .Return();

  // Resume:
  {
    RegisterAllocationScope register_scope(this);

    builder()->Bind(generator_jump_table_, suspend_id);

    // Clobbers all registers.
    builder()->RestoreGeneratorRegisters(generator_object_, live_registers);

    // Update state to indicate that we have finished resuming. Loop headers
    // rely on this.
    builder()
        ->LoadLiteral(Smi::FromInt(JSGeneratorObject::kGeneratorExecuting))
        .StoreAccumulatorInRegister(generator_state_);

    // When resuming an Async Generator from an Await expression, the sent
    // value is in the [[await_input_or_debug_pos]] slot. Otherwise, the sent
    // value is in the [[input_or_debug_pos]] slot.
    Runtime::FunctionId get_generator_input =
        flags == SuspendFlags::kAsyncGeneratorAwait
            ? Runtime::kInlineAsyncGeneratorGetAwaitInputOrDebugPos
            : Runtime::kInlineGeneratorGetInputOrDebugPos;

    builder()->CallRuntime(get_generator_input, generator_object_);
  }

  // At this point, accumulator holds the generator's sent value.
}

This combines suspend and resume into the same helper, and doesn't require the caller to know about saved registers.

This could be better if we didn't have to care about the SuspendFlags.

Currently, the GeneratorSuspend() operation needs to know about SuspendFlags, because it has some control over what the opcodes do. In particular, it affects where the bytecode offset is stored and resumed from in teh generator. This only matters for Await in AsyncGenerators, so that the yield input field is not overwritten.

We could do this better by having a special generator state for AsyncGeneratorAwait, which is stored in the generator by the caller of BuildYieldPoint(), and use that to specialize the bytecode ops. There could also just be an "Await" state, and the AsyncGenerator-ness could be determined from the receiver instance type of the generator.

So at that point, the primitive looks more like this:

void BytecodeGenerator::BuildYieldPoint(int suspend_id, Register operand) {
  // Suspend:
  // `live` is an approximation at this point.
  int live_count = register_allocator()->next_register_index();
  if (operand.index() + 1 == live_count) live_count = operand.index();
  RegisterList live_registers(0, live_count);

  builder()
      ->LoadLiteral(Smi::FromInt(suspend_id))
      .SuspendGenerator(generator_object_, live_registers)
      .LoadAccumulatorWithRegister(operand)
      .Return();

  // Resume:
  {
    RegisterAllocationScope register_scope(this);

    builder()->Bind(generator_jump_table_, suspend_id);

    // Clobbers all registers.
    builder()->RestoreGeneratorRegisters(generator_object_, live_registers);

    // Update state to indicate that we have finished resuming. Loop headers
    // rely on this.
    builder()
        ->LoadLiteral(Smi::FromInt(JSGeneratorObject::kGeneratorExecuting))
        .StoreAccumulatorInRegister(generator_state_);

    // Instead of storing the resume mode and resume value in the generator object,
    // lets pull it from the stack instead. The caller of this function can decide
    // whether or not to overwrite the saved generator input (function.sent), depending
    // if it's a yield or not.
    
    // Need to make sure the frame can hold at least a single parameter,
    // and that ResumeGeneratorTrampoline will update that frame register
    // rather than writing the register to something.
    builder()->LoadAccumulatorWithRegister(builder()->Parameter(0));
  }

  // At this point, accumulator holds the generator's sent value.
}

This is pretty good. It means the ResumeGeneratorTrampoline method can be simpler, and delegate updating function.sent (if we even still care about this) to the generator body. Without needing to update function.sent, there's no downside because there's no duplicated code for setting this field anywhere in the generator body. Even when there is, it's relatively little generated code, and doesn't seem that bad to me.

The last thing it would be nice to get rid of is the suspend_id. The suspend id should be ad hoc, and the number of suspends in BytecodeGenerator should be able to grow during bytecode generation, just as new IC feedback slots should be able to be added during bytecode generation. This would mean desugaring will be more concise and easier to follow.

So for the sake of argument, lets say the above helper can be done with BuildYieldPoint(Register operand).

We will then have the following primitives:

void BytecodeGenerator::BuildYield(Register operand) {
  BuildYieldPoint(operand);
  builder()
      ->StoreAccumulatorInRegister(function_sent_);
      .LoadAccumulatorWithRegister(builder()->Parameter(1)); // Load resume mode

  // Now dispatch on resume mode.
  STATIC_ASSERT(JSGeneratorObject::kNext + 1 == JSGeneratorObject::kReturn);
  BytecodeJumpTable* jump_table =
      builder()->AllocateJumpTable(2, JSGeneratorObject::kNext);

  // Throw case (fallthrough)
  {
    builder()
      ->SetExpressionPosition(position);
      .LoadAccumulatorWithRegister(function_sent_);
      .ReThrow();
  }

  // Resume with return.
  {
    builder()->Bind(jump_table, JSGeneratorObject::kReturn);
    if (IsAsyncGeneratorFunction(info()->literal()->kind())) {
      BuildAwait(function_sent_);
      execution_control()->AsyncReturnAccumulator();
    } else {
      builder()->LoadAccumulatorWithRegister(function_sent_);
      execution_control()->ReturnAccumulator();
    }
  }

  {
    // Resume with next.
    builder()->Bind(jump_table, JSGeneratorObject::kNext);
    builder()->LoadAccumulatorWithRegister(function_sent_);
  }
}

void BytecodeGenerator::BuildAwait(Register operand, int position = kNoSourcePosition) {
  {
    // Transform the Await operand using one of these builtin functions.
    // The return value from these functions is what is returned to
    // the caller.
    RegisterAllocationScope reg_scope(this);
    int await_builtin;
    int parameter_count;

    // TODO(caitp): Have fewer builtins for Await.
    if (IsAsyncGeneratorFunction(info()->literal()->kind())) {
      await_builtin = is_await_uncaught()
                    ? Context::ASYNC_GENERATOR_AWAIT_UNCAUGHT
                    : Context::ASYNC_GENERATOR_AWAIT_CAUGHT;
      parameter_count = 2;
    } else {
      DCHECK(IsAsyncFunction(info()->literal()->kind()));
      await_builtin = is_await_uncaught()
                    ? Context::ASYNC_FUNCTION_AWAIT_UNCAUGHT
                    : Context::ASYNC_FUNCTION_AWAIT_CAUGHT;
      parameter_count = 3;
    }

    RegisterList args = register_allocator()->NewRegisterList(parameter_count);
    builder()
        ->MoveRegister(generator_object_, args[0]) // Receiver is a generator object
        .MoveRegister(operand, args[1]);           // The awaited value

    if (!IsAsyncGeneratorFunction(info()->literal()->kind()) {
      // For async functions, also store the outer promise.
      // TODO(caitp): get the outer promise automatically, rather than passing it in.
      Variable* var_promise = closure_scope()->promise_var();
      DCHECK_NOT_NULL(var_promise);
      BuildVariableLoadForAccumulatorValue(var_promise, FeedbackSlot::Invalid(),
                                           HoleCheckMode::kElided);
      builder()->StoreAccumulatorInRegister(args[2]);
    }

    builder()
        ->CallJSRuntime(await_builtin, args);
        .StoreAccumulatorInRegister(operand);
  }

  BuildYieldPoint(operand);

  // Dispatch on resume mode
  {
    builder()
        ->LoadAccumulatorWithRegister(builder()->Parameter(1)); // Load resume mode

    // TODO(caitp): avoid wasting constant pool space for this jump.
    BytecodeJumpTable* jump_table = builder()->AllocateJumpTable(1, JSGeneratorObject::kNext);
    builder()->SwitchOnSmiNoFeedback(jump_table);

    {
      // Resume with throw (switch fallthrough).
      builder()
          ->SetExpressionPosition(position)
          .LoadAccumulatorWithRegister(builder()->Parameter(0)); // load sent value
          .ReThrow();
    }

    {
      // Resume with next.
      builder()->Bind(jump_table, JSGeneratorObject::kNext);
      builder()->LoadAccumulatorWithRegister(builder()->Parameter(0)); // load sent value
    }
  }
}

These abstractions are starting to become palatable, and would make building this stuff in BytecodeGenerator much more straight forward and easier to understand.

There is still room for improvement, but it's getting there. Eliminating the suspend id, SuspendFlags, using stack parameters rather than object field loads, all nice things. Most importantly, improving the primitives complex bytecode generation is built ontop of would make life tremendously better.

Thanks for putting this together Caitlin. I think some of these ideas could certainly make things clearer. As mentioned in the CL, my main issue is with YieldStar having multiple implicit suspend points within it (and a variable number depending on whether it is an async or non-async generator). I feel we could avoid some special casing of yield star by just making it a coordinating node which contains explicit nodes for all suspends within it so that they can be treated identically to the case where there are multiple suspends within a loop statement node or similar.

Regarding the other points brought up here:

• Eliminating SuspendFlags - I completely agree, and think your desugaring of Await goes a long way towards that, thanks.
• Eliminating suspend_id - I agree to some extent since ast-numbering is going away, however we do still need some way to calculate the number of suspends that happen within a loop before we actually visit the loop in the BytecodeGenerator, since we need to preallocate a jump table for the generator switch dispatch in the loop header. This is something JSC don't need to worry about since they do a two-pass system for generators.
• Using stack parameters - I'm not sure on this one, I think it would be a bit confusing to have a generator function which might have zero parameters but suddenly has a parameter for the internals of resuming logic. This again seems like something that works better in JSCs world since they actually have two different functions - one for the initial suspend (which can have the right number of parameters), and one for the rest of the generator (which can have whatever number of parameters they need for internals). I'll have a bit more of a think on this one.