[KeyedLoadIC] Support Smi "handlers" for element loads

[KeyedLoadIC] Support Smi "handlers" for element loads

This is an experiment as far as performance is concerned. If Smi-configured
element loading directly from the dispatcher stub is fast enough, then we
can stop compiling LoadFastElementStubs (and drop the corresponding code).

Committed: https://crrev.com/c9308147b341596de2733039223918a6202afa5f
Cr-Commit-Position: refs/heads/master@{#38377}

[Jakob Kummerow, 2016-07-27 13:29:30]

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

[Jakob Kummerow, 2016-07-27 13:29:30]

PTAL.

[Igor Sheludko, 2016-07-28 10:01:11]

It would be nice to have unit tests for this functionality.

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

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:2995: if (support_elements == kSupportElements) {
I think this new code (and maybe the property case as well) needs a separate
function.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:2998: Node* is_element_bit = WordAnd(handler_word,
IntPtrConstant(1));
It would be nice to have such a SmiHandlerIsProperty bitfield type somewhere and
define FieldOffsetIsInobject and KeyedLoadIsJsArray using
SmiHandlerIsProperty::kNext.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3002: GotoUnless(WordIsSmi(p->name), miss);
Future improvement: support "123"-like unique strings instead of missing.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3011: Bind(&is_array);
Consider wrapping blocks in {} for readability.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3016: Bind(&length_loaded);
Same here.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3019: Node* convert_hole = WordAnd(
You can probably move this to if_hole block although TF should be smart enough
to put this code into the right block.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3027: Label typed_array(this), fast_packed(this),
fast_holey(this),
As far as I understand we have a kind of naming convention in this file that the
names of labels which are targets of various if checks start with if_.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3028: fast_double(this), fast_holey_double(this),
hole(this);
STATIC_ASSERT(LAST_ELEMENTS_KIND == LAST_FIXED_TYPED_ARRAY_ELEMENTS_KIND);
I hope it will fail once we enable NO_ELEMENTS kind for real.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3034: GotoIf(WordEqual(elements_kind,
IntPtrConstant(kind)), label);
It looks like a switch construct will fit better here.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3043: DebugBreak();
?

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3048: Comment("fast packed elements");
Consider wrapping blocks in {} for readability.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3051: // Do something about that.
We just need to introduce INTPTR_PARAMETERS mode.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3063: Comment("packed double elements");
Same here.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3100: Bind(&typed_array);
And here.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3108: bitfield, Int32Constant(1 <<
JSArrayBuffer::WasNeutered::kShift));
JSArrayBuffer::WasNeutered::kMask?

https://codereview.chromium.org/2180273002/diff/20001/src/ic/handler-compiler.h
File src/ic/handler-compiler.h (right):

https://codereview.chromium.org/2180273002/diff/20001/src/ic/handler-compiler...
src/ic/handler-compiler.h:290: class KeyedLoadIsJsarray : public BitField<bool,
1, 1> {};
Maybe KeyedLoadIsJsArray ?

[Jakob Kummerow, 2016-08-03 11:44:12]

Thanks for the review; comments addressed. PTAL.

Test coverage through the existing test suite is pretty good; I'll add specific
stub tests if performance numbers look like this will stick.

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

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:2995: if (support_elements == kSupportElements) {
On 2016/07/28 10:01:11, Igor Sheludko wrote:
> I think this new code (and maybe the property case as well) needs a separate
> function.

Done (did some splitting, though not exactly here). I think this results in
readable chunks. Let me know if you'd prefer further splitting.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:2998: Node* is_element_bit = WordAnd(handler_word,
IntPtrConstant(1));
On 2016/07/28 10:01:11, Igor Sheludko wrote:
> It would be nice to have such a SmiHandlerIsProperty bitfield type somewhere
and
> define FieldOffsetIsInobject and KeyedLoadIsJsArray using
> SmiHandlerIsProperty::kNext.

Done.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3002: GotoUnless(WordIsSmi(p->name), miss);
On 2016/07/28 10:01:11, Igor Sheludko wrote:
> Future improvement: support "123"-like unique strings instead of missing.

Possibly, yes. That change would have to be made in conjunction with teaching
optimizing compilers about it. As it is, KeyedLoadIC type feedback implies that
optimized code can safely assume that the key was a Smi. If the IC has seen and
converted "123"-like strings, we might want to record that fact somewhere.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3011: Bind(&is_array);
On 2016/07/28 10:01:11, Igor Sheludko wrote:
> Consider wrapping blocks in {} for readability.

Done.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3016: Bind(&length_loaded);
On 2016/07/28 10:01:11, Igor Sheludko wrote:
> Same here.

Done.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3019: Node* convert_hole = WordAnd(
On 2016/07/28 10:01:11, Igor Sheludko wrote:
> You can probably move this to if_hole block although TF should be smart enough
> to put this code into the right block.

Done.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3027: Label typed_array(this), fast_packed(this),
fast_holey(this),
On 2016/07/28 10:01:10, Igor Sheludko wrote:
> As far as I understand we have a kind of naming convention in this file that
the
> names of labels which are targets of various if checks start with if_.

Done.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3028: fast_double(this), fast_holey_double(this),
hole(this);
On 2016/07/28 10:01:11, Igor Sheludko wrote:
> STATIC_ASSERT(LAST_ELEMENTS_KIND == LAST_FIXED_TYPED_ARRAY_ELEMENTS_KIND);
> I hope it will fail once we enable NO_ELEMENTS kind for real.

Done.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3034: GotoIf(WordEqual(elements_kind,
IntPtrConstant(kind)), label);
On 2016/07/28 10:01:10, Igor Sheludko wrote:
> It looks like a switch construct will fit better here.

Done.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3043: DebugBreak();
On 2016/07/28 10:01:11, Igor Sheludko wrote:
> ?

I want to crash if we get here. Think "UNREACHABLE()". Added comment.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3048: Comment("fast packed elements");
On 2016/07/28 10:01:11, Igor Sheludko wrote:
> Consider wrapping blocks in {} for readability.

Done.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3051: // Do something about that.
On 2016/07/28 10:01:10, Igor Sheludko wrote:
> We just need to introduce INTPTR_PARAMETERS mode.

Yes, that's what I was thinking too. Follow-up CL.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3063: Comment("packed double elements");
On 2016/07/28 10:01:11, Igor Sheludko wrote:
> Same here.

Done.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3100: Bind(&typed_array);
On 2016/07/28 10:01:11, Igor Sheludko wrote:
> And here.

Done.

https://codereview.chromium.org/2180273002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:3108: bitfield, Int32Constant(1 <<
JSArrayBuffer::WasNeutered::kShift));
On 2016/07/28 10:01:11, Igor Sheludko wrote:
> JSArrayBuffer::WasNeutered::kMask?

Done.

https://codereview.chromium.org/2180273002/diff/20001/src/ic/handler-compiler.h
File src/ic/handler-compiler.h (right):

https://codereview.chromium.org/2180273002/diff/20001/src/ic/handler-compiler...
src/ic/handler-compiler.h:290: class KeyedLoadIsJsarray : public BitField<bool,
1, 1> {};
On 2016/07/28 10:01:11, Igor Sheludko wrote:
> Maybe KeyedLoadIsJsArray ?

Done.

[Igor Sheludko, 2016-08-04 12:03:09]

lgtm with nits:

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

https://codereview.chromium.org/2180273002/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:2983: Node* int32_key, Node* is_jsarray,
int32_key -> int_ptr_key?

https://codereview.chromium.org/2180273002/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:3112: Switch(elements_kind, miss, elements_kinds,
elements_kind_labels,
miss -> unimplemented_elements_kind?

https://codereview.chromium.org/2180273002/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:3188: Node* is_element_bit =
handler_type

https://codereview.chromium.org/2180273002/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:3204: WordShr(handler_word,
IntPtrConstant(KeyedLoadElementsKind::kShift));
"BitFieldDecode<KeyedLoadElementsKind>" in order not to forget to properly mask
it if we ever add anything else after the elements kind field.

