[Interpreter] Handle various generator intrinsics

[Interpreter] Handle various generator intrinsics

We can avoid some runtime calls by implementing intrinsics.

BUG=

Review-Url: https://codereview.chromium.org/2889973002
Cr-Commit-Position: refs/heads/master@{#45418}
Committed: https://chromium.googlesource.com/v8/v8/+/0980d759f5b8bcc8bf1e8dd5c1941324ad9636b6

[mvstanton, 2017-05-17 14:03:30]

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

[commit-bot: I haz the power, 2017-05-17 14:03:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mvstanton, 2017-05-17 14:04:57]

mvstanton@chromium.org changed reviewers:
+ adamk@chromium.org, rmcilroy@chromium.org

[mvstanton, 2017-05-17 14:04:58]

Hi Adam, hi Ross,
Here is a CL that removes 3 runtime calls from generators, following the typical
intrinsic pattern. Have a look, thanks!
--Michael

[commit-bot: I haz the power, 2017-05-17 14:34:46]

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

[commit-bot: I haz the power, 2017-05-17 14:34:47]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2017-05-17 14:48:29]

https://codereview.chromium.org/2889973002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-intrinsics-generator.cc (right):

https://codereview.chromium.org/2889973002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-intrinsics-generator.cc:412: __
LoadObjectField(generator, JSGeneratorObject::kContextOffset);
Maybe there is a case for having a LoadObjectField bytecode (which takes the
object in the accumulator and the offset as an operand) now which could be used
to implement all three of these intrinsics. WDYT?

[adamk, 2017-05-17 17:35:31]

lgtm, thanks, this was on my backlog already :)

https://codereview.chromium.org/2889973002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-intrinsics-generator.cc (right):

https://codereview.chromium.org/2889973002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-intrinsics-generator.cc:412: __
LoadObjectField(generator, JSGeneratorObject::kContextOffset);
On 2017/05/17 14:48:28, rmcilroy wrote:
> Maybe there is a case for having a LoadObjectField bytecode (which takes the
> object in the accumulator and the offset as an operand) now which could be
used
> to implement all three of these intrinsics. WDYT?

I like this idea, I was curious why we didn't have something like this when I
started looking at generators last week. But I don't think it needs to hold up
this CL.

[mvstanton, 2017-05-18 10:00:59]

https://codereview.chromium.org/2889973002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-intrinsics-generator.cc (right):

https://codereview.chromium.org/2889973002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-intrinsics-generator.cc:412: __
LoadObjectField(generator, JSGeneratorObject::kContextOffset);
On 2017/05/17 17:35:31, adamk wrote:
> On 2017/05/17 14:48:28, rmcilroy wrote:
> > Maybe there is a case for having a LoadObjectField bytecode (which takes the
> > object in the accumulator and the offset as an operand) now which could be
> used
> > to implement all three of these intrinsics. WDYT?
> 
> I like this idea, I was curious why we didn't have something like this when I
> started looking at generators last week. But I don't think it needs to hold up
> this CL.

I'm happy to take a run at it. There are probably other cases to plug in
LoadObjectField too. It should make the bytecode smaller, right? (tryin' to
motivate myself :D)

[rmcilroy, 2017-05-18 11:31:23]

On 2017/05/18 10:00:59, mvstanton wrote:
>
https://codereview.chromium.org/2889973002/diff/1/src/interpreter/interpreter...
> File src/interpreter/interpreter-intrinsics-generator.cc (right):
> 
>
https://codereview.chromium.org/2889973002/diff/1/src/interpreter/interpreter...
> src/interpreter/interpreter-intrinsics-generator.cc:412: __
> LoadObjectField(generator, JSGeneratorObject::kContextOffset);
> On 2017/05/17 17:35:31, adamk wrote:
> > On 2017/05/17 14:48:28, rmcilroy wrote:
> > > Maybe there is a case for having a LoadObjectField bytecode (which takes
the
> > > object in the accumulator and the offset as an operand) now which could be
> > used
> > > to implement all three of these intrinsics. WDYT?
> > 
> > I like this idea, I was curious why we didn't have something like this when
I
> > started looking at generators last week. But I don't think it needs to hold
up
> > this CL.
> 
> I'm happy to take a run at it. There are probably other cases to plug in
> LoadObjectField too. It should make the bytecode smaller, right? (tryin' to
> motivate myself :D)

