Added allocation folding support for old space allocations.

Added allocation folding support for old space allocations.

BUG=
R=mstarzinger@chromium.org, titzer@chromium.org

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

[titzer, 2013-08-06 12:24:12]

https://codereview.chromium.org/22378003/diff/15001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/22378003/diff/15001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3339: 
This method is getting pretty big; suggest pulling out the bodies of various
if's into their own helper methods. Also a helper to create an int32 constant
and insert it after a particular instruction would probably help clean this up a
lot.

https://codereview.chromium.org/22378003/diff/15001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3394:
HConstant::cast(dominator_free_space_size)->GetInteger32Constant();
I think this cast is redundant.

https://codereview.chromium.org/22378003/diff/15001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/22378003/diff/15001/src/hydrogen-instructions...
src/hydrogen-instructions.h:5055: void set_dominating_allocate(HAllocate*
allocate) {
Don't add setters for fields that should not be modified outside the class.

[Hannes Payer (out of office), 2013-08-07 07:48:01]

https://codereview.chromium.org/22378003/diff/15001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/22378003/diff/15001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3339: 
On 2013/08/06 12:24:12, titzer wrote:
> This method is getting pretty big; suggest pulling out the bodies of various
> if's into their own helper methods. Also a helper to create an int32 constant
> and insert it after a particular instruction would probably help clean this up
a
> lot.

Done.

https://codereview.chromium.org/22378003/diff/15001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3394:
HConstant::cast(dominator_free_space_size)->GetInteger32Constant();
On 2013/08/06 12:24:12, titzer wrote:
> I think this cast is redundant.

Done.

https://codereview.chromium.org/22378003/diff/15001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/22378003/diff/15001/src/hydrogen-instructions...
src/hydrogen-instructions.h:5055: void set_dominating_allocate(HAllocate*
allocate) {
On 2013/08/06 12:24:12, titzer wrote:
> Don't add setters for fields that should not be modified outside the class.

I made all setters and getters used just by HAllocate private.

[titzer, 2013-08-07 11:53:44]

https://codereview.chromium.org/22378003/diff/29002/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/22378003/diff/29002/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3484: HConstant* filler_map = HConstant::New(
You can use CreateAndInsert() here.

https://codereview.chromium.org/22378003/diff/29002/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3496: zone, context(), filler_size, store_map,
false);
Is there any way to make this an InsertBefore() as well? This boolean
before/after parameter is cryptic.

https://codereview.chromium.org/22378003/diff/29002/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/22378003/diff/29002/src/hydrogen-instructions...
src/hydrogen-instructions.h:5113: HAllocate* dominating_allocate() const {
Private getters and setters for the fields are altogether unnecessary.

[Hannes Payer (out of office), 2013-08-07 12:47:55]

https://codereview.chromium.org/22378003/diff/29002/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/22378003/diff/29002/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3484: HConstant* filler_map = HConstant::New(
On 2013/08/07 11:53:44, titzer wrote:
> You can use CreateAndInsert() here.

No, I cannot - different HConstant. Actually I am not convinced that
CreateAndInsert improves the code significantly. It is tailored for the int32_t
case.

https://codereview.chromium.org/22378003/diff/29002/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3496: zone, context(), filler_size, store_map,
false);
On 2013/08/07 11:53:44, titzer wrote:
> Is there any way to make this an InsertBefore() as well? This boolean
> before/after parameter is cryptic.

No, it would just get to complicated. I prefer keeping the logical chain of
operations here. I added two static functions CreateAndInsertBefore and
CreateAndInsertAfter.

https://codereview.chromium.org/22378003/diff/29002/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/22378003/diff/29002/src/hydrogen-instructions...
src/hydrogen-instructions.h:5113: HAllocate* dominating_allocate() const {
On 2013/08/07 11:53:44, titzer wrote:
> Private getters and setters for the fields are altogether unnecessary.

Done.

[titzer, 2013-08-07 13:00:25]

lgtm

[Michael Starzinger, 2013-08-09 09:22:58]

https://codereview.chromium.org/22378003/diff/38001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/22378003/diff/38001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3431: // space memory.
nit: Seems like an "otherwise" is missing at the end of this sentence.

https://codereview.chromium.org/22378003/diff/38001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3452:
UpdateFreeSpaceFiller(dominator_free_space_size,
It seems these methods are called on the allocations being folded away. I think
it would make much more sense to actually call them on the dominating
allocation. These methods look like "bastard static functions not being static".
This would also make it easier to address the  comment in
HAllocate::UpdateFreeSpaceFiller.

https://codereview.chromium.org/22378003/diff/38001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3459: CreateFreeSpaceFiller(dominator,
dominator_size, current_size);
Addressing the comment above would allow you to not pass the dominator as a
parameter but as the receiver instead.

https://codereview.chromium.org/22378003/diff/38001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3471:
old_filler_size->DeleteAndReplaceWith(new_free_space_size);
This looks dangerous, can you guarantee that "old_filler_size" is only used by
the dominating allocation and not something else as well? Can we either assert
that this is the only use or set Hallocate::filler_free_space_size explicitly?

https://codereview.chromium.org/22378003/diff/38001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/22378003/diff/38001/src/hydrogen-instructions...
src/hydrogen-instructions.h:3419: Representation r = Representation::Tagged(),
As discussed offline: Please don't add default parameters to this constructor.
This constructor should only be called for very well-known values, where all the
arguments can be explicitly specified.

[Hannes Payer (out of office), 2013-08-09 11:10:43]

https://codereview.chromium.org/22378003/diff/38001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/22378003/diff/38001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3452:
UpdateFreeSpaceFiller(dominator_free_space_size,
On 2013/08/09 09:22:58, Michael Starzinger wrote:
> It seems these methods are called on the allocations being folded away. I
think
> it would make much more sense to actually call them on the dominating
> allocation. These methods look like "bastard static functions not being
static".
> This would also make it easier to address the  comment in
> HAllocate::UpdateFreeSpaceFiller.

Done, much better now!

https://codereview.chromium.org/22378003/diff/38001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3459: CreateFreeSpaceFiller(dominator,
dominator_size, current_size);
On 2013/08/09 09:22:58, Michael Starzinger wrote:
> Addressing the comment above would allow you to not pass the dominator as a
> parameter but as the receiver instead.

Done.

https://codereview.chromium.org/22378003/diff/38001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3471:
old_filler_size->DeleteAndReplaceWith(new_free_space_size);
On 2013/08/09 09:22:58, Michael Starzinger wrote:
> This looks dangerous, can you guarantee that "old_filler_size" is only used by
> the dominating allocation and not something else as well? Can we either assert
> that this is the only use or set Hallocate::filler_free_space_size explicitly?

Done.

https://codereview.chromium.org/22378003/diff/38001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/22378003/diff/38001/src/hydrogen-instructions...
src/hydrogen-instructions.h:3419: Representation r = Representation::Tagged(),
On 2013/08/09 09:22:58, Michael Starzinger wrote:
> As discussed offline: Please don't add default parameters to this constructor.
> This constructor should only be called for very well-known values, where all
the
> arguments can be explicitly specified.

Done.

[Michael Starzinger, 2013-08-09 12:46:08]

LGTM.

[Hannes Payer (out of office), 2013-08-13 08:57:32]

On 2013/08/09 12:46:08, Michael Starzinger wrote:
> LGTM.

Update: mstarzinger model checker found a bug when old space allocations of
different old spaces do not occur in alternating order. Adding the filler map
right before the dominating allocate of the "other" old space should fix that
problem.

[titzer, 2013-08-26 09:50:24]

https://codereview.chromium.org/22378003/diff/63001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/22378003/diff/63001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3366:
filler_free_space_size_->DeleteAndReplaceWith(new_free_space_size);
Whoa. I don't think you want to DeleteAndReplaceWith() here, because the size is
just an integer constant, and if it is used for more than the allocation in
question, then you've just updated the other uses of that constant to point to
the new constant. That would be an ouchie. This probably doesn't happen now,
because I think you insert a new constant each time, but you shouldn't assume
this will hold in the future.

[Hannes Payer (out of office), 2013-08-26 13:34:08]

https://codereview.chromium.org/22378003/diff/63001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/22378003/diff/63001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3366:
filler_free_space_size_->DeleteAndReplaceWith(new_free_space_size);
On 2013/08/26 09:50:24, titzer wrote:
> Whoa. I don't think you want to DeleteAndReplaceWith() here, because the size
is
> just an integer constant, and if it is used for more than the allocation in
> question, then you've just updated the other uses of that constant to point to
> the new constant. That would be an ouchie. This probably doesn't happen now,
> because I think you insert a new constant each time, but you shouldn't assume
> this will hold in the future.

Yeah, let's play safe. Done.

[titzer, 2013-08-26 16:54:52]

lgtm

[Hannes Payer (out of office), 2013-08-27 07:42:32]

Committed patchset #12 manually as r16346 (presubmit successful).