Create optimized inline versions of Map and Set initialization

Create optimized inline versions of Map and Set initialization

[adamk, 2014-12-08 21:41:13]

adamk@chromium.org changed reviewers:
+ dslomov@chromium.org

[adamk, 2014-12-08 21:41:14]

https://codereview.chromium.org/779173010/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/779173010/diff/1/src/hydrogen.cc#newcode12660
src/hydrogen.cc:12660: Add<HAllocate>(size, HType::HeapObject(), NOT_TENURED,
FIXED_ARRAY_TYPE);
One question about HAllocate: do I need to add code here to fill the rest of
this space with undefineds, to mirror what Heap::AllocateFixedArray() does?

[Dmitry Lomov (no reviews), 2014-12-09 12:05:40]

https://codereview.chromium.org/779173010/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/779173010/diff/1/src/hydrogen.cc#newcode12660
src/hydrogen.cc:12660: Add<HAllocate>(size, HType::HeapObject(), NOT_TENURED,
FIXED_ARRAY_TYPE);
On 2014/12/08 21:41:14, adamk wrote:
> One question about HAllocate: do I need to add code here to fill the rest of
> this space with undefineds, to mirror what Heap::AllocateFixedArray() does?

Yes. See HOGB::BuildAllocateFixedTypedArray for inspiration: 
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/hydrogen.cc...

[adamk, 2014-12-09 19:24:23]

https://codereview.chromium.org/779173010/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/779173010/diff/1/src/hydrogen.cc#newcode12660
src/hydrogen.cc:12660: Add<HAllocate>(size, HType::HeapObject(), NOT_TENURED,
FIXED_ARRAY_TYPE);
On 2014/12/09 12:05:39, Dmitry Lomov (chromium) wrote:
> On 2014/12/08 21:41:14, adamk wrote:
> > One question about HAllocate: do I need to add code here to fill the rest of
> > this space with undefineds, to mirror what Heap::AllocateFixedArray() does?
> 
> Yes. See HOGB::BuildAllocateFixedTypedArray for inspiration: 
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/hydrogen.cc...

Added a bunch of proper initialization. Now the code's a bit funny, since I have
one "unrolled" loop (using a C++ for loop) and one "proper" loop (using
LoopBuilder). But these loops are 2 and 4 (constant) iterations each. I figure
they should either both be "unrolled" or both use LoopBuilder. Thoughts?

[Dmitry Lomov (no reviews), 2014-12-09 20:06:57]

On 2014/12/09 19:24:23, adamk wrote:
> https://codereview.chromium.org/779173010/diff/1/src/hydrogen.cc
> File src/hydrogen.cc (right):
> 
> https://codereview.chromium.org/779173010/diff/1/src/hydrogen.cc#newcode12660
> src/hydrogen.cc:12660: Add<HAllocate>(size, HType::HeapObject(), NOT_TENURED,
> FIXED_ARRAY_TYPE);
> On 2014/12/09 12:05:39, Dmitry Lomov (chromium) wrote:
> > On 2014/12/08 21:41:14, adamk wrote:
> > > One question about HAllocate: do I need to add code here to fill the rest
of
> > > this space with undefineds, to mirror what Heap::AllocateFixedArray()
does?
> > 
> > Yes. See HOGB::BuildAllocateFixedTypedArray for inspiration: 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/hydrogen.cc...
> 
> Added a bunch of proper initialization. Now the code's a bit funny, since I
have
> one "unrolled" loop (using a C++ for loop) and one "proper" loop (using
> LoopBuilder). But these loops are 2 and 4 (constant) iterations each. I figure
> they should either both be "unrolled" or both use LoopBuilder. Thoughts?

If it is constant, just unroll.

[adamk, 2014-12-09 20:25:20]

Unrolled in latest patch.

[Dmitry Lomov (no reviews), 2014-12-09 20:48:52]

lgtm % comment

https://codereview.chromium.org/779173010/diff/80001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/779173010/diff/80001/src/hydrogen.cc#newcode1...
src/hydrogen.cc:12693: Add<HStoreKeyed>(table, Add<HConstant>(i), not_found,
FAST_ELEMENTS);
This should use HStoreNamedField.

https://codereview.chromium.org/779173010/diff/80001/src/hydrogen.cc#newcode1...
src/hydrogen.cc:12699: Add<HStoreKeyed>(table, Add<HConstant>(i), undefined,
FAST_ELEMENTS);
Ditto.

[adamk, 2014-12-09 21:16:59]

https://codereview.chromium.org/779173010/diff/80001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/779173010/diff/80001/src/hydrogen.cc#newcode1...
src/hydrogen.cc:12693: Add<HStoreKeyed>(table, Add<HConstant>(i), not_found,
FAST_ELEMENTS);
On 2014/12/09 20:48:51, Dmitry Lomov (chromium) wrote:
> This should use HStoreNamedField.

What do you have in mind? There's a variety of HObjectAccesses I could add, from
something as simple to ForFixedArrayIndex() to two different methods, like
ForOrderedHashTableBucket() and ForOrderedHashTableDataTableIndex().

[Dmitry Lomov (no reviews), 2014-12-09 21:31:56]

https://codereview.chromium.org/779173010/diff/80001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/779173010/diff/80001/src/hydrogen.cc#newcode1...
src/hydrogen.cc:12693: Add<HStoreKeyed>(table, Add<HConstant>(i), not_found,
FAST_ELEMENTS);
On 2014/12/09 21:16:59, adamk wrote:
> On 2014/12/09 20:48:51, Dmitry Lomov (chromium) wrote:
> > This should use HStoreNamedField.
> 
> What do you have in mind? There's a variety of HObjectAccesses I could add,
from
> something as simple to ForFixedArrayIndex() to two different methods, like
> ForOrderedHashTableBucket() and ForOrderedHashTableDataTableIndex().

Two different methods sound most semantically clear to me here

[adamk, 2014-12-09 21:45:27]

Done, switched to StoreNamedField and in the process shortened some of my
constant names to make this a little easier to read.

[Dmitry Lomov (no reviews), 2014-12-09 21:50:07]

still lgtm

[adamk, 2014-12-10 18:20:29]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2014-12-10 18:21:07]

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

[commit-bot: I haz the power, 2014-12-10 18:47:30]

Committed patchset #6 (id:100001)