It'd make the bytecode smaller and would certainly be faster (avoid having to
build a frame and switch on the intrinsic ID). Might also allow better codegen
in TF (how's that for modivation :)). Happy for you to land this CL as is in the
interim. LGTM

[mvstanton, 2017-05-19 08:36:22]

Thanks guys...

Okay, I did this change in the morning and decided to back off from it and land
the CL as is. The reason is that the loss of contextual information about what
we are doing is usually benign but occasionally (ie, popping up at the worst
time) severe.

Looking at our intrinsic lowering in TF, mostly we do the same thing as we do in
interpreter intrinsic lowering -- simply loading a field and moving on. But for
one of these generator fields, we at least know it's a smi, and that produces
better code. In bytecode generator, these LoadObjectField opcodes would be
opaque to us...we no longer know the type of the resulting object.

It's a balancing act. I'd love to save space/time in the bytecode, but each
introduction of a LoadObjectField there risks a silent failure to recognize
context in TF lowering.

One solution would be to have dedicated bytecodes that preserve object type. Or,
use LoadObjectField only for those for which we know we don't need more
information. In that sense, I could currently use it for 2 of these intrinsics,
but not the one that loads a smi.

I'd propose not to do that now...because to the extent that we imagine
significant savings in bytecode, we are hiding information from ourselves in the
downwind (TF) compiler.

Super interesting stuff!

[rmcilroy, 2017-05-19 08:55:20]

On 2017/05/19 08:36:22, mvstanton wrote:
> Thanks guys...
> 
> Okay, I did this change in the morning and decided to back off from it and
land
> the CL as is. The reason is that the loss of contextual information about what
> we are doing is usually benign but occasionally (ie, popping up at the worst
> time) severe.
> 
> Looking at our intrinsic lowering in TF, mostly we do the same thing as we do
in
> interpreter intrinsic lowering -- simply loading a field and moving on. But
for
> one of these generator fields, we at least know it's a smi, and that produces
> better code. In bytecode generator, these LoadObjectField opcodes would be
> opaque to us...we no longer know the type of the resulting object.
> 
> It's a balancing act. I'd love to save space/time in the bytecode, but each
> introduction of a LoadObjectField there risks a silent failure to recognize
> context in TF lowering.
> 
> One solution would be to have dedicated bytecodes that preserve object type.
Or,
> use LoadObjectField only for those for which we know we don't need more
> information. In that sense, I could currently use it for 2 of these
intrinsics,
> but not the one that loads a smi.
> 
> I'd propose not to do that now...because to the extent that we imagine
> significant savings in bytecode, we are hiding information from ourselves in
the
> downwind (TF) compiler.
> 
> Super interesting stuff!

Very interesting, thanks for the explanation. Not adding a LoadObjectField
bytecode now and landing this now SGTM. 

There are a couple of ways we could address this:
  - For this example we could simply have LoadObjectField and LoadObjectSmi
bytecodes.
  - If we have more statically known types we could have a single
LoadObjectField with an operand that describes the statically known type
  - If there is more optimizations we could do with better feedback, we could
add a feedback slot to LoadObjectSmi and dynanically update it with the types
seen in the field.

Let's figure out if there are any other cases where this bytecode would be
useful and choose the option which suites best.

[mvstanton, 2017-05-19 08:56:41]

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

[mvstanton, 2017-05-19 08:56:46]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2017-05-19 08:56:47]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2889973002/#ps20001
(title: "REBASE.")

[commit-bot: I haz the power, 2017-05-19 08:56:56]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 09:27:26]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495184206895650,
"parent_rev": "2f92e9eb6b730dd680fda745dacc935dfc9e39fc", "commit_rev":
"0980d759f5b8bcc8bf1e8dd5c1941324ad9636b6"}

[commit-bot: I haz the power, 2017-05-19 09:27:32]

Description was changed from

==========
[Interpreter] Handle various generator intrinsics

We can avoid some runtime calls by implementing intrinsics.

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

to

==========
[Interpreter] Handle various generator intrinsics

We can avoid some runtime calls by implementing intrinsics.

BUG=

Review-Url: https://codereview.chromium.org/2889973002
Cr-Commit-Position: refs/heads/master@{#45418}
Committed:
https://chromium.googlesource.com/v8/v8/+/0980d759f5b8bcc8bf1e8dd5c1941324ad9...
==========

[commit-bot: I haz the power, 2017-05-19 09:27:33]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/0980d759f5b8bcc8bf1e8dd5c1941324ad9...

[mvstanton, 2017-05-19 09:45:34]

Yep, sounds good! I could also imagine "hint" immediate values that allow us to
keep an optimized/slimmed-down bytecode and recover full type/context
information later. That should keep most of the savings and protect downwind
compilation as well.