harmony-scoping: Implement LoadIC handler for loads from global contexts.

harmony-scoping: Implement LoadIC handler for loads from global contexts.

R=ishell@chromium.org, adamk@chromium.org, rossberg@chromium.org, verwaest@chromium.org
BUG=v8:2198
LOG=N

Committed: https://code.google.com/p/v8/source/detail?r=25259

[Dmitry Lomov (no reviews), 2014-11-07 15:17:22]

FYI, this is mostly finished work in progress; needs
https://codereview.chromium.org/705663004/ to land, but let me know if you have
comments.

[Dmitry Lomov (no reviews), 2014-11-07 15:24:46]

dslomov@chromium.org changed reviewers:
+ adamk@chromium.org
- adamk@chromum.org

[Dmitry Lomov (no reviews), 2014-11-07 16:47:29]

This is now ready for review; PTAL

[adamk, 2014-11-07 16:57:03]

Toon is clearly the expert here, but I'm learning by reading this patch. Just
one nit.

https://codereview.chromium.org/696783005/diff/20001/src/hydrogen-instruction...
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/696783005/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.cc:4647: int offset = GlobalContextTable::kHeaderSize
+ (index + 1) * kPointerSize;
Here's that "+ 1" again. Now that it's leaking out of the GlobalContextTable
implementation, perhaps it's worth adding a constant?

[Dmitry Lomov (no reviews), 2014-11-07 18:25:15]

Cleaned it up a bit per Adam's request

[adamk, 2014-11-10 17:23:26]

style-wise looking good, but Toon is the right person to review this further

[Igor Sheludko, 2014-11-11 09:49:51]

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

[Igor Sheludko, 2014-11-11 09:49:51]

lgtm with nits:

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

https://codereview.chromium.org/696783005/diff/40001/src/code-stubs.h#newcode...
src/code-stubs.h:2041: return lookup_result->context_index >= 0 &&
Suggestion: there is a BitField::is_valid() method which could be convenient to
use here.

https://codereview.chromium.org/696783005/diff/40001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/696783005/diff/40001/src/hydrogen.cc#newcode5380
src/hydrogen.cc:5380: Handle<GlobalContextTable> global_contexts(
It looks like this piece of code should be inside if (FLAG_harmony_scoping...)
block.

https://codereview.chromium.org/696783005/diff/40001/test/cctest/test-decls.cc
File test/cctest/test-decls.cc (right):

https://codereview.chromium.org/696783005/diff/40001/test/cctest/test-decls.c...
test/cctest/test-decls.cc:892: context.Check("%OptimizeFunctionOnNextCall(g);
g()", EXPECT_RESULT,
What about also trying to optimize f() and g() before the "let x" script to
ensure that the optimized code does not stuck with the wrong x? Maybe as another
test.

[Dmitry Lomov (no reviews), 2014-11-11 11:16:20]

Comments addressed, landing

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

https://codereview.chromium.org/696783005/diff/40001/src/code-stubs.h#newcode...
src/code-stubs.h:2041: return lookup_result->context_index >= 0 &&
On 2014/11/11 09:49:51, Igor Sheludko wrote:
> Suggestion: there is a BitField::is_valid() method which could be convenient
to
> use here.

Done.

https://codereview.chromium.org/696783005/diff/40001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/696783005/diff/40001/src/hydrogen.cc#newcode5380
src/hydrogen.cc:5380: Handle<GlobalContextTable> global_contexts(
On 2014/11/11 09:49:51, Igor Sheludko wrote:
> It looks like this piece of code should be inside if (FLAG_harmony_scoping...)
> block.

Done for consistency, even though it does not really matter. If flag is off,
global context table is empty

https://codereview.chromium.org/696783005/diff/40001/test/cctest/test-decls.cc
File test/cctest/test-decls.cc (right):

https://codereview.chromium.org/696783005/diff/40001/test/cctest/test-decls.c...
test/cctest/test-decls.cc:892: context.Check("%OptimizeFunctionOnNextCall(g);
g()", EXPECT_RESULT,
On 2014/11/11 09:49:51, Igor Sheludko wrote:
> What about also trying to optimize f() and g() before the "let x" script to
> ensure that the optimized code does not stuck with the wrong x? Maybe as
another
> test.

Done.

[Dmitry Lomov (no reviews), 2014-11-11 11:16:40]

Committed patchset #4 (id:60001) manually as 25259 (presubmit successful).