Properly filter types using the initial map from HAllocate.

Properly filter types using the initial map from HAllocate.

R=mstarzinger@chromium.org

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

[Toon Verwaest, 2013-09-05 18:16:17]

PTAL

[Michael Starzinger, 2013-09-05 20:07:22]

LGTM with comments.

https://codereview.chromium.org/23615012/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/23615012/diff/1/src/ast.h#newcode291
src/ast.h:291: void FilterForPossibleTransitions(Map* root_map, Zone* zone) {
The "zone" parameter is obsolete, as we only remove elements with this method
and don't add new ones.

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

https://codereview.chromium.org/23615012/diff/1/src/hydrogen.cc#newcode5706
src/hydrogen.cc:5706: if (types != NULL && obj->HasMonomorphicJSObjectType()) {
suggestion: Does it make sense to factor this out into a static helper method
that computes the receiver types for us? I was thinking along the lines of a
method with either of the following signatures (I prefer the first one)...

// Return true if receiver is monomorphic.
static bool ComputeReceiverTypes(Expression* expr,
                                 Object* receiver,
                                 SmallMapList** types);

static SmallMapList* ComputeReceiverTypes(Expression* expr,
                                          Object* receiver,
                                          bool* monomorphic);

https://codereview.chromium.org/23615012/diff/1/src/hydrogen.cc#newcode6991
src/hydrogen.cc:6991: 
nit: Drop one of the two empty newlines.

https://codereview.chromium.org/23615012/diff/1/src/hydrogen.cc#newcode7005
src/hydrogen.cc:7005: receiver_map = (types == NULL || types->is_empty())
I know it's not part of this change, but can "types" really be NULL or empty of
expr->IsMonomorphic() reports true? The lines 7003 until 7008 look needlessly
verbose.

[Toon Verwaest, 2013-09-06 09:23:12]

Addressed comments, PTAL again.

https://chromiumcodereview.appspot.com/23615012/diff/1/src/ast.h
File src/ast.h (right):

https://chromiumcodereview.appspot.com/23615012/diff/1/src/ast.h#newcode291
src/ast.h:291: void FilterForPossibleTransitions(Map* root_map, Zone* zone) {
On 2013/09/05 20:07:23, Michael Starzinger wrote:
> The "zone" parameter is obsolete, as we only remove elements with this method
> and don't add new ones.

Done.

https://chromiumcodereview.appspot.com/23615012/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/23615012/diff/1/src/hydrogen.cc#newcod...
src/hydrogen.cc:5706: if (types != NULL && obj->HasMonomorphicJSObjectType()) {
On 2013/09/05 20:07:23, Michael Starzinger wrote:
> suggestion: Does it make sense to factor this out into a static helper method
> that computes the receiver types for us? I was thinking along the lines of a
> method with either of the following signatures (I prefer the first one)...
> 
> // Return true if receiver is monomorphic.
> static bool ComputeReceiverTypes(Expression* expr,
>                                  Object* receiver,
>                                  SmallMapList** types);
> 
> static SmallMapList* ComputeReceiverTypes(Expression* expr,
>                                           Object* receiver,
>                                           bool* monomorphic);

Done.

[Michael Starzinger, 2013-09-06 09:34:40]

Still LGTM.

https://codereview.chromium.org/23615012/diff/7001/src/ast.cc
File src/ast.cc (right):

https://codereview.chromium.org/23615012/diff/7001/src/ast.cc#newcode655
src/ast.cc:655: holder_ = GetPrototypeForPrimitiveCheck(check_type_,
oracle->isolate());
Does it hold that "receiver_types_.IsEmpty()" down here? If so, then we should
add an assert.

[Toon Verwaest, 2013-09-06 11:24:28]

https://codereview.chromium.org/23615012/diff/7001/src/ast.cc
File src/ast.cc (right):

https://codereview.chromium.org/23615012/diff/7001/src/ast.cc#newcode655
src/ast.cc:655: holder_ = GetPrototypeForPrimitiveCheck(check_type_,
oracle->isolate());
The receiver_types_ are Clear()ed right above, so I don't think it's necessary.
Will land as is.

On 2013/09/06 09:34:40, Michael Starzinger wrote:
> Does it hold that "receiver_types_.IsEmpty()" down here? If so, then we should
> add an assert.

[Toon Verwaest, 2013-09-06 11:32:57]

Committed patchset #2 manually as r16572 (presubmit successful).