[interpreter] Update ForInPrepare to conditionally use runtime.

[interpreter] Update ForInPrepare to conditionally use runtime.

Copies the behaviour of FullCode in attempting to get the state for
ForInPrepare inline and falling back to the runtime if necessary.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/263131cf2afd3488e2cf47fe1063f7738620783e
Cr-Commit-Position: refs/heads/master@{#37874}

[oth, 2016-07-18 12:57:19]

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

[commit-bot: I haz the power, 2016-07-18 12:57:27]

Dry run: 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, 2016-07-18 13:17:49]

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

[commit-bot: I haz the power, 2016-07-18 13:17:49]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[oth, 2016-07-19 09:53:23]

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

[commit-bot: I haz the power, 2016-07-19 09:53:32]

Dry run: 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, 2016-07-19 09:55:01]

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

[commit-bot: I haz the power, 2016-07-19 09:55:02]

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

[oth, 2016-07-19 09:58:15]

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

[commit-bot: I haz the power, 2016-07-19 09:58:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[oth, 2016-07-19 10:23:54]

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

[oth, 2016-07-19 10:23:55]

rmcilroy, PTAL, thanks and apologies in advance :-)

[commit-bot: I haz the power, 2016-07-19 10:29:34]

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

[commit-bot: I haz the power, 2016-07-19 10:29:35]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-07-19 11:08:54]

Overall looks good, thanks!

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

https://codereview.chromium.org/2155153002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:639: void
InterpreterAssembler::CheckEnumCache(Node* receiver, Label* use_cache,
Could we move these both to CodeStubAssembler - they aren't specific to the
interpreter, and might be useful elsewhere.

(random thought - perhaps we want to make this a ForInPrepareStub which is
called by TF in the generic lowering, and inlined by the interpreter -  just a
thought, we don't need to do it here).

https://codereview.chromium.org/2155153002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:649: Label start(this, 2, loop_vars);
nit - start->loop

https://codereview.chromium.org/2155153002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:662: Label next(this);
nit - move definition next to start

https://codereview.chromium.org/2155153002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:664: // Check that there are no
elements. prototype contains the current JS
/s/prototype/|current_js_object| ?

https://codereview.chromium.org/2155153002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:672: BranchIf(WordEqual(elements,
empty_fixed_array), &if_no_elements,
You can do BranchIfWordEqual if you like.

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

https://codereview.chromium.org/2155153002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1866: nothing_to_iterate(assembler,
Label::kDeferred),
Just wondering if nothing_to_iterate should be kDeferred? It doesn't involve a
call (so doesn't cause the fast-path to build a frame), but is probably not a
common occurrence, so might still be worth being deferred. Was there a reason it
was made deferred?

https://codereview.chromium.org/2155153002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1876:
assembler->Int32GreaterThanOrEqual(instance_type, first_receiver_type),
There is a BranchIfInt32GreaterThanOrEqual (I think).

https://codereview.chromium.org/2155153002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1881: __ BranchIf(assembler->WordEqual(object,
assembler->NullConstant()),
BranchIfWordEqual (and below)

https://codereview.chromium.org/2155153002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1891: __ Bind(&convert_to_receiver);
I'm assuming this is relatively uncommon. If so, could this be kDeferred? That
might allow us to avoid building a frame in the fast-path.

https://codereview.chromium.org/2155153002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1906: Label use_enum_cache(assembler),
use_runtime(assembler);
make use_runtime kDeferred.

[oth, 2016-07-19 12:58:01]

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

