Vector ICs: Get rid of stack arguments on ia32 transitioning stores.

Vector ICs: Get rid of stack arguments on ia32 transitioning stores.

The stack manipulation was expensive. Two virtual registers are better.

BUG=

Committed: https://crrev.com/2d4aeaad2fa3888c5e4cbfd30f6338e1fbf794e9
Cr-Commit-Position: refs/heads/master@{#31204}

[mvstanton, 2015-10-05 14:49:30]

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

[mvstanton, 2015-10-05 14:49:31]

Hi Jakob,
Here is the CL that eliminates stack arguments for ia32 transitioning stores
(the other platforms never had such a thing). I think after this, we'll be able
to land --vector-stores. Thanks for the look!
--Michael

[Jakob Kummerow, 2015-10-07 14:40:10]

LGTM with a bunch of nits.

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

https://codereview.chromium.org/1376933006/diff/20001/src/code-stubs.h#newcod...
src/code-stubs.h:1262: DCHECK((int)VectorStoreTransitionDescriptor::kMapIndex ==
nit: the style guide wants "static_cast<int>(...)" instead of "(int)...".

https://codereview.chromium.org/1376933006/diff/20001/src/code-stubs.h#newcod...
src/code-stubs.h:1276: static bool VirtualSlotArg() { return
SlotRegister().is(no_reg); }
nit: please pick a name that sounds more like the function returns a bool, e.g.
"HasVirtualSlotArg" or "UsesVirtualSlotArg".

https://codereview.chromium.org/1376933006/diff/20001/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/1376933006/diff/20001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:4810: __ pop(feedback);  // Pop "value".
Drive-by: if you move the definition "Register value = ...ValueRegister()" from
line 4822 up to, say, line 4779, then you can just say "pop(value);" here and
drop the scary comment.

https://codereview.chromium.org/1376933006/diff/20001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:4816: //   +4  ret addr       sp -> ret addr
I'm finding it a bit hard to make sense of this comment. Suggestion: follow the
notation used elsewhere in this file, so roughly:

// Current stack layout:
// - esp[0]  -- key
// - esp[4]  -- vector
// - esp[8]  -- receiver
// - esp[12] -- value
// - esp[16] -- return address
//
// Required stack layout for handler call:
// - esp[0] -- return address
// - receiver, key, value, map, vector in registers.
// - handler and slot in virtual registers.

https://codereview.chromium.org/1376933006/diff/20001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:4824: // Get the handler in value.
nit: outdated comment; the code below doesn't move the handler into |value|.

https://codereview.chromium.org/1376933006/diff/20001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:4833: __
mov(VectorStoreTransitionDescriptor::MapRegister(), cached_map);
nit: this deserves a
DCHECK(!cached_map.is(VectorStoreTransitionDescriptor::MapRegister()).

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

https://codereview.chromium.org/1376933006/diff/20001/src/ic/handler-compiler...
src/ic/handler-compiler.cc:459: bool stack_args =
StoreTransitionHelper::VirtualSlotArg();
nit: this variable should probably be called "virtual_args" now.

https://codereview.chromium.org/1376933006/diff/20001/src/ic/ia32/handler-com...
File src/ic/ia32/handler-compiler-ia32.cc (right):

https://codereview.chromium.org/1376933006/diff/20001/src/ic/ia32/handler-com...
src/ic/ia32/handler-compiler-ia32.cc:367: //       current               after
RearrangeVectorAndSlot
This legend helps a lot making the notation below understandable; however maybe
we don't need it at all any more? The description you put in the header file
("Pops the vector and slot into appropriate registers") might be enough.

https://codereview.chromium.org/1376933006/diff/20001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/1376933006/diff/20001/src/ic/ic.cc#newcode2483
src/ic/ic.cc:2483: // We might have slot and vector, for a normal miss.
(slot(3), vector(4)).
nit: drop the first full stop:
// ... normal miss (slot(3), vector(4)).

https://codereview.chromium.org/1376933006/diff/20001/src/ic/ic.cc#newcode2486
src/ic/ic.cc:2486: // If we have 6 arguments, then we have map, slot, vector.
nit: for consistency with the above, please add indices: "map(3), slot(4),
vector(5)".

https://codereview.chromium.org/1376933006/diff/20001/src/interface-descripto...
File src/interface-descriptors.h (right):

https://codereview.chromium.org/1376933006/diff/20001/src/interface-descripto...
src/interface-descriptors.h:294: kVirtualSlotParameterCount = 5,
I don't see this used anywhere.

https://codereview.chromium.org/1376933006/diff/20001/src/interface-descripto...
src/interface-descriptors.h:296: kParameterCount = 6
I don't see this used anywhere either.

https://codereview.chromium.org/1376933006/diff/20001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/1376933006/diff/20001/src/isolate.h#newcode1311
src/isolate.h:1311: Address vector_store_virtual_slot_;
I'm not overly happy with this pair of names, because "register" is very
generic, whereas "slot" describes a very specific use case. I think either both
should have generic names, such as "virtual_register_one" and
"virtual_register_two" (which would make sense if we assume that they'll be used
for other purposes in the future), or both names should indicate usage, e.g.
"virtual_handler_register" and "virtual_slot_register".

I'm fine with leaving the renaming (+discussion, as necessary) to a future CL,
though.

[mvstanton, 2015-10-09 13:29:47]

Thanks for the good comments! It's a kind of awkward CL, they've been a big
help....
--Michael

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

https://codereview.chromium.org/1376933006/diff/20001/src/code-stubs.h#newcod...
src/code-stubs.h:1262: DCHECK((int)VectorStoreTransitionDescriptor::kMapIndex ==
On 2015/10/07 14:40:10, Jakob wrote:
> nit: the style guide wants "static_cast<int>(...)" instead of "(int)...".

Done.

https://codereview.chromium.org/1376933006/diff/20001/src/code-stubs.h#newcod...
src/code-stubs.h:1276: static bool VirtualSlotArg() { return
SlotRegister().is(no_reg); }
On 2015/10/07 14:40:10, Jakob wrote:
> nit: please pick a name that sounds more like the function returns a bool,
e.g.
> "HasVirtualSlotArg" or "UsesVirtualSlotArg".

Done.

https://codereview.chromium.org/1376933006/diff/20001/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/1376933006/diff/20001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:4810: __ pop(feedback);  // Pop "value".
On 2015/10/07 14:40:10, Jakob wrote:
> Drive-by: if you move the definition "Register value = ...ValueRegister()"
from
> line 4822 up to, say, line 4779, then you can just say "pop(value);" here and
> drop the scary comment.

Done.

https://codereview.chromium.org/1376933006/diff/20001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:4816: //   +4  ret addr       sp -> ret addr
On 2015/10/07 14:40:10, Jakob wrote:
> I'm finding it a bit hard to make sense of this comment. Suggestion: follow
the
> notation used elsewhere in this file, so roughly:
> 
> // Current stack layout:
> // - esp[0]  -- key
> // - esp[4]  -- vector
> // - esp[8]  -- receiver
> // - esp[12] -- value
> // - esp[16] -- return address
> //
> // Required stack layout for handler call:
> // - esp[0] -- return address
> // - receiver, key, value, map, vector in registers.
> // - handler and slot in virtual registers.

Thanks! Seems like git-cl format messed up my formatting, but yours is still
better. Done.

https://codereview.chromium.org/1376933006/diff/20001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:4824: // Get the handler in value.
On 2015/10/07 14:40:10, Jakob wrote:
> nit: outdated comment; the code below doesn't move the handler into |value|.

Done.

https://codereview.chromium.org/1376933006/diff/20001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:4833: __
mov(VectorStoreTransitionDescriptor::MapRegister(), cached_map);
On 2015/10/07 14:40:10, Jakob wrote:
> nit: this deserves a
> DCHECK(!cached_map.is(VectorStoreTransitionDescriptor::MapRegister()).

Done.

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

https://codereview.chromium.org/1376933006/diff/20001/src/ic/handler-compiler...
src/ic/handler-compiler.cc:459: bool stack_args =
StoreTransitionHelper::VirtualSlotArg();
On 2015/10/07 14:40:10, Jakob wrote:
> nit: this variable should probably be called "virtual_args" now.

Done.

https://codereview.chromium.org/1376933006/diff/20001/src/ic/ia32/handler-com...
File src/ic/ia32/handler-compiler-ia32.cc (right):

https://codereview.chromium.org/1376933006/diff/20001/src/ic/ia32/handler-com...
src/ic/ia32/handler-compiler-ia32.cc:367: //       current               after
RearrangeVectorAndSlot
On 2015/10/07 14:40:10, Jakob wrote:
> This legend helps a lot making the notation below understandable; however
maybe
> we don't need it at all any more? The description you put in the header file
> ("Pops the vector and slot into appropriate registers") might be enough.

I agree...it's not this complex card shuffling anymore. Done.

https://codereview.chromium.org/1376933006/diff/20001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/1376933006/diff/20001/src/ic/ic.cc#newcode2483
src/ic/ic.cc:2483: // We might have slot and vector, for a normal miss.
(slot(3), vector(4)).
On 2015/10/07 14:40:10, Jakob wrote:
> nit: drop the first full stop:
> // ... normal miss (slot(3), vector(4)).

Done.

https://codereview.chromium.org/1376933006/diff/20001/src/ic/ic.cc#newcode2486
src/ic/ic.cc:2486: // If we have 6 arguments, then we have map, slot, vector.
On 2015/10/07 14:40:10, Jakob wrote:
> nit: for consistency with the above, please add indices: "map(3), slot(4),
> vector(5)".

Done.

https://codereview.chromium.org/1376933006/diff/20001/src/interface-descripto...
File src/interface-descriptors.h (right):

https://codereview.chromium.org/1376933006/diff/20001/src/interface-descripto...
src/interface-descriptors.h:294: kVirtualSlotParameterCount = 5,
On 2015/10/07 14:40:10, Jakob wrote:
> I don't see this used anywhere.

Done.

https://codereview.chromium.org/1376933006/diff/20001/src/interface-descripto...
src/interface-descriptors.h:296: kParameterCount = 6
On 2015/10/07 14:40:10, Jakob wrote:
> I don't see this used anywhere either.

Done.

https://codereview.chromium.org/1376933006/diff/20001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/1376933006/diff/20001/src/isolate.h#newcode1311
src/isolate.h:1311: Address vector_store_virtual_slot_;
On 2015/10/07 14:40:10, Jakob wrote:
> I'm not overly happy with this pair of names, because "register" is very
> generic, whereas "slot" describes a very specific use case. I think either
both
> should have generic names, such as "virtual_register_one" and
> "virtual_register_two" (which would make sense if we assume that they'll be
used
> for other purposes in the future), or both names should indicate usage, e.g.
> "virtual_handler_register" and "virtual_slot_register".
> 
> I'm fine with leaving the renaming (+discussion, as necessary) to a future CL,
> though.

Good point. I'll go with virtual_handler_register and virtual_slot_register for
the moment. I hope we don't need to carry this kind of thing further! But if we
do, I agree it would be time to genericize these names.

[mvstanton, 2015-10-09 13:30:02]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2015-10-09 13:30:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1376933006/#ps40001
(title: "Code comments.")

[commit-bot: I haz the power, 2015-10-09 13:30:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1376933006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1376933006/40001

[commit-bot: I haz the power, 2015-10-09 13:32:22]

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

[commit-bot: I haz the power, 2015-10-09 13:32:22]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/6529)

[mvstanton, 2015-10-09 14:35:39]

mvstanton@chromium.org changed reviewers:
+ vogelheim@chromium.org, yangguo@chromium.org

[mvstanton, 2015-10-09 14:35:39]

Hi Daniel or Yang, could you look at the serialize.cc changes? Thanks,
--Michael

[Yang, 2015-10-12 06:16:16]

lgtm.

[mvstanton, 2015-10-12 07:10:25]

The CQ bit was checked by mvstanton@chromium.org

[commit-bot: I haz the power, 2015-10-12 07:10:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1376933006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1376933006/40001

[commit-bot: I haz the power, 2015-10-12 07:34:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-12 07:34:45]

Patchset 3 (id:??) landed as
https://crrev.com/2d4aeaad2fa3888c5e4cbfd30f6338e1fbf794e9
Cr-Commit-Position: refs/heads/master@{#31204}