Crankshaft support for polymorphic array handling

Crankshaft support for polymorphic array handling

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

[Jakob Kummerow, 2011-06-15 09:35:46]

Please take a look. 

ARM and x64 aren't implemented yet (so this only compiles for ia32); I'll add
those if/when the rest looks good to you.

[fschneider, 2011-06-15 12:11:46]

drive-by comments:

http://codereview.chromium.org/7170012/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (left):

http://codereview.chromium.org/7170012/diff/1/src/hydrogen-instructions.h#old...
src/hydrogen-instructions.h:405: kJSArray = 0x701,        // 0000 0111 1000 0001
Accidental edit?

http://codereview.chromium.org/7170012/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/7170012/diff/1/src/hydrogen-instructions.h#new...
src/hydrogen-instructions.h:489: kUseGVN,  // Participate in Global Value
Numbering, i.e. elimination of
I'd insert the comment in a separate line before kUseGVN.

http://codereview.chromium.org/7170012/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

http://codereview.chromium.org/7170012/diff/1/src/hydrogen.h#newcode902
src/hydrogen.h:902: HValue* key);
Remove extra indentation.

http://codereview.chromium.org/7170012/diff/1/src/hydrogen.h#newcode910
src/hydrogen.h:910: HValue* key,
Align parameters with the first one.

http://codereview.chromium.org/7170012/diff/1/src/hydrogen.h#newcode919
src/hydrogen.h:919: HValue* key,
Indent parameters with the first one.

http://codereview.chromium.org/7170012/diff/1/src/type-info.cc
File src/type-info.cc (right):

http://codereview.chromium.org/7170012/diff/1/src/type-info.cc#newcode95
src/type-info.cc:95: Handle<Code> code(Code::cast(*map_or_code));
This will allocate a fresh handle from the pointer. Instead write:

Handle<Code> code = Handle<Code>::cast(map_or_code);

http://codereview.chromium.org/7170012/diff/1/src/type-info.cc#newcode121
src/type-info.cc:121: Handle<Code> code(Code::cast(*map_or_code));
Same here. Write:

Handle<Code> code = Handle<Code>::cast(map_or_code);

[Jakob Kummerow, 2011-06-15 12:28:58]

Thanks for the comments!

http://codereview.chromium.org/7170012/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (left):

http://codereview.chromium.org/7170012/diff/1/src/hydrogen-instructions.h#old...
src/hydrogen-instructions.h:405: kJSArray = 0x701,        // 0000 0111 1000 0001
On 2011/06/15 12:11:46, fschneider wrote:
> Accidental edit?

Nope. In a drive-by fashion, I noticed that comment and code weren't in sync and
decided to fix the comment.

http://codereview.chromium.org/7170012/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/7170012/diff/1/src/hydrogen-instructions.h#new...
src/hydrogen-instructions.h:489: kUseGVN,  // Participate in Global Value
Numbering, i.e. elimination of
On 2011/06/15 12:11:46, fschneider wrote:
> I'd insert the comment in a separate line before kUseGVN.

Done.

http://codereview.chromium.org/7170012/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

http://codereview.chromium.org/7170012/diff/1/src/hydrogen.h#newcode902
src/hydrogen.h:902: HValue* key);
On 2011/06/15 12:11:46, fschneider wrote:
> Remove extra indentation.

Done.
The presubmit.py script should be clever enough to catch these :-/

http://codereview.chromium.org/7170012/diff/1/src/hydrogen.h#newcode910
src/hydrogen.h:910: HValue* key,
On 2011/06/15 12:11:46, fschneider wrote:
> Align parameters with the first one.

Done.

http://codereview.chromium.org/7170012/diff/1/src/hydrogen.h#newcode919
src/hydrogen.h:919: HValue* key,
On 2011/06/15 12:11:46, fschneider wrote:
> Indent parameters with the first one.

Done.

http://codereview.chromium.org/7170012/diff/1/src/type-info.cc
File src/type-info.cc (right):

http://codereview.chromium.org/7170012/diff/1/src/type-info.cc#newcode95
src/type-info.cc:95: Handle<Code> code(Code::cast(*map_or_code));
On 2011/06/15 12:11:46, fschneider wrote:
> This will allocate a fresh handle from the pointer. Instead write:
> 
> Handle<Code> code = Handle<Code>::cast(map_or_code);

Done.

