[KeyedLoadIC] Support Smi "handlers" for simple field loads

[KeyedLoadIC] Support Smi "handlers" for simple field loads

This ports 9c59539f2 / r37803 to KeyedLoadICs.

Committed: https://crrev.com/9a6ee8d687455b81f12748076051c2c6a6f51592
Cr-Commit-Position: refs/heads/master@{#38070}

[Jakob Kummerow, 2016-07-26 14:43:27]

jkummerow@chromium.org changed reviewers:
+ ishell@chromium.org, jarin@chromium.org

[Jakob Kummerow, 2016-07-26 14:43:29]

Jaro: please review src/compiler/.
Igor: please review everything else.

I've broken the CL into patch sets for your reviewing convenience:
- patch set 1 just moves code around, no change in behavior.
- patch set 2 changes the "handler" Smis' encoding to a new scheme. That's
necessary to cleanly distinguish between element and property loads later
(future CL), but doesn't hurt to do right away.
- patch set 3 ports the KeyedLoadIC stub to TF, which as a side effect makes it
Smi "handler" aware (for property loads).

I'd recommend to review patch set by patch set.

https://codereview.chromium.org/2182103002/diff/1/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2182103002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:2980: void CodeStubAssembler::HaveLoadICHandler(const
LoadICParameters* p,
This entire function has been pulled out from below, no further changes have
been made to it (in this patch set).

https://codereview.chromium.org/2182103002/diff/1/src/ic/handler-compiler.cc
File src/ic/handler-compiler.cc (right):

https://codereview.chromium.org/2182103002/diff/1/src/ic/handler-compiler.cc#...
src/ic/handler-compiler.cc:580: Handle<Code>
ElementHandlerCompiler::GetKeyedLoadHandler(
This unifies the loop body of CompileElementHandlers (below) and
PropertyICCompiler::ComputeKeyedLoadMonomorphicHandler (in ic-compiler.cc),
which were identical modulo TRACE_HANDLER_STATS and the additional conditions in
the interceptor case. (I've wanted to clean this up for a long time!)

https://codereview.chromium.org/2182103002/diff/20001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2182103002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3000: Label inobject_double(this),
out_of_object(this),
Disregard these label redeclarations; editing mistake; gone in the next patch
set.

https://codereview.chromium.org/2182103002/diff/40001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2182103002/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:2419: GotoUnless(Uint32LessThan(index,
SmiToWord32(length)), if_not_found);
Drive-by fix: bounds checks should use unsigned comparisons. This isn't
currently a bug because callers are careful enough, but it's better to be robust
here.

https://codereview.chromium.org/2182103002/diff/40001/src/full-codegen/full-c...
File src/full-codegen/full-codegen.cc (right):

https://codereview.chromium.org/2182103002/diff/40001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1033: if (FLAG_tf_load_ic_stub)
RestoreContext();
Having to do this makes me sad, but the conclusion of last week's discussion was
that this is the preferred solution for now.

[Jarin, 2016-07-26 14:49:47]

src/compiler/* lgtm.

[Igor Sheludko, 2016-07-26 16:25:12]

lgtm with nits:

https://codereview.chromium.org/2182103002/diff/1/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2182103002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:2980: void CodeStubAssembler::HaveLoadICHandler(const
LoadICParameters* p,
On 2016/07/26 14:43:28, Jakob wrote:
> This entire function has been pulled out from below, no further changes have
> been made to it (in this patch set).

How about HandleLoadICHandlerCase() or something similar?

https://codereview.chromium.org/2182103002/diff/1/src/ic/handler-compiler.cc
File src/ic/handler-compiler.cc (right):

https://codereview.chromium.org/2182103002/diff/1/src/ic/handler-compiler.cc#...
src/ic/handler-compiler.cc:580: Handle<Code>
ElementHandlerCompiler::GetKeyedLoadHandler(
On 2016/07/26 14:43:28, Jakob wrote:
> This unifies the loop body of CompileElementHandlers (below) and
> PropertyICCompiler::ComputeKeyedLoadMonomorphicHandler (in ic-compiler.cc),
> which were identical modulo TRACE_HANDLER_STATS and the additional conditions
in
> the interceptor case. (I've wanted to clean this up for a long time!)

Acknowledged.

https://codereview.chromium.org/2182103002/diff/1/src/ic/handler-compiler.cc#...
src/ic/handler-compiler.cc:593: if (instance_type < FIRST_JS_RECEIVER_TYPE) {
TRACE_HANDLER_STATS(isolate(), KeyedLoadIC_SlowStub);

and once you handled all the stats cases here you can remove
KeyedLoadIC_PolymorphicElement counter.

https://codereview.chromium.org/2182103002/diff/20001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2182103002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3000: Label inobject_double(this),
out_of_object(this),
On 2016/07/26 14:43:28, Jakob wrote:
> Disregard these label redeclarations; editing mistake; gone in the next patch
> set.

Acknowledged.

https://codereview.chromium.org/2182103002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3002: Node* inobject_bit = WordAnd(handler_word,
IntPtrConstant(1 << 1));
It would be nice to have respective constants in FieldIndex. Or maybe even a set
of BitField<>s.

https://codereview.chromium.org/2182103002/diff/20001/src/field-index-inl.h
File src/field-index-inl.h (right):

https://codereview.chromium.org/2182103002/diff/20001/src/field-index-inl.h#n...
src/field-index-inl.h:92: bool is_inobject = offset & 2;
Same here.

https://codereview.chromium.org/2182103002/diff/20001/src/field-index-inl.h#n...
src/field-index-inl.h:115: return (offset << 3) | ((is_double() ? 1 : 0) << 2) |
And here.

https://codereview.chromium.org/2182103002/diff/40001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2182103002/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:2419: GotoUnless(Uint32LessThan(index,
SmiToWord32(length)), if_not_found);
On 2016/07/26 14:43:28, Jakob wrote:
> Drive-by fix: bounds checks should use unsigned comparisons. This isn't
> currently a bug because callers are careful enough, but it's better to be
robust
> here.

Acknowledged.

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

https://codereview.chromium.org/2182103002/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:476: Node* receiver = assembler->Parameter(0);
Please use Descriptor::ParameterIndices constants.

https://codereview.chromium.org/2182103002/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:489: Node* receiver = assembler->Parameter(0);
Same here.

https://codereview.chromium.org/2182103002/diff/40001/src/full-codegen/full-c...
File src/full-codegen/full-codegen.cc (right):

https://codereview.chromium.org/2182103002/diff/40001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1033: if (FLAG_tf_load_ic_stub)
RestoreContext();
On 2016/07/26 14:43:28, Jakob wrote:
> Having to do this makes me sad, but the conclusion of last week's discussion
was
> that this is the preferred solution for now.

Acknowledged.

[Jakob Kummerow, 2016-07-26 17:10:40]

Thanks for the review, comments addressed.

https://codereview.chromium.org/2182103002/diff/1/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2182103002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:2980: void CodeStubAssembler::HaveLoadICHandler(const
LoadICParameters* p,
On 2016/07/26 16:25:11, Igor Sheludko wrote:
> On 2016/07/26 14:43:28, Jakob wrote:
> > This entire function has been pulled out from below, no further changes have
> > been made to it (in this patch set).
> 
> How about HandleLoadICHandlerCase() or something similar?

Done.

https://codereview.chromium.org/2182103002/diff/1/src/ic/handler-compiler.cc
File src/ic/handler-compiler.cc (right):

https://codereview.chromium.org/2182103002/diff/1/src/ic/handler-compiler.cc#...
src/ic/handler-compiler.cc:593: if (instance_type < FIRST_JS_RECEIVER_TYPE) {
On 2016/07/26 16:25:11, Igor Sheludko wrote:
> TRACE_HANDLER_STATS(isolate(), KeyedLoadIC_SlowStub);
> 
> and once you handled all the stats cases here you can remove
> KeyedLoadIC_PolymorphicElement counter.

Done.

https://codereview.chromium.org/2182103002/diff/20001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2182103002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3002: Node* inobject_bit = WordAnd(handler_word,
IntPtrConstant(1 << 1));
On 2016/07/26 16:25:11, Igor Sheludko wrote:
> It would be nice to have respective constants in FieldIndex. Or maybe even a
set
> of BitField<>s.

Done.

https://codereview.chromium.org/2182103002/diff/20001/src/field-index-inl.h
File src/field-index-inl.h (right):

https://codereview.chromium.org/2182103002/diff/20001/src/field-index-inl.h#n...
src/field-index-inl.h:92: bool is_inobject = offset & 2;
On 2016/07/26 16:25:11, Igor Sheludko wrote:
> Same here.

Done.

https://codereview.chromium.org/2182103002/diff/20001/src/field-index-inl.h#n...
src/field-index-inl.h:115: return (offset << 3) | ((is_double() ? 1 : 0) << 2) |
On 2016/07/26 16:25:11, Igor Sheludko wrote:
> And here.

Done.

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

https://codereview.chromium.org/2182103002/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:476: Node* receiver = assembler->Parameter(0);
On 2016/07/26 16:25:11, Igor Sheludko wrote:
> Please use Descriptor::ParameterIndices constants.

Done.

https://codereview.chromium.org/2182103002/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:489: Node* receiver = assembler->Parameter(0);
On 2016/07/26 16:25:11, Igor Sheludko wrote:
> Same here.

Done.

[Jakob Kummerow, 2016-07-26 17:23:03]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2016-07-26 17:23:03]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org,
jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/2182103002/#ps80001
(title: "rebased")

[commit-bot: I haz the power, 2016-07-26 17:23:13]

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-07-26 17:51:32]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-26 17:54:40]

Description was changed from

==========
[KeyedLoadIC] Support Smi "handlers" for simple field loads

This ports 9c59539f2 / r37803 to KeyedLoadICs.
==========

to

==========
[KeyedLoadIC] Support Smi "handlers" for simple field loads

This ports 9c59539f2 / r37803 to KeyedLoadICs.

Committed: https://crrev.com/9a6ee8d687455b81f12748076051c2c6a6f51592
Cr-Commit-Position: refs/heads/master@{#38070}
==========

[commit-bot: I haz the power, 2016-07-26 17:54:41]

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