Use platform specific stubs for vector-based Load/KeyedLoad.

Use platform specific stubs for vector-based Load/KeyedLoad.

A hydrogen code stub is not the best approach because it builds a frame
and doesn't have the technology to discard roots at tail call exits.
Platform-specific stubs provide much better performance at this point.

R=verwaest@chromium.org

BUG=

Committed: https://crrev.com/34a1a76ddf30109f7b6cb60aa2651493ab38660a
Cr-Commit-Position: refs/heads/master@{#27235}

[mvstanton, 2015-03-06 09:37:42]

Hi Toon,
Here is the CL we discussed. I'll probably add a MIPS port as well to keep
builders happy.

I'll also turn off the flag which is on in the CL. But otherwise, it's ready for
a look. Note, per our discussion that x64 has additional code to avoid reading
the map word of the item in vector[slot]. For simplicity, the other platforms
don't do this yet.
Thanks!
--Michael

[mvstanton, 2015-03-09 12:26:55]

Patchset #3 (id:40001) has been deleted

[mvstanton, 2015-03-09 12:32:30]

Hi Toon,
Here is an updated CL with
1) FLAG_vector_ics off,
2) Addressed an issue where the number of slots per IC slot was not correctly
tied to --vector-ics. Now it is, with one slot per IC slot with --novector-ics,
otherwise 2.
Thanks for the look,
--Michael

[Toon Verwaest, 2015-03-11 17:55:02]

Just ia32 for now

https://codereview.chromium.org/988653003/diff/60001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/988653003/diff/60001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:117: void HandleMonoCase(HValue* weak_cell, HValue*
receiver, HValue* name,
HandleMonomorphicCase

https://codereview.chromium.org/988653003/diff/60001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/988653003/diff/60001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:4438: __ mov(feedback, FieldOperand(feedback,
FixedArray::OffsetOfElementAt(1)));
feedback -> handler?
Maybe just do Register handler = feedback;

https://codereview.chromium.org/988653003/diff/60001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:4446: Immediate(Smi::FromInt(2)));
This isn't possible for named loads afaict.

https://codereview.chromium.org/988653003/diff/60001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:4455: __ push(key);
As discussed, you don't need the key for named loads if you store it the next to
the feedback array on top-level; so you don't need to spill it.

https://codereview.chromium.org/988653003/diff/60001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:4499: __ cmp(weak_cell, FieldOperand(receiver, 0));
Register receiver_map = weak_cell;

https://codereview.chromium.org/988653003/diff/60001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:4501: __ mov(weak_cell, FieldOperand(vector, slot,
times_half_pointer_size,
Register code_object = weak_cell;

https://codereview.chromium.org/988653003/diff/60001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:4617: __ int3();  // We shouldn't get here.
drop int3(), I presume we can trust __jmp to do the right thing :)

https://codereview.chromium.org/988653003/diff/60001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/988653003/diff/60001/src/objects.h#newcode8804
src/objects.h:8804: static const int kFlagsOffset = Name::kSize;
Is this still necessary?
[At this point haven't looked at platform specific files yet, but I seem to
recall that this idea was discarded]

https://codereview.chromium.org/988653003/diff/60001/src/type-feedback-vector.cc
File src/type-feedback-vector.cc (right):

https://codereview.chromium.org/988653003/diff/60001/src/type-feedback-vector...
src/type-feedback-vector.cc:214: FixedArray::cast(*feedback_extra)->length() !=
length) {
< ?

https://codereview.chromium.org/988653003/diff/60001/src/type-feedback-vector.h
File src/type-feedback-vector.h (right):

https://codereview.chromium.org/988653003/diff/60001/src/type-feedback-vector...
src/type-feedback-vector.h:284: int ExtractMapsImpl(MapHandleList* maps) const;
Impl?

https://codereview.chromium.org/988653003/diff/60001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/988653003/diff/60001/src/x64/code-stubs-x64.c...
src/x64/code-stubs-x64.cc:4489: __ int3();  // We shouldn't get here.
Same here

[mvstanton, 2015-03-12 17:05:34]

Hi Toon,
Here you go. I've removed the x64 optimizations that just aren't worth it at
this moment (so objects.h change is gone). Thanks for good comments, addressed.
--Michael

https://codereview.chromium.org/988653003/diff/60001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/988653003/diff/60001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:117: void HandleMonoCase(HValue* weak_cell, HValue*
receiver, HValue* name,
On 2015/03/11 17:55:01, Toon Verwaest wrote:
> HandleMonomorphicCase

Happily all this code is dead. Removing.

https://codereview.chromium.org/988653003/diff/60001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/988653003/diff/60001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:4438: __ mov(feedback, FieldOperand(feedback,
FixedArray::OffsetOfElementAt(1)));
On 2015/03/11 17:55:02, Toon Verwaest wrote:
> feedback -> handler?
> Maybe just do Register handler = feedback;

