[Interpreter] Implement ForIn in bytecode graph builder.

[Interpreter] Implement ForIn in bytecode graph builder.

A pre-requisite for this change was changing the interpreter to use
Runtime::ForInStep to bring the interpreter implementation closer
to the turbofan implementation. Also required to flatten out the
cache parameters into the interpreter frame for de-opt.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/4926be6eb21e1add4144eba0a398a28f289ce2d5
Cr-Commit-Position: refs/heads/master@{#32986}

[oth, 2015-12-16 09:59:02]

oth@chromium.org changed reviewers:
+ bmeurer@chromium.org, rmcilroy@chromium.org

[oth, 2015-12-16 09:59:03]

rmcilroy@chromium.org: Please review changes all over.

bmeurer@chromium.org: Please review changes in the compiler.

[Benedikt Meurer, 2015-12-16 10:53:25]

Looking into compiler.

https://codereview.chromium.org/1531693002/diff/1/src/compiler/bytecode-graph...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1531693002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:1375:
javascript()->CallRuntime(Runtime::kInterpreterForInPrepare, 2);
What about JSForInPrepare? Calling interpreter runtime functions from TurboFan
doesn't look right to me.

[oth, 2015-12-16 16:29:57]

On 2015/12/16 10:53:25, Benedikt Meurer wrote:
> Looking into compiler.
> 
>
https://codereview.chromium.org/1531693002/diff/1/src/compiler/bytecode-graph...
> File src/compiler/bytecode-graph-builder.cc (right):
> 
>
https://codereview.chromium.org/1531693002/diff/1/src/compiler/bytecode-graph...
> src/compiler/bytecode-graph-builder.cc:1375:
> javascript()->CallRuntime(Runtime::kInterpreterForInPrepare, 2);
> What about JSForInPrepare? Calling interpreter runtime functions from TurboFan
> doesn't look right to me.

Thanks, yes, fixed. Reached the mental deadzone when coding this and trying to
line up all the pieces.

[oth, 2015-12-16 17:04:11]

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

[commit-bot: I haz the power, 2015-12-16 17:04:22]

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

[commit-bot: I haz the power, 2015-12-16 17:25:52]

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

[commit-bot: I haz the power, 2015-12-16 17:25:53]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2015-12-16 20:00:14]

https://codereview.chromium.org/1531693002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1531693002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:876: loop_builder.BreakIfUndefined();
This looks wrong/unnecessary (now). As far as I can tell the ForInPrepare sets
the accumulator to the FixedArray returned by the interpreter wrapper for the
runtime call, so it's for sure never undefined.

