Clear next map word when folding allocations into js arrays.

Clear next map word when folding allocations into js arrays.

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

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

[mvstanton, 2013-08-26 10:01:27]

Hi Hannes, have a look at my comments, thx!

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

https://codereview.chromium.org/22915007/diff/5001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:3244: if
(dominator_allocate_instr->type().Equals(HType::JSArray()) &&
Actually here could you make use of Map::CanTrackAllocationSite(), or at least
extend/generalize that function to also work with an HType? That is a
centralized place for deciding which types have "the right" to use allocation
mementos.

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

https://codereview.chromium.org/22915007/diff/5001/src/hydrogen-instructions....
src/hydrogen-instructions.h:5149: bool clear_next_map_word_;
Actually, do you need this boolean, since you handle things directly during the
dominance walk.

[Hannes Payer (out of office), 2013-08-26 11:51:07]

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

https://codereview.chromium.org/22915007/diff/5001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:3244: if
(dominator_allocate_instr->type().Equals(HType::JSArray()) &&
On 2013/08/26 10:01:27, mvstanton wrote:
> Actually here could you make use of Map::CanTrackAllocationSite(), or at least
> extend/generalize that function to also work with an HType? That is a
> centralized place for deciding which types have "the right" to use allocation
> mementos.

Thanks for the comment, makes sense. I made a static check method in the
AlloationSite class.

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

https://codereview.chromium.org/22915007/diff/5001/src/hydrogen-instructions....
src/hydrogen-instructions.h:5149: bool clear_next_map_word_;
On 2013/08/26 10:01:27, mvstanton wrote:
> Actually, do you need this boolean, since you handle things directly during
the
> dominance walk.

Yes, otherwise would create n stores, where n is the number of folding
operations.

[mvstanton, 2013-08-26 12:00:01]

lgtm

[Michael Starzinger, 2013-08-26 15:03:35]

not lgtm.

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

https://codereview.chromium.org/22915007/diff/10001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3249: if
(AllocationSite::CanTrack(dominator_allocate_instr->instance_type()) &&
This doesn't work properly for nested JSArray objects that are all folded
together. Imagine the following sequence ...

v1 = HAllocate(JS_OBJECT_TYPE);
v2 = HAllocate(JS_ARRAY_TYPE + ALLOCATION_SITE);
v3 = HAllocate(WHATEVER);

Here "v1" isn't tracked with an allocation site, so this path never triggers. On
the other hand "v2" is tracked with an allocation site, but garbage that happens
to be at the position of "v3" is not smashed.

https://codereview.chromium.org/22915007/diff/10001/test/mjsunit/allocation-f...
File test/mjsunit/allocation-folding.js (right):

https://codereview.chromium.org/22915007/diff/10001/test/mjsunit/allocation-f...
test/mjsunit/allocation-folding.js:59: result = doubles();
suggestion: If you properly scope each test in a separate function scope, you
could have local variables without accidentally re-declaring or re-using
previous ones.

https://codereview.chromium.org/22915007/diff/10001/test/mjsunit/allocation-f...
test/mjsunit/allocation-folding.js:81: // Test allocation folding of over a
branch.
nit: s/of//

[Michael Starzinger, 2013-08-26 15:12:39]

As discussed offline: I think this can be realized by not remembering the
instance type in HAllocate instructions (hence it also cannot become invalid for
folded allocations). Instead we could track a
"depends_on_following_garbage_being_zapped" field (name might need some work).
That is initially set depending on whether the instance type is tracked or not.
On folding, the field gets reset depending on whatever the dominated instruction
requests.

[mvstanton, 2013-08-26 15:24:33]

Thx for catching that Michael!

[Hannes Payer (out of office), 2013-08-26 16:42:57]

On 2013/08/26 15:24:33, mvstanton wrote:
> Thx for catching that Michael!

Thx for the catch Michael, the original version would have just worked for a
simple fast literal.

[Michael Starzinger, 2013-08-27 08:41:24]

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

https://codereview.chromium.org/22915007/diff/21001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3246: #else
The #ifdef is not entirely correct. If we compile with the heap verifier but
don't enable it, nobody will smash garbage.

[Hannes Payer (out of office), 2013-08-27 12:50:54]

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

https://codereview.chromium.org/22915007/diff/21001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3246: #else
On 2013/08/27 08:41:24, Michael Starzinger wrote:
> The #ifdef is not entirely correct. If we compile with the heap verifier but
> don't enable it, nobody will smash garbage.

Done.

[Michael Starzinger, 2013-08-27 14:03:28]

LGTM with one final nit.

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

https://codereview.chromium.org/22915007/diff/29001/src/hydrogen-instructions...
src/hydrogen-instructions.h:5108: void ClearNextMapWord(int offset);
nit: Should be private.

[Hannes Payer (out of office), 2013-08-27 14:13:02]

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

https://codereview.chromium.org/22915007/diff/29001/src/hydrogen-instructions...
src/hydrogen-instructions.h:5108: void ClearNextMapWord(int offset);
On 2013/08/27 14:03:28, Michael Starzinger wrote:
> nit: Should be private.

Done.

[Hannes Payer (out of office), 2013-08-28 08:39:55]

Committed patchset #9 manually as r16381 (presubmit successful).