Unify grow mode and stub kind

Unify grow mode and stub kind

In the process, ensure that transition-causing element stores handle all cases of the transitioned receiver map.

Committed: http://code.google.com/p/v8/source/detail?r=13850

[Toon Verwaest, 2013-03-06 14:47:28]

A first round of comments.
As you can see from the comments, I find the code in ic.cc and stub-cache.cc
quite difficult to navigate. Most of the time it's not immediately obvious to me
what state we are in. At least some extra helper-functions / comments /
restructured tests are required.

https://codereview.chromium.org/12390031/diff/1013/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1644
src/ic.cc:1644: store_mode = GetUntransitionedStoreMode(store_mode);
GetNonTransitioningStoreMode? The store mode itself is not untransitioned right?

https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1649
src/ic.cc:1649: GetReceiverMapsForStub(Handle<Code>(target()),
&target_receiver_maps);
Not directly related to this CL, but I think we can get rid of
GetReceiverMapsForStub, and just use
target()->FindAllMaps(&target_receiver_maps) instead.

https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1658
src/ic.cc:1658: // There are several special cases where a IC that is
MONOMORPHIC can still
an IC

https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1663
src/ic.cc:1663: Code::GetKeyedAccessStoreMode(target()->extra_ic_state());
As far as I can tell, we are not certain that the target() is actually a
store-element stub. The map we retrieved may be a random map of a prototype that
we checked, in case of a prototype chain check required for a, for example,
global property load. We can make odd decisions based on such maps. Probably
this will not result in wrong semantics, but may have weird performance
implications.

Can we use the extra_ic_state somehow to distinguish different types of Keyed
Store ICs? Non-element-stores should only use the extra_ic_state for encoding
the strict-mode afaik.

