Introduce bytecodes for assisting generator suspend and resume.

Introduce bytecodes for assisting generator suspend and resume.

The new bytecodes replace two runtime functions. They are still unsupported by the bytecode graphbuilder, though.

BUG=v8:4907
LOG=n

Committed: https://crrev.com/b09071525070dc62f6a923c2f6f8f5f1bf35dd37
Cr-Commit-Position: refs/heads/master@{#35716}

[neis, 2016-04-21 08:16:30]

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

[commit-bot: I haz the power, 2016-04-21 08:16:38]

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

[neis, 2016-04-21 08:43:04]

Description was changed from

==========
Introduce bytecodes for assisting generator suspend and resume.

This replaces two runtime functions.

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

to

==========
Introduce bytecodes for assisting generator suspend and resume.

This replaces two runtime functions.

BUG=v8:4907
LOG=n
==========

[neis, 2016-04-21 08:43:04]

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

[neis, 2016-04-21 08:43:20]

PTAL. This is orthogonal to the other CL.

[neis, 2016-04-21 08:45:26]

Description was changed from

==========
Introduce bytecodes for assisting generator suspend and resume.

This replaces two runtime functions.

BUG=v8:4907
LOG=n
==========

to

==========
Introduce bytecodes for assisting generator suspend and resume.

The new bytecodes replace two runtime functions. They are still unsupported by
the bytecode graphbuilder, though.

BUG=v8:4907
LOG=n
==========

[commit-bot: I haz the power, 2016-04-21 08:48:03]

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

[commit-bot: I haz the power, 2016-04-21 08:48:04]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-04-21 09:50:50]

Very nice. LGTM with comment.

https://codereview.chromium.org/1904933002/diff/1/src/compiler/code-assembler.h
File src/compiler/code-assembler.h (right):

https://codereview.chromium.org/1904933002/diff/1/src/compiler/code-assembler...
src/compiler/code-assembler.h:248: Node* ChangeInt32ToWord(Node* value);
Rename to ChangeInt32ToIntPtr. We use "word" when signedness doesn't matter.

[Jarin, 2016-04-21 09:51:19]

Love it! lgtm.

[rmcilroy, 2016-04-21 10:21:32]

Looks great, a couple of comments.

https://codereview.chromium.org/1904933002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1904933002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:752:
Int32Sub(Int32Constant(Register::kRegisterFileStartOffset), index);
I'd prefer to keep kRegisterFileStartOffset private if possible. Could you
replace var_index with  InterpreterAssembler::RegisterLocation(Register(0))
outside the loop and then just do a raw Load on this location (incremented by
kPointerSize each iteration) instead?

