Implement [Generator].prototype.return.

Implement [Generator].prototype.return.

Note:
- for-of does not yet call it.
- yield* is not yet properly supported.  The planned desugaring of yield* will
  address this.

BUG=v8:3566
LOG=n

[neis, 2016-01-20 16:03:35]

Description was changed from

==========
Implement [Generator].prototype.return.

Note:
- for-of does not yet call it.
- yield* is not yet properly supported.  The planned desugaring of yield* will
  address this.

BUG=
==========

to

==========
Implement [Generator].prototype.return.

Note:
- for-of does not yet call it.
- yield* is not yet properly supported.  The planned desugaring of yield* will
  address this.

BUG=
==========

[neis, 2016-01-20 16:03:35]

neis@chromium.org changed reviewers:
+ bmeurer@chromium.org, jarin@chromium.org, mstarzinger@chromium.org

[neis, 2016-01-20 16:05:41]

On 2016/01/20 16:03:35, neis wrote:
> mailto:neis@chromium.org changed reviewers:
> + mailto:bmeurer@chromium.org, mailto:jarin@chromium.org,
mailto:mstarzinger@chromium.org

Hi guys, I would like to get your feedback on this before I touch the other
architectures.

Thanks,
 georg

[adamk, 2016-01-20 21:16:19]

High-level question: are you planning on putting this behind a flag?

[Dan Ehrenberg, 2016-01-20 21:21:18]

littledan@chromium.org changed reviewers:
+ littledan@chromium.org

[Dan Ehrenberg, 2016-01-20 21:21:18]

I wonder if we might want to implement yield by a desugaring as well, to expand
into something that constructs the iteration result rather than having 4 yield
types. If we do this, then return could also be done by a desugaring, and we
wouldn't need platform-specific changes in fullcodegen like this. However, that
could be done in a later refactoring.

[Dan Ehrenberg, 2016-01-20 23:43:03]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-20 23:43:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1606273002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1606273002/1

[Dan Ehrenberg, 2016-01-20 23:48:21]

Aside from that possible other high-level change (which would require
do-expressions and we probably shouldn't block generator return on for now),
this design looks good to me. The code came out a lot cleaner than I expected,
and I really appreciate all the comments.

https://codereview.chromium.org/1606273002/diff/1/src/full-codegen/x64/full-c...
File src/full-codegen/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/1606273002/diff/1/src/full-codegen/x64/full-c...
src/full-codegen/x64/full-codegen-x64.cc:1967: EmitReturnSequence();
Looks like you're supporting yield* here; what's the remaining issue?

https://codereview.chromium.org/1606273002/diff/1/src/js/generator.js
File src/js/generator.js (right):

https://codereview.chromium.org/1606273002/diff/1/src/js/generator.js#newcode64
src/js/generator.js:64: return { value: UNDEFINED, done: true };
Not that this should be done in this patch, but shouldn't we make sure that
these objects are of the IterationResult map by calling
%_CreateIterResultObject? (Probably slower in fullcodegen for now, but more
optimizable.)

[commit-bot: I haz the power, 2016-01-20 23:58:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-20 23:58:37]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)

[Benedikt Meurer, 2016-01-21 05:09:03]

https://codereview.chromium.org/1606273002/diff/1/src/full-codegen/x64/full-c...
File src/full-codegen/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/1606273002/diff/1/src/full-codegen/x64/full-c...
src/full-codegen/x64/full-codegen-x64.cc:2101: __
Push(Smi::FromInt(resume_mode));  // Consumed in continuation.
I'd vote for dedicated continuations instead of this push magic.

https://codereview.chromium.org/1606273002/diff/1/src/js/generator.js
File src/js/generator.js (right):

https://codereview.chromium.org/1606273002/diff/1/src/js/generator.js#newcode64
src/js/generator.js:64: return { value: UNDEFINED, done: true };
Nice catch! Yes that should use %_CreateIterResultObject, which is BTW.
optimized in fullcodegen as well.

https://codereview.chromium.org/1606273002/diff/1/src/js/generator.js#newcode92
src/js/generator.js:92: return { value: value, done: true };
Use %_CreateIterResultObject, as explained above.

[neis, 2016-01-21 09:06:45]

Description was changed from

==========
Implement [Generator].prototype.return.

Note:
- for-of does not yet call it.
- yield* is not yet properly supported.  The planned desugaring of yield* will
  address this.

BUG=
==========

to

==========
Implement [Generator].prototype.return.

Note:
- for-of does not yet call it.
- yield* is not yet properly supported.  The planned desugaring of yield* will
  address this.

BUG=v8:3566
LOG=n
==========

[neis, 2016-01-21 12:02:46]

On 2016/01/20 21:16:19, adamk wrote:
> High-level question: are you planning on putting this behind a flag?

Yes

[neis, 2016-01-21 12:07:22]

On 2016/01/20 23:48:21, Dan Ehrenberg wrote:
> Aside from that possible other high-level change (which would require
> do-expressions and we probably shouldn't block generator return on for now),
> this design looks good to me. The code came out a lot cleaner than I expected,
> and I really appreciate all the comments.

I don't understand the comments in your first message.  Maybe you can explain
your idea when you're here in two weeks.

>
https://codereview.chromium.org/1606273002/diff/1/src/full-codegen/x64/full-c...
> File src/full-codegen/x64/full-codegen-x64.cc (right):
> 
>
https://codereview.chromium.org/1606273002/diff/1/src/full-codegen/x64/full-c...
> src/full-codegen/x64/full-codegen-x64.cc:1967: EmitReturnSequence();
> Looks like you're supporting yield* here; what's the remaining issue?

In that patch set I had just copied the behavior of the normal yield, which is
clearly wrong.  I now do something simpler that is also clearly wrong :)  At the
very least, yield* must call .return on the inner iterator.  There is one
commented-out test in the test file that demonstrates wrong behavior.

> https://codereview.chromium.org/1606273002/diff/1/src/js/generator.js
> File src/js/generator.js (right):
> 
>
https://codereview.chromium.org/1606273002/diff/1/src/js/generator.js#newcode64
> src/js/generator.js:64: return { value: UNDEFINED, done: true };
> Not that this should be done in this patch, but shouldn't we make sure that
> these objects are of the IterationResult map by calling
> %_CreateIterResultObject? (Probably slower in fullcodegen for now, but more
> optimizable.)

Thanks, done.

[Jarin, 2016-01-22 13:42:48]

The runtime and full-code changes look good (have not looked at the js files).
Happy porting.

https://codereview.chromium.org/1606273002/diff/60001/src/full-codegen/x64/fu...
File src/full-codegen/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/1606273002/diff/60001/src/full-codegen/x64/fu...
src/full-codegen/x64/full-codegen-x64.cc:1959: // Ignoring the resume mode here
is clearly wrong.  Currently, return is
TODO(neis), perhaps?

https://codereview.chromium.org/1606273002/diff/60001/src/runtime/runtime-gen...
File src/runtime/runtime-generator.cc (right):

https://codereview.chromium.org/1606273002/diff/60001/src/runtime/runtime-gen...
src/runtime/runtime-generator.cc:218: return NULL;
NULL -> nullptr.

[neis, 2016-01-22 15:26:21]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-22 15:26:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1606273002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1606273002/80001

[commit-bot: I haz the power, 2016-01-22 16:07:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-22 16:07:50]

Dry run: This issue passed the CQ dry run.

[neis, 2016-01-28 14:22:26]

Closing this CL, it is superseded by https://codereview.chromium.org/1639343005/