Generator objects can suspend

Generator objects can suspend

* src/ast.h:
* src/parser.cc: Differentiate between the different kinds of yields, in
  anticipation of boxing return values.  Parse `return' into `yield' in
  a generator.

* src/runtime.h:
* src/runtime.cc (Runtime_SuspendJSGeneratorObject): New horrible
  runtime function: saves continuation, context, and operands into the
  generator object.

* src/arm/full-codegen-arm.cc (VisitYield):
* src/ia32/full-codegen-ia32.cc (VisitYield):
* src/x64/full-codegen-x64.cc (VisitYield): Arrange to call
  SuspendJSGeneratorObject.  If the call returns the hole, we suspend.
  Otherwise we resume.

BUG=v8:2355
TEST=These codepaths are tested when the generator is first invoked, and so
are covered by mjsunit/harmony/generators-objects.js.

Committed: http://code.google.com/p/v8/source/detail?r=14353

[wingo, 2013-04-05 15:27:09]

This is an early patch that depends on https://codereview.chromium.org/13192004/
and https://codereview.chromium.org/13542002/.  It is more or less complete
though; the next step is to implement resume.

[wingo, 2013-04-12 11:17:26]

Updated patch prepares for boxing "yield" return values, but does not implement
that yet, as we need resume support to be able to test that.

The updated patch also keeps more code in full-codegen.cc.  It is able to do
this via some perhaps-tasteless additions to the assembler, macroassembler, and
full-codegen abstractions, namely:

 * Common aliases for condition codes
 * Common prototype for new JumpIf
 * Addition of FrameOperand in full-codegen

We already have common interfaces for labels and comparisons; seems a shame not
to be able to do conditional jumps.

If you think this is totally tasteless, I understand and will refactor to do
more copy&pastery.

[wingo, 2013-04-15 12:17:34]

Updated patch moves VisitYield to the platform-specific full-codegen.

[wingo, 2013-04-15 12:20:11]

Updated CL moves VisitYield to platform-specific full-codegen.

[wingo, 2013-04-16 14:13:39]

Updated patch initializes JSGeneratorObject::context when allocating, as was the
intention.  (We can avoid saving the context when suspending if there is no
operand stack, but only if it was correctly initialized to begin with.)

[Michael Starzinger, 2013-04-17 13:43:48]

Looking good. We are getting there, awesome! A bunch of comments.

https://codereview.chromium.org/13704010/diff/26001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/13704010/diff/26001/src/ast.h#newcode1969
src/ast.h:1969: INITIAL,
nit: Can we compact this enum a bit (e.g. by moving the comments to after the
constant into the same line)? But definitely keep the comments!

https://codereview.chromium.org/13704010/diff/26001/src/ast.h#newcode1983
src/ast.h:1983: Kind kind() const { return kind_; }
nit: There are so many "kinds" in V8 already, can we rename the accessor and the
field to "yield_kind"?

