[ic] Inline LoadIC into LdaNamedProperty bytecode handler

[ic] Inline LoadIC into LdaNamedProperty bytecode handler

This inlines common LoadIC cases into the LdaNamedProperty bytecode
handler. Smi handlers resulting in constant/field loads for
monomorphic ICs omit frame construction. The same counts for the
polymorphic case as long as the target handler is in the first two
vector slots.

Other cases (megamorphic, uninitialized) call the new
LoadIC_Noninlined stub.

Local benchmarks show up to 6% improvement on Sunspider with --future.

BUG=v8:5917
Review-Url: https://codereview.chromium.org/2733563002
Cr-Commit-Position: refs/heads/master@{#43630}
Committed: https://chromium.googlesource.com/v8/v8/+/0bfabaf17484579ff0adc33085ac232b4168636e

[jgruber, 2017-03-03 12:45:39]

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

[commit-bot: I haz the power, 2017-03-03 12:45:43]

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, 2017-03-03 13:12:15]

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

[commit-bot: I haz the power, 2017-03-03 13:12:16]

Dry run: This issue passed the CQ dry run.

[jgruber, 2017-03-03 13:31:13]

jgruber@chromium.org changed reviewers:
+ ishell@chromium.org, rmcilroy@chromium.org

[jgruber, 2017-03-03 13:31:14]

PTAL. Some self-review comments inline.

https://codereview.chromium.org/2733563002/diff/1/src/ic/accessor-assembler.cc
File src/ic/accessor-assembler.cc (right):