https://codereview.chromium.org/1904933002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:755:
StoreFixedArrayElementNoWriteBarrier(
I'm not sure NoWriteBarrier is correct here. The runtime implementation used a
write barrier, and since these values are tagged objects I think we need a write
barrier here too. Am I missing something?

https://codereview.chromium.org/1904933002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.h (right):

https://codereview.chromium.org/1904933002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.h:83: compiler::Node* value);
Should these live in CodeStubAssembler instead (they seem like the might be
useful to code stubs too). You could also just remove the LoadObjectField from
here too now that it is provided by CodeStubAssembler.

https://codereview.chromium.org/1904933002/diff/1/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1904933002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:1691: // current context and the given state.
nit - s/the given state/the state given in the accumulator into the generator/

https://codereview.chromium.org/1904933002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:1710: // generator's state, before overwriting it
with kGeneratorExecuting.
nit - Also loads the generator's state and stores it in the accumulator,
before...

[neis, 2016-04-21 11:30:15]

https://codereview.chromium.org/1904933002/diff/1/src/compiler/code-assembler.h
File src/compiler/code-assembler.h (right):

https://codereview.chromium.org/1904933002/diff/1/src/compiler/code-assembler...
src/compiler/code-assembler.h:248: Node* ChangeInt32ToWord(Node* value);
On 2016/04/21 09:50:50, Benedikt Meurer wrote:
> Rename to ChangeInt32ToIntPtr. We use "word" when signedness doesn't matter.

Done.

https://codereview.chromium.org/1904933002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1904933002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:752:
Int32Sub(Int32Constant(Register::kRegisterFileStartOffset), index);
On 2016/04/21 10:21:32, rmcilroy wrote:
> I'd prefer to keep kRegisterFileStartOffset private if possible. Could you
> replace var_index with  InterpreterAssembler::RegisterLocation(Register(0))
> outside the loop and then just do a raw Load on this location (incremented by
> kPointerSize each iteration) instead?

I don't want to change var_index because it's also used to access the fixed
array. Instead, I now replaced kRegisterFileStartOffset with
Register(0).ToOperand. Is that fine?

https://codereview.chromium.org/1904933002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:755:
StoreFixedArrayElementNoWriteBarrier(
On 2016/04/21 10:21:32, rmcilroy wrote:
> I'm not sure NoWriteBarrier is correct here. The runtime implementation used a
> write barrier, and since these values are tagged objects I think we need a
write
> barrier here too. Am I missing something?

I was told that it's not needed since we are writing into a newly allocated
object.

https://codereview.chromium.org/1904933002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.h (right):

https://codereview.chromium.org/1904933002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.h:83: compiler::Node* value);
On 2016/04/21 10:21:32, rmcilroy wrote:
> Should these live in CodeStubAssembler instead (they seem like the might be
> useful to code stubs too). You could also just remove the LoadObjectField from
> here too now that it is provided by CodeStubAssembler.

Agreed, removed them from here.

https://codereview.chromium.org/1904933002/diff/1/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1904933002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:1691: // current context and the given state.
On 2016/04/21 10:21:32, rmcilroy wrote:
> nit - s/the given state/the state given in the accumulator into the generator/

Done.

https://codereview.chromium.org/1904933002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:1710: // generator's state, before overwriting it
with kGeneratorExecuting.
On 2016/04/21 10:21:32, rmcilroy wrote:
> nit - Also loads the generator's state and stores it in the accumulator,
> before...

Done.

[caitp (gmail), 2016-04-21 12:43:37]

full-codegen still uses that runtime func, will it be able to use the
replacement instead?

[caitp (gmail), 2016-04-21 12:44:41]

On 2016/04/21 12:43:37, caitp wrote:
> full-codegen still uses that runtime func, will it be able to use the
> replacement instead?

wait, my bad, missed "Ignition" in the names. carry on

[rmcilroy, 2016-04-21 13:55:02]

LGTM, thanks.

https://codereview.chromium.org/1904933002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1904933002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:752:
Int32Sub(Int32Constant(Register::kRegisterFileStartOffset), index);
On 2016/04/21 11:30:15, neis wrote:
> On 2016/04/21 10:21:32, rmcilroy wrote:
> > I'd prefer to keep kRegisterFileStartOffset private if possible. Could you
> > replace var_index with  InterpreterAssembler::RegisterLocation(Register(0))
> > outside the loop and then just do a raw Load on this location (incremented
by
> > kPointerSize each iteration) instead?
> 
> I don't want to change var_index because it's also used to access the fixed
> array. Instead, I now replaced kRegisterFileStartOffset with
> Register(0).ToOperand. Is that fine?

Works for me, thanks!

https://codereview.chromium.org/1904933002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:755:
StoreFixedArrayElementNoWriteBarrier(
On 2016/04/21 11:30:15, neis wrote:
> On 2016/04/21 10:21:32, rmcilroy wrote:
> > I'm not sure NoWriteBarrier is correct here. The runtime implementation used
a
> > write barrier, and since these values are tagged objects I think we need a
> write
> > barrier here too. Am I missing something?
> 
> I was told that it's not needed since we are writing into a newly allocated
> object.

That sounds plausible :). Please add a comment to that effect.

[neis, 2016-04-21 15:02:23]

The CQ bit was checked by neis@chromium.org

[neis, 2016-04-21 15:02:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, jarin@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1904933002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-04-21 15:02:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1904933002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1904933002/40001

[commit-bot: I haz the power, 2016-04-21 17:08:09]

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

[commit-bot: I haz the power, 2016-04-21 17:08:11]

Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_rel_ng on tryserver.v8 (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/4659)
  v8_linux_nodcheck_rel_ng on tryserver.v8 (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_mac_rel_ng on tryserver.v8 (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/541)

[Benedikt Meurer, 2016-04-21 17:51:12]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2016-04-21 17:51:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1904933002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1904933002/40001

[commit-bot: I haz the power, 2016-04-21 19:53:41]

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

[commit-bot: I haz the power, 2016-04-21 19:53:43]

Try jobs failed on following builders:
  v8_mac_rel_ng on tryserver.v8 (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/550)

[neis, 2016-04-22 08:56:36]

The CQ bit was checked by neis@chromium.org

[neis, 2016-04-22 08:56:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, jarin@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1904933002/#ps60001
(title: "Nix")

[commit-bot: I haz the power, 2016-04-22 08:56:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1904933002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1904933002/60001

[commit-bot: I haz the power, 2016-04-22 09:18:02]

Description was changed from

==========
Introduce bytecodes for assisting generator suspend and resume.

The new bytecodes replace two runtime functions. They are still unsupported by
the bytecode graphbuilder, though.

BUG=v8:4907
LOG=n
==========

to

==========
Introduce bytecodes for assisting generator suspend and resume.

The new bytecodes replace two runtime functions. They are still unsupported by
the bytecode graphbuilder, though.

BUG=v8:4907
LOG=n
==========

[commit-bot: I haz the power, 2016-04-22 09:18:03]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-22 19:15:09]

Description was changed from

==========
Introduce bytecodes for assisting generator suspend and resume.

The new bytecodes replace two runtime functions. They are still unsupported by
the bytecode graphbuilder, though.

BUG=v8:4907
LOG=n
==========

to

==========
Introduce bytecodes for assisting generator suspend and resume.

The new bytecodes replace two runtime functions. They are still unsupported by
the bytecode graphbuilder, though.

BUG=v8:4907
LOG=n

Committed: https://crrev.com/b09071525070dc62f6a923c2f6f8f5f1bf35dd37
Cr-Commit-Position: refs/heads/master@{#35716}
==========

[commit-bot: I haz the power, 2016-04-22 19:15:11]

Patchset 4 (id:??) landed as
https://crrev.com/b09071525070dc62f6a923c2f6f8f5f1bf35dd37
Cr-Commit-Position: refs/heads/master@{#35716}