https://codereview.chromium.org/13704010/diff/26001/src/ia32/full-codegen-ia3...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/13704010/diff/26001/src/ia32/full-codegen-ia3...
src/ia32/full-codegen-ia32.cc:1893: if (expr->kind() == Yield::FINAL) {
For the record: I think it will be best to just pass the yield kind as a second
parameter to the suspend runtime functions, because we will have different state
transitions depending on the kind. But let's not worry about that now for the
purpose of this CL.

https://codereview.chromium.org/13704010/diff/26001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/13704010/diff/26001/src/parser.cc#newcode2524
src/parser.cc:2524: Expression* iterator = factory()->NewVariableProxy(
Hehe, you really like calling generator objects "iterator". Could I convince you
to rename this local variable to "generator" instead?

https://codereview.chromium.org/13704010/diff/26001/src/parser.cc#newcode2528
src/parser.cc:2528: result = factory()->NewExpressionStatement(yield);
Could we add parsing tests to generators-parsing.js that have a return
statements? I know we cannot test execution yet, but we should test generating
the AST for them. I was thinking along the lines of ...

function* g() { yield 1; return; }
function* g() { yield 1; return 2; }
function* g() { yield 1; return 2; yield "dead"; }

https://codereview.chromium.org/13704010/diff/26001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/13704010/diff/26001/src/runtime.cc#newcode2297
src/runtime.cc:2297: ASSERT(args.length() == 0);
nit: Let's add an empty newline after the assert and before the frame iterator
is instantiated here.

https://codereview.chromium.org/13704010/diff/26001/src/runtime.cc#newcode2327
src/runtime.cc:2327: Handle<JSFunction>
function(JSFunction::cast(frame->function()));
Let's play it safe for now and add the following here ...

RUNTIME_ASSERT(function->shared()->is_generator());

https://codereview.chromium.org/13704010/diff/26001/src/runtime.cc#newcode2338
src/runtime.cc:2338: (frame->sp() - kPointerSize);
Just for the record: This magic computation needs to be moved into the frames.h
files later. But I agree that for now we should just keep it inline until the
first prototype is up an running and the implementation has stabilized. But
let's add a TODO in that regard.

https://codereview.chromium.org/13704010/diff/26001/src/runtime.cc#newcode2350
src/runtime.cc:2350: 
IIUC the current implementation relies and the context within the function not
to change. So let's add an assertio checking that. I wouldn't try to fix any
issues with that in this CL, let's just keep the assert around for now.

ASSERT(*generator_object == Context::cast(frame->context()));

https://codereview.chromium.org/13704010/diff/26001/src/runtime.cc#newcode2361
src/runtime.cc:2361: // Saving the operand stack or stack handlers is not
currently implemented.
Let's turn this comment into a TODO.

https://codereview.chromium.org/13704010/diff/26001/src/runtime.cc#newcode2367
src/runtime.cc:2367: return *isolate->factory()->the_hole_value();
Use "isolate->heap()->the_hole_value()" here instead.

[Michael Starzinger, 2013-04-17 13:49:02]

Clarified one of my comments.

https://codereview.chromium.org/13704010/diff/26001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/13704010/diff/26001/src/runtime.cc#newcode2350
src/runtime.cc:2350: 
On 2013/04/17 13:43:48, Michael Starzinger wrote:
> IIUC the current implementation relies and the context within the function not
> to change. So let's add an assertio checking that. I wouldn't try to fix any
> issues with that in this CL, let's just keep the assert around for now.
> 
> ASSERT(*generator_object == Context::cast(frame->context()));

That should have been "generator_object->context()" of course.

[wingo, 2013-04-17 15:22:27]

Thanks for the review.  Updated patch addresses comments.

There are also a few additional things; apologies for the churn.
  * Added convention to indicate that the sign of
JSGeneratorObject::continuation() indicates the state of the generator: running
< 0, closed == 0, or suspended > 0.
  * A Yield::FINAL doesn't capture the continuation; it just marks the generator
as closed.  Also, unlike suspensions, Yield::FINAL emits code to leave nested
statements.
  * Generators get an additional Yield::FINAL in the AST after the function
body, so that the fallthrough return marks the generator as closed.

The above changes were made due to experience with resuming generators from a
later CL, https://codereview.chromium.org/14066016/.

[Michael Starzinger, 2013-04-19 13:15:56]

Just one more comment.

https://codereview.chromium.org/13704010/diff/36001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/13704010/diff/36001/src/objects.h#newcode6286
src/objects.h:6286: 
Can we add constants here for the magic sentinel values used in the
continuation? I assume that you just need two values (i.e. one for "executing"
and one for "closed") right? I was thinking along the lines of ...

static const int kGeneratorExecuting = -1;
static const int kGeneratorClosed = 0;

This would allow us to track statically where those magic values are set and
used. Also I don't think we need the full range of negative numbers.

[wingo, 2013-04-19 13:57:41]

Thanks for the review.  Updated patch adds sentinel values and uses them.

https://codereview.chromium.org/13704010/diff/36001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/13704010/diff/36001/src/objects.h#newcode6286
src/objects.h:6286: 
On 2013/04/19 13:15:56, Michael Starzinger wrote:
> Can we add constants here for the magic sentinel values used in the
> continuation? I assume that you just need two values (i.e. one for "executing"
> and one for "closed") right? I was thinking along the lines of ...
> 
> static const int kGeneratorExecuting = -1;
> static const int kGeneratorClosed = 0;
> 
> This would allow us to track statically where those magic values are set and
> used. Also I don't think we need the full range of negative numbers.

Good point; done.

[Michael Starzinger, 2013-04-19 14:01:09]

LGTM. I'll land this.

[Michael Starzinger, 2013-04-19 14:11:34]

Committed patchset #6 manually as r14353 (presubmit successful).