Done.

https://codereview.chromium.org/988653003/diff/60001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:4446: Immediate(Smi::FromInt(2)));
On 2015/03/11 17:55:01, Toon Verwaest wrote:
> This isn't possible for named loads afaict.

I addressed this with a boolean, is_polymorphic.

https://codereview.chromium.org/988653003/diff/60001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:4455: __ push(key);
On 2015/03/11 17:55:02, Toon Verwaest wrote:
> As discussed, you don't need the key for named loads if you store it the next
to
> the feedback array on top-level; so you don't need to spill it.

Yeah, I don't want to load it from the array very much I think that might be
more expensive. It would introduce a lot of variations downwind (like
KeyedLoadIC::GenerateMiss(), that has to know about this case).

https://codereview.chromium.org/988653003/diff/60001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:4499: __ cmp(weak_cell, FieldOperand(receiver, 0));
On 2015/03/11 17:55:02, Toon Verwaest wrote:
> Register receiver_map = weak_cell;

Good idea, but I called it ic_map to indicate it's the map held by the ic, and
not necessary the map of the receiver.

https://codereview.chromium.org/988653003/diff/60001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:4501: __ mov(weak_cell, FieldOperand(vector, slot,
times_half_pointer_size,
On 2015/03/11 17:55:02, Toon Verwaest wrote:
> Register code_object = weak_cell;

Cool, but I called it handler per your good suggestion above.

https://codereview.chromium.org/988653003/diff/60001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:4617: __ int3();  // We shouldn't get here.
On 2015/03/11 17:55:01, Toon Verwaest wrote:
> drop int3(), I presume we can trust __jmp to do the right thing :)

Done.

https://codereview.chromium.org/988653003/diff/60001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/988653003/diff/60001/src/objects.h#newcode8804
src/objects.h:8804: static const int kFlagsOffset = Name::kSize;
On 2015/03/11 17:55:02, Toon Verwaest wrote:
> Is this still necessary?
> [At this point haven't looked at platform specific files yet, but I seem to
> recall that this idea was discarded]

Yes, I still want to do something in here but I'll move it out to a future CL.

https://codereview.chromium.org/988653003/diff/60001/src/type-feedback-vector.cc
File src/type-feedback-vector.cc (right):

https://codereview.chromium.org/988653003/diff/60001/src/type-feedback-vector...
src/type-feedback-vector.cc:214: FixedArray::cast(*feedback_extra)->length() !=
length) {
On 2015/03/11 17:55:02, Toon Verwaest wrote:
> < ?

Hmm, if the array in place is larger, then I'd have to fill the remaining
elements with a sentinel that the polymorphic handler then has to check for.
That's why I'll only reuse an existing array if it's the right size.

https://codereview.chromium.org/988653003/diff/60001/src/type-feedback-vector.h
File src/type-feedback-vector.h (right):

https://codereview.chromium.org/988653003/diff/60001/src/type-feedback-vector...
src/type-feedback-vector.h:284: int ExtractMapsImpl(MapHandleList* maps) const;
On 2015/03/11 17:55:02, Toon Verwaest wrote:
> Impl?

I'll reorganize this, providing a base implementation of these three functions
which LoadICNexus and KeyedLoadICNexus don't have to override.

https://codereview.chromium.org/988653003/diff/60001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/988653003/diff/60001/src/x64/code-stubs-x64.c...
src/x64/code-stubs-x64.cc:4489: __ int3();  // We shouldn't get here.
On 2015/03/11 17:55:02, Toon Verwaest wrote:
> Same here

Done.

[Benedikt Meurer, 2015-03-16 10:30:16]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2015-03-16 10:30:17]

ARM looks good to me.

https://codereview.chromium.org/988653003/diff/80001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/988653003/diff/80001/src/arm/code-stubs-arm.c...
src/arm/code-stubs-arm.cc:4467: // TODO(mvstanton): does this hold on ARM?
I guess it does not matter.

[Toon Verwaest, 2015-03-16 11:39:54]

Looking great.
Some final comments; we discussed all of them offline.

https://codereview.chromium.org/988653003/diff/80001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/988653003/diff/80001/src/x64/code-stubs-x64.c...
src/x64/code-stubs-x64.cc:4389: __ cmpp(length, Immediate(2));
cmpl, since SmiToInteger32 above does movl of the payload

https://codereview.chromium.org/988653003/diff/80001/src/x64/code-stubs-x64.c...
src/x64/code-stubs-x64.cc:4392: __ movp(counter, Immediate(2));
perhaps there's a shorter sequence to get a 0-extended 2?