[commit-bot: I haz the power, 2016-07-19 12:58:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[oth, 2016-07-19 12:59:25]

Thanks. All incorporated. The code has kept nothing_to_iterate on the deferred
path. Happy to move it, but suspect it's off the main path.

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

https://codereview.chromium.org/2155153002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:639: void
InterpreterAssembler::CheckEnumCache(Node* receiver, Label* use_cache,
On 2016/07/19 11:08:54, rmcilroy wrote:
> Could we move these both to CodeStubAssembler - they aren't specific to the
> interpreter, and might be useful elsewhere.
> 
> (random thought - perhaps we want to make this a ForInPrepareStub which is
> called by TF in the generic lowering, and inlined by the interpreter -  just a
> thought, we don't need to do it here).

Done.

https://codereview.chromium.org/2155153002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:649: Label start(this, 2, loop_vars);
On 2016/07/19 11:08:54, rmcilroy wrote:
> nit - start->loop

Done.

https://codereview.chromium.org/2155153002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:662: Label next(this);
On 2016/07/19 11:08:54, rmcilroy wrote:
> nit - move definition next to start

Done.

https://codereview.chromium.org/2155153002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:664: // Check that there are no
elements. prototype contains the current JS
On 2016/07/19 11:08:54, rmcilroy wrote:
> /s/prototype/|current_js_object| ?

Done.

https://codereview.chromium.org/2155153002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:672: BranchIf(WordEqual(elements,
empty_fixed_array), &if_no_elements,
On 2016/07/19 11:08:54, rmcilroy wrote:
> You can do BranchIfWordEqual if you like.

Done.

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

https://codereview.chromium.org/2155153002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1866: nothing_to_iterate(assembler,
Label::kDeferred),
On 2016/07/19 11:08:54, rmcilroy wrote:
> Just wondering if nothing_to_iterate should be kDeferred? It doesn't involve a
> call (so doesn't cause the fast-path to build a frame), but is probably not a
> common occurrence, so might still be worth being deferred. Was there a reason
it
> was made deferred?

I expect this is the lesser trodden path - it's deferred to reduce the sharing
i-cache entries that are used more frequently.

https://codereview.chromium.org/2155153002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1876:
assembler->Int32GreaterThanOrEqual(instance_type, first_receiver_type),
On 2016/07/19 11:08:54, rmcilroy wrote:
> There is a BranchIfInt32GreaterThanOrEqual (I think).

Thanks! Done.

https://codereview.chromium.org/2155153002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1881: __ BranchIf(assembler->WordEqual(object,
assembler->NullConstant()),
On 2016/07/19 11:08:54, rmcilroy wrote:
> BranchIfWordEqual (and below)

Done.

https://codereview.chromium.org/2155153002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1891: __ Bind(&convert_to_receiver);
On 2016/07/19 11:08:54, rmcilroy wrote:
> I'm assuming this is relatively uncommon. If so, could this be kDeferred? That
> might allow us to avoid building a frame in the fast-path.

Done.

https://codereview.chromium.org/2155153002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1906: Label use_enum_cache(assembler),
use_runtime(assembler);
On 2016/07/19 11:08:54, rmcilroy wrote:
> make use_runtime kDeferred.

Done.

[rmcilroy, 2016-07-19 14:02:21]

LGTM, thanks.

[oth, 2016-07-19 14:07:09]

The CQ bit was checked by oth@chromium.org

[commit-bot: I haz the power, 2016-07-19 14:07:12]

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, 2016-07-19 14:46:33]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-19 14:46:59]

Description was changed from

==========
[interpreter] Update ForInPrepare to conditionally use runtime.

Copies the behaviour of FullCode in attempting to get the state for
ForInPrepare inline and falling back to the runtime if necessary.

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

to

==========
[interpreter] Update ForInPrepare to conditionally use runtime.

Copies the behaviour of FullCode in attempting to get the state for
ForInPrepare inline and falling back to the runtime if necessary.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/263131cf2afd3488e2cf47fe1063f7738620783e
Cr-Commit-Position: refs/heads/master@{#37874}
==========

[commit-bot: I haz the power, 2016-07-19 14:47:00]

Patchset 5 (id:??) landed as
https://crrev.com/263131cf2afd3488e2cf47fe1063f7738620783e
Cr-Commit-Position: refs/heads/master@{#37874}