Make FastNewFunctionContextStub take slots parameter

Make FastNewFunctionContextStub take slots parameter

This will enable the interpreter to add a bytecode and use the stub.

BUG=v8:4280
LOG=n

Committed: https://crrev.com/5bc243978396ab902f115e49aff2cf4bcb5d8a4c
Cr-Commit-Position: refs/heads/master@{#38219}

[klaasb, 2016-07-25 16:10:06]

The CQ bit was checked by klaasb@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 16:10:10]

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-25 16:13:18]

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

[commit-bot: I haz the power, 2016-07-25 16:13:18]

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

[klaasb, 2016-07-25 16:55:15]

The CQ bit was checked by klaasb@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 16:55:20]

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-25 16:59:29]

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

[commit-bot: I haz the power, 2016-07-25 16:59:30]

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

[klaasb, 2016-07-25 17:58:42]

The CQ bit was checked by klaasb@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 17:58:51]

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-25 18:22:46]

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

[commit-bot: I haz the power, 2016-07-25 18:22:47]

Dry run: This issue passed the CQ dry run.

[klaasb, 2016-07-26 09:56:34]

Description was changed from

==========
Make FastNewFunctionContextStub take slots parameter

This will enable the interpreter to add a bytecode and use the stub.

BUG=
LOG=n
==========

to

==========
Make FastNewFunctionContextStub take slots parameter

This will enable the interpreter to add a bytecode and use the stub.

BUG=
LOG=n
==========

[klaasb, 2016-07-26 09:56:35]

klaasb@google.com changed reviewers:
+ bmeurer@chromium.org, rmcilroy@chromium.org

[klaasb, 2016-07-26 10:00:34]

Description was changed from

==========
Make FastNewFunctionContextStub take slots parameter

This will enable the interpreter to add a bytecode and use the stub.

BUG=
LOG=n
==========

to

==========
Make FastNewFunctionContextStub take slots parameter

This will enable the interpreter to add a bytecode and use the stub.

BUG=v8:4280
LOG=n
==========

[klaasb, 2016-07-26 17:04:52]

https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:4508: assembler->Int32Constant(Context::MIN_CONTEXT_SLOTS);
I've used int32 throughout, as it was used before and it works. However nothing
should ever be negative here and in the end the array stores further down use
IntPtr arithmetic anyway. Any reason why I shouldn't use Ptr here?

https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.h#newcod...
src/code-stubs.h:1061: static const int kMaximumSlots = 64;
I'm unsure whether/why we need to still limit this?
From here:
https://chromium.googlesource.com/v8/v8/+/013860d42efb102403207f9a1d9688509d3...
I can see that the original motivation was to limit the amount of code stubs but
with this change there will ever be only one. Get rid of it and all related
lines that branch on it and otherwise call the runtime?

[rmcilroy, 2016-07-27 11:11:39]

Overall looks good. Couple of comments and suggestions.

https://codereview.chromium.org/2177273002/diff/40001/src/arm/interface-descr...
File src/arm/interface-descriptors-arm.cc (right):

https://codereview.chromium.org/2177273002/diff/40001/src/arm/interface-descr...
src/arm/interface-descriptors-arm.cc:28: const Register
FastNewFunctionContextDescriptor::SlotsRegister() { return r0; }
I see this is used only for lithium-codegen and full-codegen. I wonder whether
the logic for context creation could be moved to hydrogen for Crankshaft use
GetCallInterfaceDescriptor().GetRegisterParameter(0) in full-codegen (e.g., see
FastNewClosureStub). 

Maybe have a chat with Michi or Benedikt about the best plan here since I don't
know the potential issues with crankshaft at all.

https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:4511: assembler->Int32Mul(length,
assembler->Int32Constant(kPointerSize)),
You could probably shift by kPointerSizeLog2 instead (although TF might already
do this optimization in instruction selection

https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:4554: &initialize_slot, &after_loop);
You could do this check on the backward branch - you are guaranteed that there
will be at least Context::MIN_CONTEXT_SLOTS

https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.h#newcod...
src/code-stubs.h:1061: static const int kMaximumSlots = 64;
On 2016/07/26 17:04:52, klaasb wrote:
> I'm unsure whether/why we need to still limit this?
> From here:
>
https://chromium.googlesource.com/v8/v8/+/013860d42efb102403207f9a1d9688509d3...
> I can see that the original motivation was to limit the amount of code stubs
but
> with this change there will ever be only one. Get rid of it and all related
> lines that branch on it and otherwise call the runtime?

Sounds reasonable to me, any reason why not to do this Benedikt?

https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.h#newcod...
src/code-stubs.h:1067: class SlotsBits : public BitField<int, 0, 8> {};
You can delete the SlotBits class now too.

[klaasb, 2016-07-27 17:06:31]