[Jakob Kummerow, 2016-08-05 11:36:03]

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

https://codereview.chromium.org/2180273002/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:2983: Node* int32_key, Node* is_jsarray,
On 2016/08/04 12:03:09, Igor Sheludko wrote:
> int32_key -> int_ptr_key?

Done.

https://codereview.chromium.org/2180273002/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:3112: Switch(elements_kind, miss, elements_kinds,
elements_kind_labels,
On 2016/08/04 12:03:09, Igor Sheludko wrote:
> miss -> unimplemented_elements_kind?

Done (doesn't make a difference here; as long as
STATIC_ASSERT(LAST_ELEMENTS_KIND == LAST_FIXED_TYPED_ARRAY_ELEMENTS_KIND) holds
(line 3010 above), this switch's default case is unreachable).

https://codereview.chromium.org/2180273002/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:3188: Node* is_element_bit =
On 2016/08/04 12:03:09, Igor Sheludko wrote:
> handler_type

Done.

https://codereview.chromium.org/2180273002/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:3204: WordShr(handler_word,
IntPtrConstant(KeyedLoadElementsKind::kShift));
On 2016/08/04 12:03:09, Igor Sheludko wrote:
> "BitFieldDecode<KeyedLoadElementsKind>" in order not to forget to properly
mask
> it if we ever add anything else after the elements kind field.

Done.

[Jakob Kummerow, 2016-08-05 11:44:39]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2016-08-05 11:44:40]

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

[commit-bot: I haz the power, 2016-08-05 11:44:45]

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-08-05 12:07:01]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-05 12:11:09]

Description was changed from

==========
[KeyedLoadIC] Support Smi "handlers" for element loads

This is an experiment as far as performance is concerned. If Smi-configured
element loading directly from the dispatcher stub is fast enough, then we
can stop compiling LoadFastElementStubs (and drop the corresponding code).
==========

to

==========
[KeyedLoadIC] Support Smi "handlers" for element loads

This is an experiment as far as performance is concerned. If Smi-configured
element loading directly from the dispatcher stub is fast enough, then we
can stop compiling LoadFastElementStubs (and drop the corresponding code).

Committed: https://crrev.com/c9308147b341596de2733039223918a6202afa5f
Cr-Commit-Position: refs/heads/master@{#38377}
==========

[commit-bot: I haz the power, 2016-08-05 12:11:10]

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

[Michael Hablich, 2016-08-09 06:44:09]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2222273003/ by hablich@chromium.org.

The reason for reverting is: Times out webgl errors:
https://bugs.chromium.org/p/chromium/issues/detail?id=635545.