[interpreter] Add CreateFunctionContext bytecode

[interpreter] Add CreateFunctionContext bytecode

Add a new bytecode to create a function context. The handler inlines
FastNewFunctionContextStub.

BUG=v8:4280
LOG=n

Committed: https://crrev.com/8097eeb9f2a222540d5745d769290d65e38e7f05
Cr-Commit-Position: refs/heads/master@{#38301}

[klaasb, 2016-07-29 10:45:32]

Description was changed from

==========
[interpreter] Add CreateContext bytecode

Add a new bytecode to create a function context. The handler inlines
FastNewFunctionContextStub.

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

to

==========
[interpreter] Add CreateFunctionContext bytecode

Add a new bytecode to create a function context. The handler inlines
FastNewFunctionContextStub.

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

[klaasb, 2016-08-02 10:04:54]

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

[commit-bot: I haz the power, 2016-08-02 10:04:57]

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-08-02 10:27:07]

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

[commit-bot: I haz the power, 2016-08-02 10:27:08]

Dry run: This issue passed the CQ dry run.

[klaasb, 2016-08-02 11:22:15]

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

[Michael Starzinger, 2016-08-02 11:58:41]

https://codereview.chromium.org/2187523002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2187523002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:3014:
scope->GetScopeInfo(isolate())->ContextLength());
Using the {ScopeInfo} here looks dangerous. The slot count should be ...

int slot_count = scope->num_heap_slots() - Context::MIN_CONTEXT_SLOTS;

[Michael Starzinger, 2016-08-02 11:59:41]

LGTM modulo the one comment.

[klaasb, 2016-08-02 13:02:26]

https://codereview.chromium.org/2187523002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2187523002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:3014:
scope->GetScopeInfo(isolate())->ContextLength());
On 2016/08/02 11:58:41, Michael Starzinger wrote:
> Using the {ScopeInfo} here looks dangerous. The slot count should be ...
> 
> int slot_count = scope->num_heap_slots() - Context::MIN_CONTEXT_SLOTS;

Fixed.

[rmcilroy, 2016-08-03 10:58:14]

https://codereview.chromium.org/2187523002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/2187523002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:341:
Output(Bytecode::kCreateFunctionContext, SignedOperand(slots));
Signed? This should always be positive right? Could you make the bytecode take
an unsigned operand (we probably need to add a new type or rename the Idx type).
Feel free to do this in a followup CL and just add a TODO here

https://codereview.chromium.org/2187523002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2187523002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.h:138: // Create a new context with size
|slots| in the accumulator.
nit - drop the "in the accumulator" (slight confusion that it sounds like
|slots| should be in the accumulator, and the default is for results to be in
the accumulator.

https://codereview.chromium.org/2187523002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/2187523002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecodes.h:229: /* Context allocation */                       
                             \
nit - newline above

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

https://codereview.chromium.org/2187523002/diff/120001/src/interpreter/interp...
src/interpreter/interpreter.cc:1720: // CreateContext
Add operands here: <slots> and mention in description below.

[rmcilroy, 2016-08-03 10:58:30]

LGTM with comments in previous reply.

[klaasb, 2016-08-03 12:28:16]

https://codereview.chromium.org/2187523002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/2187523002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.cc:341:
Output(Bytecode::kCreateFunctionContext, SignedOperand(slots));
On 2016/08/03 10:58:14, rmcilroy (OOO) wrote:
> Signed? This should always be positive right? Could you make the bytecode take
> an unsigned operand (we probably need to add a new type or rename the Idx
type).
> Feel free to do this in a followup CL and just add a TODO here

Switched to unsigned and Idx in bytecodes.h, added a comment there.

https://codereview.chromium.org/2187523002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2187523002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-array-builder.h:138: // Create a new context with size
|slots| in the accumulator.
On 2016/08/03 10:58:14, rmcilroy (OOO) wrote:
> nit - drop the "in the accumulator" (slight confusion that it sounds like
> |slots| should be in the accumulator, and the default is for results to be in
> the accumulator.

Done.

https://codereview.chromium.org/2187523002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/2187523002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecodes.h:229: /* Context allocation */                       
                             \
On 2016/08/03 10:58:14, rmcilroy (OOO) wrote:
> nit - newline above

Done.

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

https://codereview.chromium.org/2187523002/diff/120001/src/interpreter/interp...
src/interpreter/interpreter.cc:1720: // CreateContext
On 2016/08/03 10:58:14, rmcilroy (OOO) wrote:
> Add operands here: <slots> and mention in description below.

Done.

[klaasb, 2016-08-03 12:28:26]

The CQ bit was checked by klaasb@google.com

[klaasb, 2016-08-03 12:28:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2187523002/#ps140001
(title: "comments")

[commit-bot: I haz the power, 2016-08-03 12:28:36]

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-03 12:45:31]

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

[commit-bot: I haz the power, 2016-08-03 12:45:32]

Try jobs failed on following builders:
  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_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[klaasb, 2016-08-03 13:25:06]

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

[commit-bot: I haz the power, 2016-08-03 13:25:14]

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-08-03 14:21:40]

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

[commit-bot: I haz the power, 2016-08-03 14:21:41]

Dry run: This issue passed the CQ dry run.

[klaasb, 2016-08-03 14:39:39]

The CQ bit was checked by klaasb@google.com

[klaasb, 2016-08-03 14:39:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2187523002/#ps160001
(title: "fix bytecode graph builder")

[commit-bot: I haz the power, 2016-08-03 14:39:55]

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-03 14:41:51]

Description was changed from

==========
[interpreter] Add CreateFunctionContext bytecode

Add a new bytecode to create a function context. The handler inlines
FastNewFunctionContextStub.

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

to

==========
[interpreter] Add CreateFunctionContext bytecode

Add a new bytecode to create a function context. The handler inlines
FastNewFunctionContextStub.

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

[commit-bot: I haz the power, 2016-08-03 14:41:52]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-08-03 14:43:36]

Description was changed from

==========
[interpreter] Add CreateFunctionContext bytecode

Add a new bytecode to create a function context. The handler inlines
FastNewFunctionContextStub.

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

to

==========
[interpreter] Add CreateFunctionContext bytecode

Add a new bytecode to create a function context. The handler inlines
FastNewFunctionContextStub.

BUG=v8:4280
LOG=n

Committed: https://crrev.com/8097eeb9f2a222540d5745d769290d65e38e7f05
Cr-Commit-Position: refs/heads/master@{#38301}
==========

[commit-bot: I haz the power, 2016-08-03 14:43:37]

Patchset 9 (id:??) landed as
https://crrev.com/8097eeb9f2a222540d5745d769290d65e38e7f05
Cr-Commit-Position: refs/heads/master@{#38301}