HStoreKeyed for Smis optimized for x64 + related redundant moves of elements removed

HStoreKeyed for Smis optimized for x64 + related redundant moves of elements removed

R=verwaest@chromium.org

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

[Igor Sheludko, 2013-12-13 15:20:06]

PTAL

[Toon Verwaest, 2013-12-17 13:53:03]

https://codereview.chromium.org/108503004/diff/20001/src/elements-kind.h
File src/elements-kind.h (right):

https://codereview.chromium.org/108503004/diff/20001/src/elements-kind.h#newc...
src/elements-kind.h:170: inline bool IsFastPackedSmiElementsKind(ElementsKind
kind) {
I wouldn't add this method. Just directly compare with FAST_SMI_ELEMENTS.

https://codereview.chromium.org/108503004/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/108503004/diff/20001/src/hydrogen.cc#newcode2115
src/hydrogen.cc:2115: 
spurious whitespace addition

https://codereview.chromium.org/108503004/diff/20001/src/hydrogen.cc#newcode2280
src/hydrogen.cc:2280: if (IsFastPackedSmiElementsKind(elements_kind)) {
store->set_store_to_initialize_element(elements_kind == FAST_SMI_ELEMENTS);

https://codereview.chromium.org/108503004/diff/20001/src/x64/lithium-x64.cc
File src/x64/lithium-x64.cc (right):

https://codereview.chromium.org/108503004/diff/20001/src/x64/lithium-x64.cc#n...
src/x64/lithium-x64.cc:2201: object = UseRegisterAtStart(instr->elements());
None of the variables here should probably be "AtStart". AtStart means:
   // Lifetime of operand inside the instruction.
   enum Lifetime {
     // USED_AT_START operand is guaranteed to be live only at
     // instruction start. Register allocator is free to assign the same
register
     // to some other operand used inside instruction (i.e. temporary or
     // output).
     USED_AT_START,

Given that you also use temps, this may make the register allocator give you the
same register for both the "AtStart" registers and the temp registers.

[Igor Sheludko, 2013-12-18 10:01:32]

I think I found a better/cleaner way to implement this optimization, PTAL patch
set 4.

https://codereview.chromium.org/108503004/diff/20001/src/elements-kind.h
File src/elements-kind.h (right):

https://codereview.chromium.org/108503004/diff/20001/src/elements-kind.h#newc...
src/elements-kind.h:170: inline bool IsFastPackedSmiElementsKind(ElementsKind
kind) {
On 2013/12/17 13:53:04, Toon Verwaest wrote:
> I wouldn't add this method. Just directly compare with FAST_SMI_ELEMENTS.

Done.

https://codereview.chromium.org/108503004/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/108503004/diff/20001/src/hydrogen.cc#newcode2115
src/hydrogen.cc:2115: 
On 2013/12/17 13:53:04, Toon Verwaest wrote:
> spurious whitespace addition

Done.

https://codereview.chromium.org/108503004/diff/20001/src/hydrogen.cc#newcode2280
src/hydrogen.cc:2280: if (IsFastPackedSmiElementsKind(elements_kind)) {
On 2013/12/17 13:53:04, Toon Verwaest wrote:
> store->set_store_to_initialize_element(elements_kind == FAST_SMI_ELEMENTS);

Done.

https://codereview.chromium.org/108503004/diff/20001/src/x64/lithium-x64.cc
File src/x64/lithium-x64.cc (right):

https://codereview.chromium.org/108503004/diff/20001/src/x64/lithium-x64.cc#n...
src/x64/lithium-x64.cc:2201: object = UseRegisterAtStart(instr->elements());
We discussed the "AtStart" issue and decided that since this comment concerns
all the other places where we use "AtStart" methods there should be a different
"cleanup" CL fixing that.

[Toon Verwaest, 2013-12-18 11:23:22]

lgtm

[Igor Sheludko, 2013-12-18 13:15:38]

Even cleaner implementation.

[Toon Verwaest, 2013-12-18 14:02:20]

lgtm

[Igor Sheludko, 2013-12-18 18:05:18]

Committed patchset #6 manually as r18362 (presubmit successful).