https://codereview.chromium.org/2177273002/diff/40001/src/arm/interface-descr...
File src/arm/interface-descriptors-arm.cc (right):

https://codereview.chromium.org/2177273002/diff/40001/src/arm/interface-descr...
src/arm/interface-descriptors-arm.cc:28: const Register
FastNewFunctionContextDescriptor::SlotsRegister() { return r0; }
On 2016/07/27 11:11:38, rmcilroy wrote:
> I see this is used only for lithium-codegen and full-codegen. I wonder whether
> the logic for context creation could be moved to hydrogen for Crankshaft use
> GetCallInterfaceDescriptor().GetRegisterParameter(0) in full-codegen (e.g.,
see
> FastNewClosureStub). 
> 
> Maybe have a chat with Michi or Benedikt about the best plan here since I
don't
> know the potential issues with crankshaft at all.

As agreed offline, it doesn't seem worth it and easily doable.

https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:4511: assembler->Int32Mul(length,
assembler->Int32Constant(kPointerSize)),
On 2016/07/27 11:11:38, rmcilroy wrote:
> You could probably shift by kPointerSizeLog2 instead (although TF might
already
> do this optimization in instruction selection

TF does it on the graph itself before instruction selection:
https://cs.chromium.org/chromium/src/v8/src/compiler/machine-operator-reducer...

https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:4554: &initialize_slot, &after_loop);
On 2016/07/27 11:11:38, rmcilroy wrote:
> You could do this check on the backward branch - you are guaranteed that there
> will be at least Context::MIN_CONTEXT_SLOTS

Done.

https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.h#newcod...
src/code-stubs.h:1067: class SlotsBits : public BitField<int, 0, 8> {};
On 2016/07/27 11:11:38, rmcilroy wrote:
> You can delete the SlotBits class now too. 

Done.

[rmcilroy, 2016-07-28 15:04:28]

LGTM once comments are addressed and Benedikt is happy.