https://codereview.chromium.org/1531693002/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1531693002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1463: Node* result = __
CallRuntime(Runtime::kInterpreterForInPrepare, receiver,
Quick question: Long-term you plan to implement all these ForIn primitives w/o
runtime calls? Just like JSTypedLowering in case of TurboFan does, right?
(modulo the unavoidable GetPropertyNamesFast ofc).

[oth, 2015-12-16 21:50:20]

On 2015/12/16 20:00:14, Benedikt Meurer wrote:
>
https://codereview.chromium.org/1531693002/diff/20001/src/interpreter/bytecod...
> File src/interpreter/bytecode-generator.cc (right):
> 
>
https://codereview.chromium.org/1531693002/diff/20001/src/interpreter/bytecod...
> src/interpreter/bytecode-generator.cc:876: loop_builder.BreakIfUndefined();
> This looks wrong/unnecessary (now). As far as I can tell the ForInPrepare sets
> the accumulator to the FixedArray returned by the interpreter wrapper for the
> runtime call, so it's for sure never undefined.

Yes, agree. There should be a re-vamp tomorrow which moves all of the logic on
the interpreter side into InterpreterForInPrepare. Sorry for kicking off the
review process too early here.

>
https://codereview.chromium.org/1531693002/diff/20001/src/interpreter/interpr...
> File src/interpreter/interpreter.cc (right):
> 
>
https://codereview.chromium.org/1531693002/diff/20001/src/interpreter/interpr...
> src/interpreter/interpreter.cc:1463: Node* result = __
> CallRuntime(Runtime::kInterpreterForInPrepare, receiver,
> Quick question: Long-term you plan to implement all these ForIn primitives w/o
> runtime calls? Just like JSTypedLowering in case of TurboFan does, right?
> (modulo the unavoidable GetPropertyNamesFast ofc).

It should be doable in the interpreter-assembler, but the logic is a bit beyond
what it handles today. It might make a good candidate for driving functionality
there. I can't see us doing this until it's a significant perf offender. If
someone takes it on pro-actively, that would be great.

[oth, 2015-12-17 14:46:56]

I think this finally unifies handling of ForInPrepare. PTAL.

More tests to follow in test-run-bytecode-graph-builder, but passes all so far.

[rmcilroy, 2015-12-17 15:43:27]

Looks good apart from deoptimization comment.

https://codereview.chromium.org/1531693002/diff/40001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1531693002/diff/40001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:1376: AddEmptyFrameStateInputs(prepare);
Hmm, this is a bit troubling for deopt - you have two sets of frame states,
which presumably means there are two potential deopt points within the same
bytecode. Not sure if that would work - I think we still need to ToObject as a
separate bytecode (sorry...).

https://codereview.chromium.org/1531693002/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1531693002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1489: // TODO(oth): Implement in interpreter
assembler.
Not sure what this TODO means? What do you want to implement in the interpreter
assembler, what you have here looks fine (or do you mean "Implement inline in
bytecode handler instead of calling out to runtime" ?

https://codereview.chromium.org/1531693002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1504: // TODO(oth): Implement in interpreter
assembler.
ditto

https://codereview.chromium.org/1531693002/diff/40001/src/runtime/runtime-int...
File src/runtime/runtime-interpreter.cc (right):

https://codereview.chromium.org/1531693002/diff/40001/src/runtime/runtime-int...
src/runtime/runtime-interpreter.cc:172: // Initialize return return triple such
that cache_length is zero
extra "return"

https://codereview.chromium.org/1531693002/diff/40001/src/runtime/runtime-int...
src/runtime/runtime-interpreter.cc:173: // which'll coax ForInDone to quit on
first attempt.
s/first attempt/first attempt if this function returns early/

https://codereview.chromium.org/1531693002/diff/40001/src/runtime/runtime-int...
src/runtime/runtime-interpreter.cc:177: // Cast to accumulator
/a/accumulator/object

[oth, 2015-12-17 19:28:24]

Thanks, all done. It stinks, but can't think of a good excuse, it's just
horrible.

https://codereview.chromium.org/1531693002/diff/40001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1531693002/diff/40001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:1376: AddEmptyFrameStateInputs(prepare);
On 2015/12/17 15:43:26, rmcilroy wrote:
> Hmm, this is a bit troubling for deopt - you have two sets of frame states,
> which presumably means there are two potential deopt points within the same
> bytecode. Not sure if that would work - I think we still need to ToObject as a
> separate bytecode (sorry...).

NP. Will re-spin shortly. The interpreter needs a wrapper around
Runtime_ToObject as the latter can throw.

https://codereview.chromium.org/1531693002/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1531693002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1489: // TODO(oth): Implement in interpreter
assembler.
On 2015/12/17 15:43:26, rmcilroy wrote:
> Not sure what this TODO means? What do you want to implement in the
interpreter
> assembler, what you have here looks fine (or do you mean "Implement inline in
> bytecode handler instead of calling out to runtime" ?

There are among the easiest bytecodes to write in interpreter assembler, smi
inc, and test two smi's for equality. However, I'd like to have lightweight
bytecode instrumentation before we do this so we can see the cycles saved for
the work.

https://codereview.chromium.org/1531693002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1504: // TODO(oth): Implement in interpreter
assembler.
On 2015/12/17 15:43:27, rmcilroy wrote:
> ditto

ditto.

https://codereview.chromium.org/1531693002/diff/40001/src/runtime/runtime-int...
File src/runtime/runtime-interpreter.cc (right):

https://codereview.chromium.org/1531693002/diff/40001/src/runtime/runtime-int...
src/runtime/runtime-interpreter.cc:172: // Initialize return return triple such
that cache_length is zero
On 2015/12/17 15:43:27, rmcilroy wrote:
> extra "return"

Done.

https://codereview.chromium.org/1531693002/diff/40001/src/runtime/runtime-int...
src/runtime/runtime-interpreter.cc:173: // which'll coax ForInDone to quit on
first attempt.
On 2015/12/17 15:43:27, rmcilroy wrote:
> s/first attempt/first attempt if this function returns early/ 

Done.

https://codereview.chromium.org/1531693002/diff/40001/src/runtime/runtime-int...
src/runtime/runtime-interpreter.cc:177: // Cast to accumulator
On 2015/12/17 15:43:27, rmcilroy wrote:
> /a/accumulator/object

Done.

[Benedikt Meurer, 2015-12-18 07:55:50]

https://codereview.chromium.org/1531693002/diff/80001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1531693002/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:1378: // have already cast the object to
JSReceiver, but JSForInPrepare
Hm, I don't understand this comment. JSForInPrepare will not perform the
ToObject itself, but assumes that the input is a receiver.

https://codereview.chromium.org/1531693002/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1531693002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:871: // ForInPrepare we need to cast first
in case it de-opts.
Same here. I'm not sure what that comment is supposed to say.
Also it's not really a "cast", but a "type conversion".

[oth, 2015-12-18 09:00:02]

On 2015/12/18 07:55:50, Benedikt Meurer wrote:
>
https://codereview.chromium.org/1531693002/diff/80001/src/compiler/bytecode-g...
> File src/compiler/bytecode-graph-builder.cc (right):
> 
>
https://codereview.chromium.org/1531693002/diff/80001/src/compiler/bytecode-g...
> src/compiler/bytecode-graph-builder.cc:1378: // have already cast the object
to
> JSReceiver, but JSForInPrepare
> Hm, I don't understand this comment. JSForInPrepare will not perform the
> ToObject itself, but assumes that the input is a receiver.

The comment was not very good. In the previous patchset the ToObject
was inside the runtime implementation of ForInPrepare. This simplified life
because it
meant the runtime ToObject could be called and any JS exception swallowed in C++
and simply
reported as failure. The approach doesn't cut the mustard because it'd require
two de-opt points
for ForInPrepare.

The bytecode for ToObject uses a special interpreter version as we don't have
support for catching
exceptions so can't safely call the default runtime_ToObject.

>
https://codereview.chromium.org/1531693002/diff/80001/src/interpreter/bytecod...
> File src/interpreter/bytecode-generator.cc (right):
> 
>
https://codereview.chromium.org/1531693002/diff/80001/src/interpreter/bytecod...
> src/interpreter/bytecode-generator.cc:871: // ForInPrepare we need to cast
first
> in case it de-opts.
> Same here. I'm not sure what that comment is supposed to say.
> Also it's not really a "cast", but a "type conversion".

Okay, removed.

[oth, 2015-12-18 16:01:47]

On 2015/12/18 09:00:02, oth wrote:
> On 2015/12/18 07:55:50, Benedikt Meurer wrote:
> >
>
https://codereview.chromium.org/1531693002/diff/80001/src/compiler/bytecode-g...
> > File src/compiler/bytecode-graph-builder.cc (right):
> > 
> >
>
https://codereview.chromium.org/1531693002/diff/80001/src/compiler/bytecode-g...
> > src/compiler/bytecode-graph-builder.cc:1378: // have already cast the object
> to
> > JSReceiver, but JSForInPrepare
> > Hm, I don't understand this comment. JSForInPrepare will not perform the
> > ToObject itself, but assumes that the input is a receiver.
> 
> The comment was not very good. In the previous patchset the ToObject
> was inside the runtime implementation of ForInPrepare. This simplified life
> because it
> meant the runtime ToObject could be called and any JS exception swallowed in
C++
> and simply
> reported as failure. The approach doesn't cut the mustard because it'd require
> two de-opt points
> for ForInPrepare.
> 
> The bytecode for ToObject uses a special interpreter version as we don't have
> support for catching
> exceptions so can't safely call the default runtime_ToObject.
> 
> >
>
https://codereview.chromium.org/1531693002/diff/80001/src/interpreter/bytecod...
> > File src/interpreter/bytecode-generator.cc (right):
> > 
> >
>
https://codereview.chromium.org/1531693002/diff/80001/src/interpreter/bytecod...
> > src/interpreter/bytecode-generator.cc:871: // ForInPrepare we need to cast
> first
> > in case it de-opts.
> > Same here. I'm not sure what that comment is supposed to say.
> > Also it's not really a "cast", but a "type conversion".
> 
> Okay, removed.

The CL needs updating following the de-opt changes landing. There's one kink
remaining which I've yet to dig into.
Thanks

[oth, 2015-12-18 16:01:47]

On 2015/12/18 09:00:02, oth wrote:
> On 2015/12/18 07:55:50, Benedikt Meurer wrote:
> >
>
https://codereview.chromium.org/1531693002/diff/80001/src/compiler/bytecode-g...
> > File src/compiler/bytecode-graph-builder.cc (right):
> > 
> >
>
https://codereview.chromium.org/1531693002/diff/80001/src/compiler/bytecode-g...
> > src/compiler/bytecode-graph-builder.cc:1378: // have already cast the object
> to
> > JSReceiver, but JSForInPrepare
> > Hm, I don't understand this comment. JSForInPrepare will not perform the
> > ToObject itself, but assumes that the input is a receiver.
> 
> The comment was not very good. In the previous patchset the ToObject
> was inside the runtime implementation of ForInPrepare. This simplified life
> because it
> meant the runtime ToObject could be called and any JS exception swallowed in
C++
> and simply
> reported as failure. The approach doesn't cut the mustard because it'd require
> two de-opt points
> for ForInPrepare.
> 
> The bytecode for ToObject uses a special interpreter version as we don't have
> support for catching
> exceptions so can't safely call the default runtime_ToObject.
> 
> >
>
https://codereview.chromium.org/1531693002/diff/80001/src/interpreter/bytecod...
> > File src/interpreter/bytecode-generator.cc (right):
> > 
> >
>
https://codereview.chromium.org/1531693002/diff/80001/src/interpreter/bytecod...
> > src/interpreter/bytecode-generator.cc:871: // ForInPrepare we need to cast
> first
> > in case it de-opts.
> > Same here. I'm not sure what that comment is supposed to say.
> > Also it's not really a "cast", but a "type conversion".
> 
> Okay, removed.

The CL needs updating following the de-opt changes landing. There's one kink
remaining which I've yet to dig into.
Thanks

[rmcilroy, 2015-12-18 16:01:55]

LGTM once Benedikt is happy.

https://codereview.chromium.org/1531693002/diff/120001/src/interpreter/interp...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1531693002/diff/120001/src/interpreter/interp...
src/interpreter/interpreter.cc:1528: Node* object = __ GetAccumulator();
nit - could you change this to get the receiver from the input register instead
of the accumulator to avoid it being input twice?

https://codereview.chromium.org/1531693002/diff/120001/src/runtime/runtime-in...
File src/runtime/runtime-interpreter.cc (right):

https://codereview.chromium.org/1531693002/diff/120001/src/runtime/runtime-in...
src/runtime/runtime-interpreter.cc:179: if (receiver->IsNull() ||
receiver->IsUndefined()) {
This won't be null given you have the branch in the bytecode. Can you remove the
JumpIfNull bytecode from the generated code (I'm fine if this is a todo for a
followup CL).

[rmcilroy, 2015-12-18 16:01:56]

LGTM once Benedikt is happy.

[Benedikt Meurer, 2015-12-18 20:19:00]

LGTM once test failures are addressed.

[oth, 2015-12-20 15:25:20]

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

[commit-bot: I haz the power, 2015-12-20 15:25:27]

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

[commit-bot: I haz the power, 2015-12-20 15:42:12]

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

[commit-bot: I haz the power, 2015-12-20 15:42:12]

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

[oth, 2015-12-21 10:05:53]

Thanks for the feedback and patience with this CL. Quite an interesting set of
constraints. The tests all pass again and will land later today unless there's
any further feedback.

https://codereview.chromium.org/1531693002/diff/120001/src/interpreter/interp...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1531693002/diff/120001/src/interpreter/interp...
src/interpreter/interpreter.cc:1528: Node* object = __ GetAccumulator();
On 2015/12/18 16:01:55, rmcilroy wrote:
> nit - could you change this to get the receiver from the input register
instead
> of the accumulator to avoid it being input twice?

Removed the receiver argument. This cleans up the + 1 offset in the updating of
the registers.

https://codereview.chromium.org/1531693002/diff/120001/src/runtime/runtime-in...
File src/runtime/runtime-interpreter.cc (right):

https://codereview.chromium.org/1531693002/diff/120001/src/runtime/runtime-in...
src/runtime/runtime-interpreter.cc:179: if (receiver->IsNull() ||
receiver->IsUndefined()) {
On 2015/12/18 16:01:55, rmcilroy wrote:
> This won't be null given you have the branch in the bytecode. Can you remove
the
> JumpIfNull bytecode from the generated code (I'm fine if this is a todo for a
> followup CL).

Done. This was stale.

[oth, 2015-12-21 13:29:44]

The CQ bit was checked by oth@chromium.org

[oth, 2015-12-21 13:29:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1531693002/#ps200001
(title: "Minor clean-up/simplication in Runtime_InterpreterForInPrepare.")

[commit-bot: I haz the power, 2015-12-21 13:29:49]

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

[commit-bot: I haz the power, 2015-12-21 13:30:51]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-12-21 13:31:18]

Description was changed from

==========
[Interpreter] Implement ForIn in bytecode graph builder.

A pre-requisite for this change was changing the interpreter to use
Runtime::ForInStep to bring the interpreter implementation closer
to the turbofan implementation. Also required to flatten out the
cache parameters into the interpreter frame for de-opt.

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Implement ForIn in bytecode graph builder.

A pre-requisite for this change was changing the interpreter to use
Runtime::ForInStep to bring the interpreter implementation closer
to the turbofan implementation. Also required to flatten out the
cache parameters into the interpreter frame for de-opt.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/4926be6eb21e1add4144eba0a398a28f289ce2d5
Cr-Commit-Position: refs/heads/master@{#32986}
==========

[commit-bot: I haz the power, 2015-12-21 13:31:19]

Patchset 11 (id:??) landed as
https://crrev.com/4926be6eb21e1add4144eba0a398a28f289ce2d5
Cr-Commit-Position: refs/heads/master@{#32986}