http://codereview.chromium.org/7170012/diff/1/src/type-info.cc#newcode121
src/type-info.cc:121: Handle<Code> code(Code::cast(*map_or_code));
On 2011/06/15 12:11:46, fschneider wrote:
> Same here. Write:
> 
> Handle<Code> code = Handle<Code>::cast(map_or_code);

Done.

[fschneider, 2011-06-16 13:21:28]

This looks already pretty good. 

Here are mostly comments to simplify the code a little.

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode3781
src/hydrogen.cc:3781: if (is_store) {
maybe shorten to:

return is_store ? BuildStoreKeyedGeneric(object, key, val) :
BuildLoadKeyedGeneric(object, key);

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode3855
src/hydrogen.cc:3855: HValue* key,
Align parameters in it fits 80 chars or else newline before the first
parameters.

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode3898
src/hydrogen.cc:3898: elements_kind = JSObject::ElementsKind(elements_kind + 1))
{
Too bad that ++ doesn't work ;)

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode3901
src/hydrogen.cc:3901: if (elements_kind == 1 && todo_external_array) {
Maybe write 

if (elements_kind > JSObject::FAST_ELEMENTS && todo_external_array)

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode3970
src/hydrogen.cc:3970: } else {
Do you handle FAST_DOUBLE_ELEMENTS at all? For now you could exclude then and
deopt on those like for DICTIONARY_ELEMENTS.

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode3973
src/hydrogen.cc:3973: switch (elements_kind) {
Could you have a helper inserting the conversion instructions since this switch
looks like a duplicate from the monomorphic load code?

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode4009
src/hydrogen.cc:4009: *has_side_effects |= access->HasSideEffects();
Isn't it always the case that (*has_side_effects == is_store)?

This would simplify the code here and in some other places.

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode4025
src/hydrogen.cc:4025: if (!is_store) {
Maybe shorten this to:

return is_store ? NULL : Pop();

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode4055
src/hydrogen.cc:4055: if (position != -1) {
Not sure, that you actually need to check for -1 (==kNoPosition) here. The
existing code just set the position in any case.

Does it work, if you just always assign a position?

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode4173
src/hydrogen.cc:4173: false,  // is_store,
This comment looks like commented out code. Maybe reformat or re-word to avoid
confusion,

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode5209
src/hydrogen.cc:5209: &has_side_effects);
Stores will always have a side effect.

[Jakob Kummerow, 2011-06-17 08:06:09]

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode3781
src/hydrogen.cc:3781: if (is_store) {
On 2011/06/16 13:21:28, fschneider wrote:
> maybe shorten to:
> 
> return is_store ? BuildStoreKeyedGeneric(object, key, val) :
> BuildLoadKeyedGeneric(object, key);

Done.

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode3855
src/hydrogen.cc:3855: HValue* key,
On 2011/06/16 13:21:28, fschneider wrote:
> Align parameters in it fits 80 chars or else newline before the first
> parameters.

Done.

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode3898
src/hydrogen.cc:3898: elements_kind = JSObject::ElementsKind(elements_kind + 1))
{
On 2011/06/16 13:21:28, fschneider wrote:
> Too bad that ++ doesn't work ;)

Yeah...

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode3901
src/hydrogen.cc:3901: if (elements_kind == 1 && todo_external_array) {
On 2011/06/16 13:21:28, fschneider wrote:
> Maybe write 
> 
> if (elements_kind > JSObject::FAST_ELEMENTS && todo_external_array)

No, because I only want to execute this if-block once, after FAST_ELEMENTS have
been handled and before all the different EXTERNAL_*_ELEMENTS are handled.
What I could do is "if (elements_kind == FAST_ELEMENTS + 1 && ...)"; or set
todo_external_array=false inside the if-block, but I don't think that'd be more
intuitive.

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode3970
src/hydrogen.cc:3970: } else {
On 2011/06/16 13:21:28, fschneider wrote:
> Do you handle FAST_DOUBLE_ELEMENTS at all? For now you could exclude then and
> deopt on those like for DICTIONARY_ELEMENTS.

Uhm... no, I don't handle them, because Hydrogen hasn't handled them before this
CL. I don't think it's a problem (support for them is a work in progress
anyway), but I've put in an explicit deopt anyway, just to be safe.

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode3973
src/hydrogen.cc:3973: switch (elements_kind) {
On 2011/06/16 13:21:28, fschneider wrote:
> Could you have a helper inserting the conversion instructions since this
switch
> looks like a duplicate from the monomorphic load code?

Good idea. Done.

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode4009
src/hydrogen.cc:4009: *has_side_effects |= access->HasSideEffects();
On 2011/06/16 13:21:28, fschneider wrote:
> Isn't it always the case that (*has_side_effects == is_store)?
> 
> This would simplify the code here and in some other places.

That assumption does indeed seem to be true in all cases this method handles,
but it's not always true when HandleKeyedElementAccess() is called, so we can't
live without the additional method parameter.
Also, I think it makes sense to keep asking the instruction whether it has side
effects or not, instead of hardcoding an assumption here. If we add an ASSERT to
protect the assumption, the gain in simplicity would be nullified.

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode4025
src/hydrogen.cc:4025: if (!is_store) {
On 2011/06/16 13:21:28, fschneider wrote:
> Maybe shorten this to:
> 
> return is_store ? NULL : Pop();

Done.

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode4055
src/hydrogen.cc:4055: if (position != -1) {
On 2011/06/16 13:21:28, fschneider wrote:
> Not sure, that you actually need to check for -1 (==kNoPosition) here. The
> existing code just set the position in any case.
> 
> Does it work, if you just always assign a position?

Done. Since kNoPosition is the default value anyway, that works.

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode4173
src/hydrogen.cc:4173: false,  // is_store,
On 2011/06/16 13:21:28, fschneider wrote:
> This comment looks like commented out code. Maybe reformat or re-word to avoid
> confusion,

I see your point. I've removed the comma. Is that enough? I think the best
convention for comments such as this one is to cite the parameter's name, but
that necessarily means that it looks like a variable name.

http://codereview.chromium.org/7170012/diff/3001/src/hydrogen.cc#newcode5209
src/hydrogen.cc:5209: &has_side_effects);
On 2011/06/16 13:21:28, fschneider wrote:
> Stores will always have a side effect.

Done.

[fschneider, 2011-06-17 08:59:34]

LGTM.

http://codereview.chromium.org/7170012/diff/6001/src/ast.cc
File src/ast.cc (right):

http://codereview.chromium.org/7170012/diff/6001/src/ast.cc#newcode611
src/ast.cc:611: // TODO(jkummerow): use constant instead of "4".
You could use sth. like kMaxKeyedLoadPolymorphism. We have such constants
already for normal polymorphic access and calls.

http://codereview.chromium.org/7170012/diff/6001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/7170012/diff/6001/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:2617: class HCompareWithConstant: public
HUnaryOperation {
Maybe name this HCompareConstantEq to indicate that this is only for ===

http://codereview.chromium.org/7170012/diff/6001/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:2621: ASSERT(op == Token::EQ || op ==
Token::EQ_STRICT);
I think you should for consistency with the other compare-operations only allow
EQ_STRICT. Normal == performs a valueOf-conversion and this instruction is
really a "===":

$ var o = {valueOf: function() { return 42; }}
$ o == 42
true
$ o === 42
false

http://codereview.chromium.org/7170012/diff/6001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/7170012/diff/6001/src/hydrogen.cc#newcode3996
src/hydrogen.cc:3996: // Let's see...
I'd add the same comment as above: "Stores always have side effects."

[Jakob Kummerow, 2011-06-17 17:43:14]

Here's the implementation for ARM and x64. 

Permission to land, sir?

http://codereview.chromium.org/7170012/diff/6001/src/ast.cc
File src/ast.cc (right):

http://codereview.chromium.org/7170012/diff/6001/src/ast.cc#newcode611
src/ast.cc:611: // TODO(jkummerow): use constant instead of "4".
On 2011/06/17 08:59:34, fschneider wrote:
> You could use sth. like kMaxKeyedLoadPolymorphism. We have such constants
> already for normal polymorphic access and calls.

Done.

http://codereview.chromium.org/7170012/diff/6001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/7170012/diff/6001/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:2617: class HCompareWithConstant: public
HUnaryOperation {
On 2011/06/17 08:59:34, fschneider wrote:
> Maybe name this HCompareConstantEq to indicate that this is only for ===

Done.

http://codereview.chromium.org/7170012/diff/6001/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:2621: ASSERT(op == Token::EQ || op ==
Token::EQ_STRICT);
On 2011/06/17 08:59:34, fschneider wrote:
> I think you should for consistency with the other compare-operations only
allow
> EQ_STRICT. Normal == performs a valueOf-conversion and this instruction is
> really a "===":
> 
> $ var o = {valueOf: function() { return 42; }}
> $ o == 42
> true
> $ o === 42
> false

Done.

http://codereview.chromium.org/7170012/diff/6001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/7170012/diff/6001/src/hydrogen.cc#newcode3996
src/hydrogen.cc:3996: // Let's see...
On 2011/06/17 08:59:34, fschneider wrote:
> I'd add the same comment as above: "Stores always have side effects."

Oops, this comment+ASSERT() was just a temporary addition that I forgot to
remove before uploading :-)

[fschneider, 2011-06-20 09:25:00]

ARM and x64 parts LGTM as well.

[Søren Thygesen Gjesse, 2011-06-21 07:09:03]

Drive by comments.

http://codereview.chromium.org/7170012/diff/10011/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/7170012/diff/10011/src/hydrogen.cc#newcode3913
src/hydrogen.cc:3913: if (elements_kind == 1 && todo_external_array) {
Shouldn't the constant 1 here be JSObject::FAST_ELEMENTS?

http://codereview.chromium.org/7170012/diff/10011/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/7170012/diff/10011/src/objects-inl.h#newcode2063
src/objects-inl.h:2063: // TODO(jkummerow): Investigate if it's possible to
s/INT/SMI/ here (and
Please avoid TODO comments with username. Create a bug and use TODO(bug number).

[Jakob Kummerow, 2011-06-21 07:49:23]

http://codereview.chromium.org/7170012/diff/10011/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/7170012/diff/10011/src/hydrogen.cc#newcode3913
src/hydrogen.cc:3913: if (elements_kind == 1 && todo_external_array) {
On 2011/06/21 07:09:04, Søren Gjesse wrote:
> Shouldn't the constant 1 here be JSObject::FAST_ELEMENTS?

No. FAST_ELEMENTS == 0. As the comment indicates, this if-block must be executed
after the first (== at the beginning of the second) run of the loop. I haven't
used a named constant instead of "1" because it doesn't matter what elements
kind 1 corresponds to, as long as it's the first one that comes after
FAST_ELEMENTS.

I could move this block to the end of the loop's code (after the end of the "if
(type_todo[elements_kind]) {...}" block), and accordingly change the condition,
but I personally don't think that would enhance readability.

http://codereview.chromium.org/7170012/diff/10011/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/7170012/diff/10011/src/objects-inl.h#newcode2063
src/objects-inl.h:2063: // TODO(jkummerow): Investigate if it's possible to
s/INT/SMI/ here (and
On 2011/06/21 07:09:04, Søren Gjesse wrote:
> Please avoid TODO comments with username. Create a bug and use TODO(bug
number).

http://codereview.chromium.org/7216007

[Søren Thygesen Gjesse, 2011-06-21 10:01:53]

http://codereview.chromium.org/7170012/diff/10011/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/7170012/diff/10011/src/hydrogen.cc#newcode3913
src/hydrogen.cc:3913: if (elements_kind == 1 && todo_external_array) {
On 2011/06/21 07:49:23, jkummerow wrote:
> On 2011/06/21 07:09:04, Søren Gjesse wrote:
> > Shouldn't the constant 1 here be JSObject::FAST_ELEMENTS?
> 
> No. FAST_ELEMENTS == 0. As the comment indicates, this if-block must be
executed
> after the first (== at the beginning of the second) run of the loop. I haven't
> used a named constant instead of "1" because it doesn't matter what elements
> kind 1 corresponds to, as long as it's the first one that comes after
> FAST_ELEMENTS.
> 
> I could move this block to the end of the loop's code (after the end of the
"if
> (type_todo[elements_kind]) {...}" block), and accordingly change the
condition,
> but I personally don't think that would enhance readability.

I see.

I think it should stay at the beginning of the loop, but you could consider
adding

    SECOND_ELEMENTS_KIND = FAST_DOUBLE_ELEMENTS,

to objects.h

and use SECOND_ELEMENTS_KIND that instead of 1, but then you should probably
also add

STATIC_ASSERT(JSObject::SECOND_ELEMENTS_KIND == 1);

Not sure whether that is better.