https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:4511: assembler->Int32Mul(length,
assembler->Int32Constant(kPointerSize)),
On 2016/07/27 17:06:31, klaasb wrote:
> On 2016/07/27 11:11:38, rmcilroy wrote:
> > You could probably shift by kPointerSizeLog2 instead (although TF might
> already
> > do this optimization in instruction selection
> 
> TF does it on the graph itself before instruction selection:
>
https://cs.chromium.org/chromium/src/v8/src/compiler/machine-operator-reducer...

I don't think this phase is run on code generated by the code assembler though
(see Pipeline::GenerateCodeForCodeStub()), so it's probably worth doing here
explicitly.

https://codereview.chromium.org/2177273002/diff/60001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2177273002/diff/60001/src/code-stubs.cc#newco...
src/code-stubs.cc:4550: {
Please add a comment that there will always be at least
Context::MIN_CONTEXT_SLOTS so we don't need to do the check before starting the
loop.

[klaasb, 2016-07-28 15:34:47]

https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:4511: assembler->Int32Mul(length,
assembler->Int32Constant(kPointerSize)),
On 2016/07/28 15:04:27, rmcilroy wrote:
> On 2016/07/27 17:06:31, klaasb wrote:
> > On 2016/07/27 11:11:38, rmcilroy wrote:
> > > You could probably shift by kPointerSizeLog2 instead (although TF might
> > already
> > > do this optimization in instruction selection
> > 
> > TF does it on the graph itself before instruction selection:
> >
>
https://cs.chromium.org/chromium/src/v8/src/compiler/machine-operator-reducer...
> 
> I don't think this phase is run on code generated by the code assembler though
> (see Pipeline::GenerateCodeForCodeStub()), so it's probably worth doing here
> explicitly.

Done.

https://codereview.chromium.org/2177273002/diff/60001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2177273002/diff/60001/src/code-stubs.cc#newco...
src/code-stubs.cc:4550: {
On 2016/07/28 15:04:27, rmcilroy wrote:
> Please add a comment that there will always be at least
> Context::MIN_CONTEXT_SLOTS so we don't need to do the check before starting
the
> loop.

Done.

[Benedikt Meurer, 2016-07-29 03:29:16]

Nice. Some final comments to address.

https://codereview.chromium.org/2177273002/diff/80001/src/code-factory.cc
File src/code-factory.cc (right):

https://codereview.chromium.org/2177273002/diff/80001/src/code-factory.cc#new...
src/code-factory.cc:474: Callable CodeFactory::FastNewContext(Isolate* isolate)
{
Nit: Please rename the factory method to FastNewFunctionContext.

https://codereview.chromium.org/2177273002/diff/80001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2177273002/diff/80001/src/code-stubs.cc#newco...
src/code-stubs.cc:4544: Variable var_slot_index(assembler,
MachineType::PointerRepresentation());
This is MachineType::Int32() since you are using it as int32 below.

https://codereview.chromium.org/2177273002/diff/80001/src/compiler/js-generic...
File src/compiler/js-generic-lowering.cc (right):

https://codereview.chromium.org/2177273002/diff/80001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:432: node->InsertInput(zone(), 1,
jsgraph()->Int32Constant(slot_count));
I'm not sure this is correct according to the interface descriptor (same for the
FCG versions). The interface descriptor declares the slots parameter as tagged,
but you are passing it as untagged signed32 value (and also use it that way in
the stub). I think for this to work you have to change the interface descriptor
to declare the parameter as signed32.

[klaasb, 2016-07-29 09:51:36]

Benedikt, please take a short look at the kMaximumSlots question. Earlier
discussion here:
https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.h#newcod...

https://codereview.chromium.org/2177273002/diff/80001/src/code-factory.cc
File src/code-factory.cc (right):

https://codereview.chromium.org/2177273002/diff/80001/src/code-factory.cc#new...
src/code-factory.cc:474: Callable CodeFactory::FastNewContext(Isolate* isolate)
{
On 2016/07/29 03:29:16, Benedikt Meurer wrote:
> Nit: Please rename the factory method to FastNewFunctionContext.

Done.

https://codereview.chromium.org/2177273002/diff/80001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2177273002/diff/80001/src/code-stubs.cc#newco...
src/code-stubs.cc:4544: Variable var_slot_index(assembler,
MachineType::PointerRepresentation());
On 2016/07/29 03:29:16, Benedikt Meurer wrote:
> This is MachineType::Int32() since you are using it as int32 below.

Done.

https://codereview.chromium.org/2177273002/diff/80001/src/compiler/js-generic...
File src/compiler/js-generic-lowering.cc (right):

https://codereview.chromium.org/2177273002/diff/80001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:430: if (slot_count <=
FastNewFunctionContextStub::kMaximumSlots) {
What about this and similar lines elsewhere? I would remove them and the
alternate branches (calls to the runtime). See discussion in earlier patches.

https://codereview.chromium.org/2177273002/diff/80001/src/compiler/js-generic...
src/compiler/js-generic-lowering.cc:432: node->InsertInput(zone(), 1,
jsgraph()->Int32Constant(slot_count));
On 2016/07/29 03:29:16, Benedikt Meurer wrote:
> I'm not sure this is correct according to the interface descriptor (same for
the
> FCG versions). The interface descriptor declares the slots parameter as
tagged,
> but you are passing it as untagged signed32 value (and also use it that way in
> the stub). I think for this to work you have to change the interface
descriptor
> to declare the parameter as signed32.

Done.

[Benedikt Meurer, 2016-08-01 17:35:09]

LGTM with question addressed.

https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/2177273002/diff/40001/src/code-stubs.h#newcod...
src/code-stubs.h:1061: static const int kMaximumSlots = 64;
Makes sense. Please do that.

[klaasb, 2016-08-01 18:31:08]

The CQ bit was checked by klaasb@google.com

[klaasb, 2016-08-01 18:31:09]

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

[commit-bot: I haz the power, 2016-08-01 18:31:10]

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-08-01 18:32:34]

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

[commit-bot: I haz the power, 2016-08-01 18:32:35]

Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  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_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/10077)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/10059)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/10017)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/6051)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/20683)

[klaasb, 2016-08-01 19:00:59]

The CQ bit was checked by klaasb@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 19:01:07]

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

[klaasb, 2016-08-01 19:11:33]

The CQ bit was unchecked by klaasb@google.com

[klaasb, 2016-08-01 19:11:36]

The CQ bit was checked by klaasb@google.com

[klaasb, 2016-08-01 19:11:36]

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/2177273002/#ps140001
(title: "rebased")

[commit-bot: I haz the power, 2016-08-01 19:11:44]

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-08-01 19:26:54]

Description was changed from

==========
Make FastNewFunctionContextStub take slots parameter

This will enable the interpreter to add a bytecode and use the stub.

BUG=v8:4280
LOG=n
==========

to

==========
Make FastNewFunctionContextStub take slots parameter

This will enable the interpreter to add a bytecode and use the stub.

BUG=v8:4280
LOG=n
==========

[commit-bot: I haz the power, 2016-08-01 19:26:55]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-01 19:29:26]

Description was changed from

==========
Make FastNewFunctionContextStub take slots parameter

This will enable the interpreter to add a bytecode and use the stub.

BUG=v8:4280
LOG=n
==========

to

==========
Make FastNewFunctionContextStub take slots parameter

This will enable the interpreter to add a bytecode and use the stub.

BUG=v8:4280
LOG=n

Committed: https://crrev.com/5bc243978396ab902f115e49aff2cf4bcb5d8a4c
Cr-Commit-Position: refs/heads/master@{#38219}
==========

[commit-bot: I haz the power, 2016-08-01 19:29:27]

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