https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1685
src/ic.cc:1685: if (store_mode == IsGrowStoreMode(store_mode)) {
It seems like store_mode == IsGrowStoreMode(store_mode) is always false?

If you meant to write something like
IsNonTransitioningGrowStoreMode(store_mode), Isn't that a tautology within its
surrounding branch? If the original receiver maps were equal, and we didn't
transition (which is what the previous branch implies), we can't have a
transitioning store right?

In any case, this whole block of code is pretty hard to wade through. I'd prefer
if it were restructured to make it more obvious which cases it's handling.

https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1700
src/ic.cc:1700: Handle<Map> new_map = ComputeTransitionedMap(receiver,
store_mode);
Can we also call this transitioned_receiver_map, to be consistent with code
above?

https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1721
src/ic.cc:1721: if (old_store_mode != STORE_NO_TRANSITION) {
Meaning STORE_AND_GROW_NO_TRANSITION?

https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1724
src/ic.cc:1724: } else if (store_mode != old_store_mode) {
Can this happen? I presumed though would only be possible if

store_mode == STORE_NO_TRANSITION_IGNORE_OUT_OF_BOUNDS ||
store_mode == STORE_NO_TRANSITION_HANDLE_COW
(or old_store_mode == one of those).

But both cases are already excluded at the top of the function.

https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1773
src/ic.cc:1773: if (receiver->IsJSArray()) {
Maybe add a comment stating that for JSArrays (including external arrays),
negative indices are handled as properties, and thus don't fall under OOB.

https://codereview.chromium.org/12390031/diff/1013/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/12390031/diff/1013/src/objects.h#newcode208
src/objects.h:208: if (IsTransitionStoreMode(store_mode)) {
What about just

if (store_mode >= IGNORE_OUT_OF_BOUNDS) return store_mode;
if (store_mode >= STORE_AND_GROW) return STORE_AND_GROW_NO..;
return STORE_NO_TRANSITION;

https://codereview.chromium.org/12390031/diff/1013/src/stub-cache.cc
File src/stub-cache.cc (right):

https://codereview.chromium.org/12390031/diff/1013/src/stub-cache.cc#newcode440
src/stub-cache.cc:440: IsGrowStoreMode(store_mode));
It seems like we should not relax this condition. The stubs we compile only care
whether or not they are grow stubs. By allowing multiple store_modes; which are
encoded into the flags; we will have multiple identical copies of code that are
just different because of their metadata.

As far as I can tell from the callsites, if my comment regarding "if
(*previous_receiver_map == receiver->map())" is correct at least, the original
condition should still hold.

https://codereview.chromium.org/12390031/diff/1013/src/stub-cache.cc#newcode904
src/stub-cache.cc:904: StrictModeFlag strict_mode) {
I presume store_mode is 
ASSERT(stub_kind == KeyedStoreIC::STORE_NO_TRANSITION ||
       stub_kind == KeyedStoreIC::STORE_AND_GROW_NO_TRANSITION);

[danno, 2013-03-06 16:56:06]

Addressed review feedback, please take another look

https://codereview.chromium.org/12390031/diff/1013/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1644
src/ic.cc:1644: store_mode = GetUntransitionedStoreMode(store_mode);
On 2013/03/06 14:47:28, Toon Verwaest wrote:
> GetNonTransitioningStoreMode? The store mode itself is not untransitioned
right?

Done.

https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1649
src/ic.cc:1649: GetReceiverMapsForStub(Handle<Code>(target()),
&target_receiver_maps);
On 2013/03/06 14:47:28, Toon Verwaest wrote:
> Not directly related to this CL, but I think we can get rid of
> GetReceiverMapsForStub, and just use
> target()->FindAllMaps(&target_receiver_maps) instead.

Done.

https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1658
src/ic.cc:1658: // There are several special cases where a IC that is
MONOMORPHIC can still
On 2013/03/06 14:47:28, Toon Verwaest wrote:
> an IC

Done.

https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1663
src/ic.cc:1663: Code::GetKeyedAccessStoreMode(target()->extra_ic_state());
On 2013/03/06 14:47:28, Toon Verwaest wrote:
> As far as I can tell, we are not certain that the target() is actually a
> store-element stub. The map we retrieved may be a random map of a prototype
that
> we checked, in case of a prototype chain check required for a, for example,
> global property load. We can make odd decisions based on such maps. Probably
> this will not result in wrong semantics, but may have weird performance
> implications.
> 
> Can we use the extra_ic_state somehow to distinguish different types of Keyed
> Store ICs? Non-element-stores should only use the extra_ic_state for encoding
> the strict-mode afaik.

Done.

https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1685
src/ic.cc:1685: if (store_mode == IsGrowStoreMode(store_mode)) {
On 2013/03/06 14:47:28, Toon Verwaest wrote:
> It seems like store_mode == IsGrowStoreMode(store_mode) is always false?
> 
> If you meant to write something like
> IsNonTransitioningGrowStoreMode(store_mode), Isn't that a tautology within its
> surrounding branch? If the original receiver maps were equal, and we didn't
> transition (which is what the previous branch implies), we can't have a
> transitioning store right?
> 
> In any case, this whole block of code is pretty hard to wade through. I'd
prefer
> if it were restructured to make it more obvious which cases it's handling.

Done.

https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1700
src/ic.cc:1700: Handle<Map> new_map = ComputeTransitionedMap(receiver,
store_mode);
On 2013/03/06 14:47:28, Toon Verwaest wrote:
> Can we also call this transitioned_receiver_map, to be consistent with code
> above?

Done.

https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1721
src/ic.cc:1721: if (old_store_mode != STORE_NO_TRANSITION) {
On 2013/03/06 14:47:28, Toon Verwaest wrote:
> Meaning STORE_AND_GROW_NO_TRANSITION?

Well, this is currently correct, but for the next CL OOB and COW will also !=
STORE_NO_TRANSITION

https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1724
src/ic.cc:1724: } else if (store_mode != old_store_mode) {
As discussed offline, no change needed.

On 2013/03/06 14:47:28, Toon Verwaest wrote:
> Can this happen? I presumed though would only be possible if
> 
> store_mode == STORE_NO_TRANSITION_IGNORE_OUT_OF_BOUNDS ||
> store_mode == STORE_NO_TRANSITION_HANDLE_COW
> (or old_store_mode == one of those).
> 
> But both cases are already excluded at the top of the function.

https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1773
src/ic.cc:1773: if (receiver->IsJSArray()) {
On 2013/03/06 14:47:28, Toon Verwaest wrote:
> Maybe add a comment stating that for JSArrays (including external arrays),
> negative indices are handled as properties, and thus don't fall under OOB.

Done.

https://codereview.chromium.org/12390031/diff/1013/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/12390031/diff/1013/src/objects.h#newcode208
src/objects.h:208: if (IsTransitionStoreMode(store_mode)) {
On 2013/03/06 14:47:28, Toon Verwaest wrote:
> What about just
> 
> if (store_mode >= IGNORE_OUT_OF_BOUNDS) return store_mode;
> if (store_mode >= STORE_AND_GROW) return STORE_AND_GROW_NO..;
> return STORE_NO_TRANSITION;

Done.

https://codereview.chromium.org/12390031/diff/1013/src/stub-cache.cc
File src/stub-cache.cc (right):

https://codereview.chromium.org/12390031/diff/1013/src/stub-cache.cc#newcode440
src/stub-cache.cc:440: IsGrowStoreMode(store_mode));
On 2013/03/06 14:47:28, Toon Verwaest wrote:
> It seems like we should not relax this condition. The stubs we compile only
care
> whether or not they are grow stubs. By allowing multiple store_modes; which
are
> encoded into the flags; we will have multiple identical copies of code that
are
> just different because of their metadata.
> 
> As far as I can tell from the callsites, if my comment regarding "if
> (*previous_receiver_map == receiver->map())" is correct at least, the original
> condition should still hold.

Done.

https://codereview.chromium.org/12390031/diff/1013/src/stub-cache.cc#newcode904
src/stub-cache.cc:904: StrictModeFlag strict_mode) {
On 2013/03/06 14:47:28, Toon Verwaest wrote:
> I presume store_mode is 
> ASSERT(stub_kind == KeyedStoreIC::STORE_NO_TRANSITION ||
>        stub_kind == KeyedStoreIC::STORE_AND_GROW_NO_TRANSITION);

Done.

[Toon Verwaest, 2013-03-06 17:21:05]

lgtm

[danno, 2013-03-06 21:51:21]

Committed patchset #6 manually as r13850 (presubmit successful).