[interpreter] Add a fast path for dynamic local load

[interpreter] Add a fast path for dynamic local load

Adds a fast path for loading DYNAMIC_LOCAL variables, which are lookup
variables that can be context 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/66d2e1fc22398f24ee43d65010b80441a0a33c80
Cr-Commit-Position: refs/heads/master@{#39473}

[Leszek Swirski, 2016-09-14 13:14:53]

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

[Leszek Swirski, 2016-09-14 13:14:54]

rmcilroy@chromium.org: Please review changes in 

mythria@chromium.org: Please review changes in

[rmcilroy, 2016-09-14 14:46:27]

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

[commit-bot: I haz the power, 2016-09-14 14:46:37]

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-14 14:54:45]

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

[commit-bot: I haz the power, 2016-09-14 14:54:46]

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...)

[mythria, 2016-09-15 08:55:53]

Nice, lgtm.

[Leszek, 2016-09-15 09:46:47]

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

[commit-bot: I haz the power, 2016-09-15 09:46:53]

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

[Leszek, 2016-09-15 09:47:16]

Added some tests, PTAL

[commit-bot: I haz the power, 2016-09-15 10:05:16]

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

[commit-bot: I haz the power, 2016-09-15 10:05:17]

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...)

[Leszek, 2016-09-15 10:33:31]

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

[commit-bot: I haz the power, 2016-09-15 10:33:37]

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-15 10:39:27]

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

[commit-bot: I haz the power, 2016-09-15 10:39:29]

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

[Leszek, 2016-09-15 11:21:10]

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

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

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-15 11:46:52]

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

[commit-bot: I haz the power, 2016-09-15 11:46:53]

Dry run: This issue passed the CQ dry run.

[Leszek, 2016-09-15 11:54:59]

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

[Leszek, 2016-09-15 11:54:59]

mstarzinger@chromium.org: Please review changes in

[rmcilroy, 2016-09-16 08:56:55]

Nice, LGTM once comments are addressed.

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

https://codereview.chromium.org/2343633002/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:890: // TODO(leszeks): Build the fast
path here
fullstop after comments (throughout cl)

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

https://codereview.chromium.org/2343633002/diff/80001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:135: // contexts actually need to be
checked.
It would probably take as long to check the bitmap as to do this check here (and
would increase bytecode size), so assuming this check is only for performance
and not correctness, I wouldn't bother.

https://codereview.chromium.org/2343633002/diff/80001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/2343633002/diff/80001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:1804: // clang-format off
Is this necessary? None of the other functions seem to have it.

https://codereview.chromium.org/2343633002/diff/80001/test/cctest/interpreter...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/2343633002/diff/80001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:3870: // Factory* factory =
isolate->factory();
remove

https://codereview.chromium.org/2343633002/diff/80001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:3873: const char*
inner_function_epilogue = "return x; };";
I'm not sure the inner_function_epilogue is particularly helpful and makes it
difficult to see what the test code actually is. Could you just inline the
"return x" into the inner test code please.

https://codereview.chromium.org/2343633002/diff/80001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:3876: std::tuple<const char*, const
char*, Handle<Object>> lookup_slot[] = {
Could you add these tests to test-run-bytecode-graph-builder too please

[Leszek Swirski, 2016-09-16 10:46:55]

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

https://codereview.chromium.org/2343633002/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:890: // TODO(leszeks): Build the fast
path here
On 2016/09/16 08:56:55, rmcilroy wrote:
> fullstop after comments (throughout cl)

Done.

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

https://codereview.chromium.org/2343633002/diff/80001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:135: // contexts actually need to be
checked.
On 2016/09/16 08:56:55, rmcilroy wrote:
> It would probably take as long to check the bitmap as to do this check here
(and
> would increase bytecode size), so assuming this check is only for performance
> and not correctness, I wouldn't bother.

I see your point, but it would e.g. make a big difference to the TurboFan graph
generated by our graph builder. Even if not, we should definitely still only
look as far up in the chain as the deepest sloppy eval.

https://codereview.chromium.org/2343633002/diff/80001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/2343633002/diff/80001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:1804: // clang-format off
On 2016/09/16 08:56:55, rmcilroy wrote:
> Is this necessary? None of the other functions seem to have it.

Good question, I wouldn't have thought so but then the presubmits failed without
it (and those wouldn't let me keep the spaces between test bodies)

https://codereview.chromium.org/2343633002/diff/80001/test/cctest/interpreter...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/2343633002/diff/80001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:3870: // Factory* factory =
isolate->factory();
On 2016/09/16 08:56:55, rmcilroy wrote:
> remove

Done.

https://codereview.chromium.org/2343633002/diff/80001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:3873: const char*
inner_function_epilogue = "return x; };";
On 2016/09/16 08:56:55, rmcilroy wrote:
> I'm not sure the inner_function_epilogue is particularly helpful and makes it
> difficult to see what the test code actually is. Could you just inline the
> "return x" into the inner test code please.

Done.

https://codereview.chromium.org/2343633002/diff/80001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:3876: std::tuple<const char*, const
char*, Handle<Object>> lookup_slot[] = {
On 2016/09/16 08:56:55, rmcilroy wrote:
> Could you add these tests to test-run-bytecode-graph-builder too please

Done.

[Michael Starzinger, 2016-09-16 12:31:59]

LGTM with one comment.

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

https://codereview.chromium.org/2343633002/diff/120001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:141:
GotoIf(Word32NotEqual(extension_slot, TheHoleConstant()), target);
Shouldn't this be WordNotEqual (full word on 64-bit systems)? Please double
check that this does a word-size comparison on all architectures.

[Leszek Swirski, 2016-09-16 13:00:50]

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

https://codereview.chromium.org/2343633002/diff/120001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:141:
GotoIf(Word32NotEqual(extension_slot, TheHoleConstant()), target);
On 2016/09/16 12:31:59, Michael Starzinger wrote:
> Shouldn't this be WordNotEqual (full word on 64-bit systems)? Please double
> check that this does a word-size comparison on all architectures.

Thank you for catching this! It has probably saved us from a very hard to debug
bug at some point in the future.

[Leszek Swirski, 2016-09-16 13:01:28]

The CQ bit was checked by leszeks@chromium.org

[Leszek Swirski, 2016-09-16 13:01:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
mythria@chromium.org, rmcilroy@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2343633002/#ps140001
(title: "Fix extension slot word size in equality test")

[commit-bot: I haz the power, 2016-09-16 13:01:38]

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 13:26:50]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-09-16 13:27:28]

Description was changed from

==========
[interpreter] Add a fast path for dynamic local load

Adds a fast path for loading DYNAMIC_LOCAL variables, which are lookup
variables that can be context 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 a fast path for dynamic local load

Adds a fast path for loading DYNAMIC_LOCAL variables, which are lookup
variables that can be context 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/66d2e1fc22398f24ee43d65010b80441a0a33c80
Cr-Commit-Position: refs/heads/master@{#39473}
==========

[commit-bot: I haz the power, 2016-09-16 13:27:31]

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