https://codereview.chromium.org/988653003/diff/80001/src/x64/code-stubs-x64.c...
src/x64/code-stubs-x64.cc:4395: __ movp(array_map, FieldOperand(feedback,
counter, times_pointer_size,
cached_map would be nicer, array_map always confuses me with map of Array.

https://codereview.chromium.org/988653003/diff/80001/src/x64/code-stubs-x64.c...
src/x64/code-stubs-x64.cc:4413: __ Move(receiver_map,
masm->isolate()->factory()->heap_number_map());
LoadRoot?

https://codereview.chromium.org/988653003/diff/80001/src/x64/code-stubs-x64.c...
src/x64/code-stubs-x64.cc:4442: __ Cmp(ic_map,
masm->isolate()->factory()->heap_number_map());
CompareRoot is shorter / faster I think.

https://codereview.chromium.org/988653003/diff/80001/src/x64/code-stubs-x64.c...
src/x64/code-stubs-x64.cc:4528: __ Cmp(FieldOperand(feedback, 0),
factory->weak_cell_map());
CompareRoot

[mvstanton, 2015-03-16 13:59:40]

Thanks guys,
I've updated the CL.
--Michael

https://codereview.chromium.org/988653003/diff/80001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/988653003/diff/80001/src/arm/code-stubs-arm.c...
src/arm/code-stubs-arm.cc:4467: // TODO(mvstanton): does this hold on ARM?
On 2015/03/16 10:30:17, Benedikt Meurer wrote:
> I guess it does not matter.

Okay, I removed the TODO for now. Certainly there will be more looks at this
code in future.

https://codereview.chromium.org/988653003/diff/80001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/988653003/diff/80001/src/x64/code-stubs-x64.c...
src/x64/code-stubs-x64.cc:4389: __ cmpp(length, Immediate(2));
On 2015/03/16 11:39:54, Toon Verwaest wrote:
> cmpl, since SmiToInteger32 above does movl of the payload

Done.

https://codereview.chromium.org/988653003/diff/80001/src/x64/code-stubs-x64.c...
src/x64/code-stubs-x64.cc:4392: __ movp(counter, Immediate(2));
On 2015/03/16 11:39:54, Toon Verwaest wrote:
> perhaps there's a shorter sequence to get a 0-extended 2?

I looked into stuff, and movl(...) offers 6 bytes, which is the same as xorl(),
movb(), the smallest yet correct sequence I could find. movl() is okay because
the 32 bit operation clears the upper bits (whereas movb() sadly doesn't :p).

I looked into changing registers around...rdi usage here can save a byte, but
the overall code size increases if I take rdi away from it's current purpose
(feedback value). And the other non-extended registers are all busy with the
LOAD_IC convention. It stands to reason that playing with that too much would
increase code size elsewhere.

https://codereview.chromium.org/988653003/diff/80001/src/x64/code-stubs-x64.c...
src/x64/code-stubs-x64.cc:4395: __ movp(array_map, FieldOperand(feedback,
counter, times_pointer_size,
On 2015/03/16 11:39:54, Toon Verwaest wrote:
> cached_map would be nicer, array_map always confuses me with map of Array.

Done.

https://codereview.chromium.org/988653003/diff/80001/src/x64/code-stubs-x64.c...
src/x64/code-stubs-x64.cc:4413: __ Move(receiver_map,
masm->isolate()->factory()->heap_number_map());
On 2015/03/16 11:39:54, Toon Verwaest wrote:
> LoadRoot?

Done.

https://codereview.chromium.org/988653003/diff/80001/src/x64/code-stubs-x64.c...
src/x64/code-stubs-x64.cc:4442: __ Cmp(ic_map,
masm->isolate()->factory()->heap_number_map());
On 2015/03/16 11:39:54, Toon Verwaest wrote:
> CompareRoot is shorter / faster I think.

Done.

https://codereview.chromium.org/988653003/diff/80001/src/x64/code-stubs-x64.c...
src/x64/code-stubs-x64.cc:4442: __ Cmp(ic_map,
masm->isolate()->factory()->heap_number_map());
On 2015/03/16 11:39:54, Toon Verwaest wrote:
> CompareRoot is shorter / faster I think.

Done.

https://codereview.chromium.org/988653003/diff/80001/src/x64/code-stubs-x64.c...
src/x64/code-stubs-x64.cc:4528: __ Cmp(FieldOperand(feedback, 0),
factory->weak_cell_map());
On 2015/03/16 11:39:54, Toon Verwaest wrote:
> CompareRoot

Done.

[Toon Verwaest, 2015-03-16 14:14:53]

lgtm

[Benedikt Meurer, 2015-03-16 16:34:16]

LGTM for ARM. Land it! :-)

[mvstanton, 2015-03-17 11:06:27]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2015-03-17 11:06:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org, bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/988653003/#ps180001
(title: "Included the MIPs port from Paul and Akos.")

[commit-bot: I haz the power, 2015-03-17 11:06:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/988653003/180001

[commit-bot: I haz the power, 2015-03-17 11:28:15]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2015-03-17 11:28:44]

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