[interpreter] Add fast path for dynamic global lookups

[interpreter] Add fast path for dynamic global lookups

Adds a fast path for loading DYNAMIC_GLOBAL variables, which are lookup
variables that can be globally loaded, without calling the runtime, as long as
there was no context extension by a sloppy eval along their context chain.

BUG=v8:5263
Committed: https://crrev.com/044a62be6c8ece4e3b95a23f485cfa6831098b39
Cr-Commit-Position: refs/heads/master@{#39537}

[Leszek Swirski, 2016-09-16 11:10:25]

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

[commit-bot: I haz the power, 2016-09-16 11:10:30]

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

[Leszek Swirski, 2016-09-16 11:22:33]

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

[Leszek Swirski, 2016-09-16 11:22:34]

rmcilroy@chromium.org: Please review changes in 

mythria@chromium.org: Please review changes in

[commit-bot: I haz the power, 2016-09-16 11:25:20]

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

[commit-bot: I haz the power, 2016-09-16 11:25:21]

Dry run: 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...)

[Leszek Swirski, 2016-09-16 11:45:55]

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

[commit-bot: I haz the power, 2016-09-16 11:46:05]

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-09-16 12:08:52]

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

[commit-bot: I haz the power, 2016-09-16 12:08:53]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-09-19 09:04:29]

LGTM with some nits.

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

https://codereview.chromium.org/2347143002/diff/40001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:919: // TODO(leszeks): Build the fast
path here
fullstops on comments.

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

https://codereview.chromium.org/2347143002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1987:
builder()->LoadLookupSlot(variable->name(), typeof_mode);
Just to check, does this branch ever get hit? If not it would be great to remove
the bytecode.

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

https://codereview.chromium.org/2347143002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:424: Node* Interpreter::BuildLoadGlobal(Callable
ic, Node* context, Node* raw_slot,
nit - rename raw_slot to feedback_slot (in header too)

https://codereview.chromium.org/2347143002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:437: Node* Interpreter::BuildLoadGlobal(Callable
ic,
Not sure it's worth having this helper - would probably be as clear just
inlining this code in LdaGlobal

https://codereview.chromium.org/2347143002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:576: __ Dispatch();
I'd prefer you made this BuildLdaLookupSlowPath and returned the result and did
the dispatch in the bytecode handler itself (at which point it probably the same
number of lines just to inline the code).

https://codereview.chromium.org/2347143002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:648: Node* raw_slot = __ BytecodeOperandIdx(1);
nit - slot_index

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

https://codereview.chromium.org/2347143002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.h:147: // fast path to a global load
fullstop