https://codereview.chromium.org/2733563002/diff/1/src/ic/accessor-assembler.c...
src/ic/accessor-assembler.cc:22: Node* AccessorAssembler::LoadFeedbackSlot(Node*
vector, Node* slot,
This is basically LoadFixedArrayElement but with the header_size optimization as
commented below. Any reason we can't make that change in LFAE and use that
instead?

https://codereview.chromium.org/2733563002/diff/1/src/ic/accessor-assembler.c...
src/ic/accessor-assembler.cc:54: std::pair<Node*, Node*>
AccessorAssembler::TryMonomorphicCase(
This version avoids needs a variable and a label less. It's used in
LdaNamedProperty.

https://codereview.chromium.org/2733563002/diff/1/src/ic/accessor-assembler.c...
src/ic/accessor-assembler.cc:301: Label constant(this), field(this),
normal(this, Label::kDeferred);
Both constant and field cases can omit frame construction like this.

https://codereview.chromium.org/2733563002/diff/1/src/ic/accessor-assembler.c...
src/ic/accessor-assembler.cc:1633: void
AccessorAssembler::LoadIC_Noninlined(const LoadICParameters* p,
I don't have a better name for this. Right now it's 'non-monomorphic,
non-deprecated map' but that can change as we inline more things.

https://codereview.chromium.org/2733563002/diff/1/src/ic/accessor-assembler.c...
src/ic/accessor-assembler.cc:2111: Node* feedback = LoadFeedbackSlot(vector,
slot, 0);
We could avoid these two loads if we pass the map and feedback to the stub.
Don't think it makes a big difference though.

[Igor Sheludko, 2017-03-03 15:29:44]

Looks good, only nits. 

I'd appreciate that if you also move the inlined LoadGlobalIC part to
AccessorAssembler in a follow-up CL.

https://codereview.chromium.org/2733563002/diff/1/src/builtins/builtins.h
File src/builtins/builtins.h (right):

https://codereview.chromium.org/2733563002/diff/1/src/builtins/builtins.h#new...
src/builtins/builtins.h:475: TFS(LoadIC_Noninlined, LOAD_IC, kNoExtraICState,
LoadWithVector, 1)          \
s/LOAD_IC/BUILTIN/ since it's just a helper stub called from .

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

https://codereview.chromium.org/2733563002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:847: // Must be kept in sync with
AccessorAssembler::LoadIC.
Please move the LoadIC related piece to AccessorAssembler. Feel free to use
lambdas.

[jgruber, 2017-03-03 17:46:12]

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

[commit-bot: I haz the power, 2017-03-03 17:46:19]

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

[jgruber, 2017-03-03 17:48:48]

Description was changed from

==========
[ic] Inline the monomorphic LoadIC case

This inlines the monomorphic LoadIC case into the LdaNamedProperty bytecode
handler.  Common cases (e.g. Smi handlers resulting in constant/field loads)
omit frame construction.

Polymorphic, megamorphic and uninitialized cases call the new LoadIC_Noninlined
stub.

Local benchmarks show up to 6% improvement on Sunspider with --future.

BUG=v8:5917
==========

to

==========
[ic] Inline LoadIC into LdaNamedProperty bytecode handler

This inlines common LoadIC cases into the LdaNamedProperty bytecode
handler. Smi handlers resulting in constant/field loads for
monomorphic ICs omit frame construction. The same counts for the
polymorphic case as long as the target handler is in the first two
vector slots.

Other cases (megamorphic, uninitialized) call the new
LoadIC_Noninlined stub.

Local benchmarks show up to 6% improvement on Sunspider with --future.

BUG=v8:5917
==========

[jgruber, 2017-03-03 17:53:43]

The latest patchset moves logic to AccessorAssembler as requested.

It also inlines the polymorphic case, which does a linear search to find the
right handler. If the handler is in the first two slots, we omit frame
construction.

PTAL

https://codereview.chromium.org/2733563002/diff/1/src/builtins/builtins.h
File src/builtins/builtins.h (right):

https://codereview.chromium.org/2733563002/diff/1/src/builtins/builtins.h#new...
src/builtins/builtins.h:475: TFS(LoadIC_Noninlined, LOAD_IC, kNoExtraICState,
LoadWithVector, 1)          \
On 2017/03/03 15:29:44, Igor Sheludko wrote:
> s/LOAD_IC/BUILTIN/ since it's just a helper stub called from .

Done.

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

https://codereview.chromium.org/2733563002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:847: // Must be kept in sync with
AccessorAssembler::LoadIC.
On 2017/03/03 15:29:44, Igor Sheludko wrote:
> Please move the LoadIC related piece to AccessorAssembler. Feel free to use
> lambdas.

Done. I left optimizations like delayed loading and removal of useless smi
untagging out of this CL so no lambdas needed.

[commit-bot: I haz the power, 2017-03-03 18:13:00]

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

[commit-bot: I haz the power, 2017-03-03 18:13:01]

Dry run: This issue passed the CQ dry run.

[Igor Sheludko, 2017-03-03 21:43:25]

lgtm

[rmcilroy, 2017-03-06 09:06:37]

> Local benchmarks show up to 6% improvement on Sunspider with --future.

Nice :).

LGTM, thanks.

https://codereview.chromium.org/2733563002/diff/20001/src/ic/accessor-assembl...
File src/ic/accessor-assembler.cc (right):

https://codereview.chromium.org/2733563002/diff/20001/src/ic/accessor-assembl...
src/ic/accessor-assembler.cc:22: Node* AccessorAssembler::LoadFeedbackSlot(Node*
vector, Node* slot,
Nit - I think we have something similar in IntepreterAssember. Maybe we could
just pull this out to CodeStubAssembler so it can be shared here and in
IntepreterAssember (in a follow-up CL)?

https://codereview.chromium.org/2733563002/diff/20001/src/ic/accessor-assembl...
src/ic/accessor-assembler.cc:1605: // Polymorphic ICs with a hit in the first
two entries also omit frames.
Out of interest, any reason only the first two entries are tried, do you run out
of registers otherwise and have to spill?

https://codereview.chromium.org/2733563002/diff/20001/src/ic/accessor-assembl...
src/ic/accessor-assembler.cc:1609: // accident.
There was a way to check this in Hydrogen code stubs, but only for the entire
stub, and we never ported it to TurboFan stubs. Maybe we could have a flag on
labels (like the deferred flag) and propagate that to basic blocks and DCHECK in
the code generator that we didn't build a frame.

[jgruber, 2017-03-07 09:40:56]

Thanks for the reviews! Removing LoadFeedbackSlot and landing after.

https://codereview.chromium.org/2733563002/diff/20001/src/ic/accessor-assembl...
File src/ic/accessor-assembler.cc (right):

https://codereview.chromium.org/2733563002/diff/20001/src/ic/accessor-assembl...
src/ic/accessor-assembler.cc:22: Node* AccessorAssembler::LoadFeedbackSlot(Node*
vector, Node* slot,
On 2017/03/06 09:06:36, rmcilroy wrote:
> Nit - I think we have something similar in IntepreterAssember. Maybe we could
> just pull this out to CodeStubAssembler so it can be shared here and in
> IntepreterAssember (in a follow-up CL)? 

I didn't find a similar function in InterpreterAssembler, which one did you
mean?

In any case, this generates identical code as LoadFixedArrayElement so I'll use
that instead.

https://codereview.chromium.org/2733563002/diff/20001/src/ic/accessor-assembl...
src/ic/accessor-assembler.cc:1605: // Polymorphic ICs with a hit in the first
two entries also omit frames.
On 2017/03/06 09:06:36, rmcilroy wrote:
> Out of interest, any reason only the first two entries are tried, do you run
out
> of registers otherwise and have to spill?

I reused the existing HandlePolymorphicCase. My assumption was that we currently
unroll two iterations because polymorphic ICs have a vector of length >= 2 (i.e.
we can skip length checks for the first 2 iters).

It probably makes sense to unroll this loop completely, I'll do that in a
follow-up CL.

https://codereview.chromium.org/2733563002/diff/20001/src/ic/accessor-assembl...
src/ic/accessor-assembler.cc:1609: // accident.
On 2017/03/06 09:06:36, rmcilroy wrote:
> There was a way to check this in Hydrogen code stubs, but only for the entire
> stub, and we never ported it to TurboFan stubs. Maybe we could have a flag on
> labels (like the deferred flag) and propagate that to basic blocks and DCHECK
in
> the code generator that we didn't build a frame.

Acknowledged. No idea how hard this would be, maybe Michi has some insights.

It'd be great to have though.

[jgruber, 2017-03-07 09:48:55]

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

[commit-bot: I haz the power, 2017-03-07 09:48:58]

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, 2017-03-07 10:15:57]

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

[commit-bot: I haz the power, 2017-03-07 10:15:58]

Dry run: This issue passed the CQ dry run.

[jgruber, 2017-03-07 10:19:46]

The CQ bit was checked by jgruber@chromium.org

[jgruber, 2017-03-07 10:19:47]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org,
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2733563002/#ps40001
(title: "Remove LoadFeedbackSlot")

[commit-bot: I haz the power, 2017-03-07 10:19:58]

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, 2017-03-07 10:21:34]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488881986687200,
"parent_rev": "95d808670e37d660f9eb8d76546587c8fe52167d", "commit_rev":
"0bfabaf17484579ff0adc33085ac232b4168636e"}

[commit-bot: I haz the power, 2017-03-07 10:21:38]

Description was changed from

==========
[ic] Inline LoadIC into LdaNamedProperty bytecode handler

This inlines common LoadIC cases into the LdaNamedProperty bytecode
handler. Smi handlers resulting in constant/field loads for
monomorphic ICs omit frame construction. The same counts for the
polymorphic case as long as the target handler is in the first two
vector slots.

Other cases (megamorphic, uninitialized) call the new
LoadIC_Noninlined stub.

Local benchmarks show up to 6% improvement on Sunspider with --future.

BUG=v8:5917
==========

to

==========
[ic] Inline LoadIC into LdaNamedProperty bytecode handler

This inlines common LoadIC cases into the LdaNamedProperty bytecode
handler. Smi handlers resulting in constant/field loads for
monomorphic ICs omit frame construction. The same counts for the
polymorphic case as long as the target handler is in the first two
vector slots.

Other cases (megamorphic, uninitialized) call the new
LoadIC_Noninlined stub.

Local benchmarks show up to 6% improvement on Sunspider with --future.

BUG=v8:5917

Review-Url: https://codereview.chromium.org/2733563002
Cr-Commit-Position: refs/heads/master@{#43630}
Committed:
https://chromium.googlesource.com/v8/v8/+/0bfabaf17484579ff0adc33085ac232b416...
==========

[commit-bot: I haz the power, 2017-03-07 10:21:39]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/0bfabaf17484579ff0adc33085ac232b416...