[Interpreter] Move context chain search loop to handler

[Interpreter] Move context chain search loop to handler

Moves the context chain search loop out of generated bytecode, and into
the (Lda|Ldr|Sda)ContextSlot handler, by passing the context depth in as
an additional operand. This should decrease the bytecode size and
increase performance for deep context chain searches, at the cost of
slightly increasing bytecode size for shallow context access.

Committed: https://crrev.com/1c0c5fda2699cba9eea89b6d250a3a52d1965198
Cr-Commit-Position: refs/heads/master@{#39378}

[Leszek, 2016-09-12 16:16:02]

leszeks@google.com changed reviewers:
+ leszeks@google.com, mythria@chromium.org, rmcilroy@chromium.org

[Leszek, 2016-09-12 16:16:02]

mythria@chromium.org: Please review changes in 

rmcilroy@chromium.org: Please review changes in

[rmcilroy, 2016-09-12 17:07:15]

Looks great. Thanks for the fast CL. Just some nits.

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:926:
builder()->LoadTheHole().StoreContextSlot(execution_context()->reg(),
Could you add the zero depth DCHECK here (same as line 964

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:109: OperandType::kIdx, OperandType::kIdx)          
                           \
This should really be a different type (an unsigned immediate rather than an
idx) but with the same properties. Klaas has a todo in this file to add
something similar. Could you do this as a follow-up CL please.

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:526: // Fast path if the depth is 0
Nit - full stop on comments.

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:540: context.Bind(__
LoadContextSlot(context.value(), Context::PREVIOUS_INDEX));
Nit - swapping these around might give slightly better codegen (merge subtract
instructions modification of flags with branch check). You can see generated
code with --trace-ignition-codegen or some similarly named flag)

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:542: __ Branch(__ WordEqual(cur_depth.value(), __
Int32Constant(0)),
You can do BranchIfWordEqual(...)

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:557: return __
LoadContextSlot(GetContextAtDepth(assembler, root_context, depth),
Nit - do the GetContextAtDepth on a different line to make the schedule of the
code obvious.

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:561: // LdaContextSlot <context> <slot_index>
Please update these comments (with <depth> and /s/context/root_context/

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:590: __
StoreContextSlot(GetContextAtDepth(assembler, root_context, depth),
Ditto

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/interpreter.h
File src/interpreter/interpreter.h (right):

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.h:141: compiler::Node*
GetContextAtDepth(InterpreterAssembler* assembler,
This can probably live in interpreter-assembler since its a fairly generic thing
to have.

[Leszek Swirski, 2016-09-13 09:42:35]

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:926:
builder()->LoadTheHole().StoreContextSlot(execution_context()->reg(),
On 2016/09/12 17:07:15, rmcilroy wrote:
> Could you add the zero depth DCHECK here (same as line 964

Done.

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:109: OperandType::kIdx, OperandType::kIdx)          
                           \
On 2016/09/12 17:07:15, rmcilroy wrote:
> This should really be a different type (an unsigned immediate rather than an
> idx) but with the same properties. Klaas has a todo in this file to add
> something similar. Could you do this as a follow-up CL please.

Acknowledged.

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:526: // Fast path if the depth is 0
On 2016/09/12 17:07:15, rmcilroy wrote:
> Nit - full stop on comments.

Done.

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:540: context.Bind(__
LoadContextSlot(context.value(), Context::PREVIOUS_INDEX));
On 2016/09/12 17:07:15, rmcilroy wrote:
> Nit - swapping these around might give slightly better codegen (merge subtract
> instructions modification of flags with branch check). You can see generated
> code with --trace-ignition-codegen or some similarly named flag)

Sadly there's no improvement in the codegen, but agree that there could be, so
I'll leave this reordered in case that optimisation comes along later.

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:542: __ Branch(__ WordEqual(cur_depth.value(), __
Int32Constant(0)),
On 2016/09/12 17:07:15, rmcilroy wrote:
> You can do BranchIfWordEqual(...)

Done.

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:557: return __
LoadContextSlot(GetContextAtDepth(assembler, root_context, depth),
On 2016/09/12 17:07:15, rmcilroy wrote:
> Nit - do the GetContextAtDepth on a different line to make the schedule of the
> code obvious.

Done.

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:561: // LdaContextSlot <context> <slot_index>
On 2016/09/12 17:07:15, rmcilroy wrote:
> Please update these comments (with <depth> and /s/context/root_context/

Done.

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:590: __
StoreContextSlot(GetContextAtDepth(assembler, root_context, depth),
On 2016/09/12 17:07:15, rmcilroy wrote:
> Ditto

Done.

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/interpreter.h
File src/interpreter/interpreter.h (right):

https://codereview.chromium.org/2336643002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.h:141: compiler::Node*
GetContextAtDepth(InterpreterAssembler* assembler,
On 2016/09/12 17:07:15, rmcilroy wrote:
> This can probably live in interpreter-assembler since its a fairly generic
thing
> to have.

Done.

[Michael Starzinger, 2016-09-13 09:53:07]

mstarzinger@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Michael Starzinger, 2016-09-13 09:53:08]

Looking good. Just nits.

https://codereview.chromium.org/2336643002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2336643002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:97: // Load the object at |slot_index|
at |depth| in |context| into the
nit: "... at |depth| in the context chain starting with |context| ..."

https://codereview.chromium.org/2336643002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:102: // Stores the object in the
accumulator into |slot_index| at |depth| of
nit: Likewise.

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

https://codereview.chromium.org/2336643002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:87: Node*
InterpreterAssembler::GetContextAtDepth(Node* root_context, Node* depth) {
nit: Likewise about "root_context" vs "context" here and throughout this file.

https://codereview.chromium.org/2336643002/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter-assembler.h (right):

https://codereview.chromium.org/2336643002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.h:57: compiler::Node*
GetContextAtDepth(compiler::Node* root_context,
nit: Likewise about "root_context" vs "context" here.

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

https://codereview.chromium.org/2336643002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:521: Node* root_context = __
LoadRegister(reg_index);
nit: Likewise about "root_context" vs "context" here and throughout this file.

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

https://codereview.chromium.org/2336643002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.h:140: // |root_context|.
nit: The name "root_context" sounds misleading because "root" sounds as it would
be the end of the chain (i.e. the native context). Please just use plain
"context" for the variable/parameter name instead. This is in sync with the rest
of the code.

[Leszek Swirski, 2016-09-13 10:17:51]

https://codereview.chromium.org/2336643002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2336643002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:97: // Load the object at |slot_index|
at |depth| in |context| into the
On 2016/09/13 09:53:08, Michael Starzinger wrote:
> nit: "... at |depth| in the context chain starting with |context| ..."

Done.

https://codereview.chromium.org/2336643002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:102: // Stores the object in the
accumulator into |slot_index| at |depth| of
On 2016/09/13 09:53:08, Michael Starzinger wrote:
> nit: Likewise.

Done, and done wherever else I added a depth parameter.

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

https://codereview.chromium.org/2336643002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:87: Node*
InterpreterAssembler::GetContextAtDepth(Node* root_context, Node* depth) {
On 2016/09/13 09:53:08, Michael Starzinger wrote:
> nit: Likewise about "root_context" vs "context" here and throughout this file.

Done.

https://codereview.chromium.org/2336643002/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter-assembler.h (right):

https://codereview.chromium.org/2336643002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.h:57: compiler::Node*
GetContextAtDepth(compiler::Node* root_context,
On 2016/09/13 09:53:08, Michael Starzinger wrote:
> nit: Likewise about "root_context" vs "context" here.

Done.

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

https://codereview.chromium.org/2336643002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:521: Node* root_context = __
LoadRegister(reg_index);
On 2016/09/13 09:53:08, Michael Starzinger wrote:
> nit: Likewise about "root_context" vs "context" here and throughout this file.

Done.

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

https://codereview.chromium.org/2336643002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.h:140: // |root_context|.
On 2016/09/13 09:53:08, Michael Starzinger wrote:
> nit: The name "root_context" sounds misleading because "root" sounds as it
would
> be the end of the chain (i.e. the native context). Please just use plain
> "context" for the variable/parameter name instead. This is in sync with the
rest
> of the code.

Done (actually, this was a leftover definition from a refactor, so removed
entirely).

[mythria, 2016-09-13 10:21:21]

Nice, lgtm.

[rmcilroy, 2016-09-13 10:32:23]

lgtm, thanks

[Michael Starzinger, 2016-09-13 10:35:49]

LGTM. Thanks!

[Leszek, 2016-09-13 10:39:35]

The CQ bit was checked by leszeks@google.com

[commit-bot: I haz the power, 2016-09-13 10:39:40]

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-09-13 11:09:07]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-13 11:09:42]

Description was changed from

==========
[Interpreter] Move context chain search loop to handler

Moves the context chain search loop out of generated bytecode, and into
the (Lda|Ldr|Sda)ContextSlot handler, by passing the context depth in as
an additional operand. This should decrease the bytecode size and
increase performance for deep context chain searches, at the cost of
slightly increasing bytecode size for shallow context access.
==========

to

==========
[Interpreter] Move context chain search loop to handler

Moves the context chain search loop out of generated bytecode, and into
the (Lda|Ldr|Sda)ContextSlot handler, by passing the context depth in as
an additional operand. This should decrease the bytecode size and
increase performance for deep context chain searches, at the cost of
slightly increasing bytecode size for shallow context access.

Committed: https://crrev.com/1c0c5fda2699cba9eea89b6d250a3a52d1965198
Cr-Commit-Position: refs/heads/master@{#39378}
==========

[commit-bot: I haz the power, 2016-09-13 11:09:44]

Patchset 3 (id:??) landed as
https://crrev.com/1c0c5fda2699cba9eea89b6d250a3a52d1965198
Cr-Commit-Position: refs/heads/master@{#39378}