https://codereview.chromium.org/2347143002/diff/40001/test/cctest/compiler/te...
File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2347143002/diff/40001/test/cctest/compiler/te...
test/cctest/compiler/test-run-bytecode-graph-builder.cc:1137: SNPrintF(script,
"function %s(p1) { %s %s %s } ; %s() ;", kFunctionName,
(optional) you could just use std::string to construct this string. like some
other tests.

https://codereview.chromium.org/2347143002/diff/40001/test/cctest/compiler/te...
test/cctest/compiler/test-run-bytecode-graph-builder.cc:1159: SNPrintF(script,
"function %s() { %s %s %s } ; %s() ;", kFunctionName,
ditto

[mythria, 2016-09-19 09:26:53]

Thanks, lgtm with a minor nit.

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

https://codereview.chromium.org/2347143002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:141: // Lookup the variable with
|name|, which is known to be at |feedback_slot| in
nit: update the comment. I think feedback is stored at the |feedback_slot|.

[Leszek Swirski, 2016-09-19 10:34:52]

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

https://codereview.chromium.org/2347143002/diff/40001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:919: // TODO(leszeks): Build the fast
path here
On 2016/09/19 09:04:29, rmcilroy wrote:
> fullstops on comments.

Damn it. Done.

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

https://codereview.chromium.org/2347143002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:141: // Lookup the variable with
|name|, which is known to be at |feedback_slot| in
On 2016/09/19 09:26:53, mythria wrote:
> nit: update the comment. I think feedback is stored at the |feedback_slot|.

Done.

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

https://codereview.chromium.org/2347143002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1987:
builder()->LoadLookupSlot(variable->name(), typeof_mode);
On 2016/09/19 09:04:29, rmcilroy wrote:
> Just to check, does this branch ever get hit? If not it would be great to
remove
> the bytecode.

I still see one generation, in WithStatement.golden, for the code

  with ({x:42}) { return x; }

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

https://codereview.chromium.org/2347143002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:424: Node* Interpreter::BuildLoadGlobal(Callable
ic, Node* context, Node* raw_slot,
On 2016/09/19 09:04:29, rmcilroy wrote:
> nit - rename raw_slot to feedback_slot (in header too)

Done.

https://codereview.chromium.org/2347143002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:437: Node* Interpreter::BuildLoadGlobal(Callable
ic,
On 2016/09/19 09:04:29, rmcilroy wrote:
> Not sure it's worth having this helper - would probably be as clear just
> inlining this code in LdaGlobal

Fair enough, done (or rather, undone).

https://codereview.chromium.org/2347143002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:576: __ Dispatch();
On 2016/09/19 09:04:29, rmcilroy wrote:
> I'd prefer you made this BuildLdaLookupSlowPath and returned the result and
did
> the dispatch in the bytecode handler itself (at which point it probably the
same
> number of lines just to inline the code).

Done.

https://codereview.chromium.org/2347143002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:648: Node* raw_slot = __ BytecodeOperandIdx(1);
On 2016/09/19 09:04:29, rmcilroy wrote:
> nit - slot_index

Changed to feedback_slot, to match the parameter name in BuildLoadGlobal

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

https://codereview.chromium.org/2347143002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.h:147: // fast path to a global load
On 2016/09/19 09:04:29, rmcilroy wrote:
> fullstop

Obsolete, function got removed.

https://codereview.chromium.org/2347143002/diff/40001/test/cctest/compiler/te...
File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2347143002/diff/40001/test/cctest/compiler/te...
test/cctest/compiler/test-run-bytecode-graph-builder.cc:1137: SNPrintF(script,
"function %s(p1) { %s %s %s } ; %s() ;", kFunctionName,
On 2016/09/19 09:04:29, rmcilroy wrote:
> (optional) you could just use std::string to construct this string. like some
> other tests.

Hm, the surrounding code uses SNPrintF so I wanted to keep it consistent.

https://codereview.chromium.org/2347143002/diff/40001/test/cctest/compiler/te...
test/cctest/compiler/test-run-bytecode-graph-builder.cc:1159: SNPrintF(script,
"function %s() { %s %s %s } ; %s() ;", kFunctionName,
On 2016/09/19 09:04:29, rmcilroy wrote:
> ditto

As above

[Leszek Swirski, 2016-09-19 10:45:45]

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

[Leszek Swirski, 2016-09-19 10:45:46]

mstarzinger@chromium.org: Please review changes in

[Michael Starzinger, 2016-09-20 09:26:38]

LGTM.

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

https://codereview.chromium.org/2347143002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter.cc:630: // LdaLookupSlot <name_index>
nit: // LdaLookupContextSlot <name_index> <slot_index> <depth>

https://codereview.chromium.org/2347143002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter.cc:638: // LdaLookupSlotInsideTypeof <name_index>
nit: Likewise.

https://codereview.chromium.org/2347143002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter.cc:680: // LdaLookupSlot <name_index>
nit: // LdaLookupGlobalSlot <name_index> <slot> <depth>

https://codereview.chromium.org/2347143002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter.cc:688: // LdaLookupSlotInsideTypeof <name_index>
nit: Likewise.

[Leszek Swirski, 2016-09-20 09:39:38]

The CQ bit was checked by leszeks@chromium.org

[Leszek Swirski, 2016-09-20 09:39:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, mythria@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2347143002/#ps80001
(title: "Fix bytecode operand documentation")

[commit-bot: I haz the power, 2016-09-20 09:39:47]

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-20 10:31:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-20 10:31:59]

Description was changed from

==========
[interpreter] Add fast path for dynamic global lookups

Adds a fast path for loading DYNAMIC_GLOBAL variables, which are lookup
variables that can be globally loaded, without calling the runtime, as long as
there was no context extension by a sloppy eval along their context chain.

BUG=v8:5263
==========

to

==========
[interpreter] Add fast path for dynamic global lookups

Adds a fast path for loading DYNAMIC_GLOBAL variables, which are lookup
variables that can be globally loaded, without calling the runtime, as long as
there was no context extension by a sloppy eval along their context chain.

BUG=v8:5263

Committed: https://crrev.com/044a62be6c8ece4e3b95a23f485cfa6831098b39
Cr-Commit-Position: refs/heads/master@{#39537}
==========

[commit-bot: I haz the power, 2016-09-20 10:32:01]

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