Handle array construction in native code (x64 version)

Handle array construction in native code (x64 version).

Ported the handle array construction in native code to x64. See http://codereview.chromium.org/193125 for details.

Please take a closer look of my use of the macro assembler Smi abstractions.

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

[Lasse Reichstein, 2009-09-22 11:29:33]

LGTM

http://codereview.chromium.org/209048/diff/1/2
File src/x64/builtins-x64.cc (right):

http://codereview.chromium.org/209048/diff/1/2#newcode480
Line 480: int holes,
Rename "holes" to "initial_capacity".

http://codereview.chromium.org/209048/diff/1/2#newcode494
Line 494: __ AllocateObjectInNewSpace(size,
You are not really allocating an object in NewSpace, but just a patch of memory
for two objects.
Should the allocate function be renamed to AllocateInNewSpace(...)?

http://codereview.chromium.org/209048/diff/1/2#newcode553
Line 553: __ Move(Operand(scratch1, 0), Factory::the_hole_value());
You should use the scratch3 register for the "hole" value here as well. The way
it is done will reload the value into kScratchRegister, using a a 64-bit
immediate, on every round (adding 10+ bytes to the loop).

http://codereview.chromium.org/209048/diff/1/2#newcode674
Line 674: __ j(below, &loop);
Why not put the test before the loop, in "while"-style?

http://codereview.chromium.org/209048/diff/1/2#newcode720
Line 720: Condition cond = __ CheckNotPositiveSmi(rdx);
rename "cond" to not_positive_smi. Makes the jump read much better.
(Or use JumpIfNotPositiveSmi(rdx, call_generic_code), if it exists).

http://codereview.chromium.org/209048/diff/1/2#newcode726
Line 726: __ cmpq(rdx, Immediate(JSObject::kInitialMaxFastElementArray <<
kSmiTagSize));
Don't create smis like this (if the immediate is really a smi, and not just
tagged as a smi).
Use: 
  __ JumpIfSmiEqualsConstant(rdx, JSObject::kInitialMxFastElementArray,
call_generic_code)

http://codereview.chromium.org/209048/diff/1/2#newcode826
Line 826: __ testq(rbx, Immediate(kSmiTagMask));
Use 
 Condition is_smi = CheckSmi(rbx);
... as long as NULL is a smi. Check that with ASSERT_EQ(0, kSmiTag);

http://codereview.chromium.org/209048/diff/1/2#newcode862
Line 862: __ testq(rbx, Immediate(kSmiTagMask));
Use CheckSmi as above.

[Søren Thygesen Gjesse, 2009-09-22 13:00:42]

Please review the added andl instruction to the assembler and the
CheckSmiGreaterEqualsConstant in the macro assembler.

http://codereview.chromium.org/209048/diff/1/2
File src/x64/builtins-x64.cc (right):

http://codereview.chromium.org/209048/diff/1/2#newcode480
Line 480: int holes,
On 2009/09/22 11:29:34, Lasse Reichstein wrote:
> Rename "holes" to "initial_capacity".

Done. Changed in IA32 and ARM versions as well.

http://codereview.chromium.org/209048/diff/1/2#newcode494
Line 494: __ AllocateObjectInNewSpace(size,
On 2009/09/22 11:29:34, Lasse Reichstein wrote:
> You are not really allocating an object in NewSpace, but just a patch of
memory
> for two objects.
> Should the allocate function be renamed to AllocateInNewSpace(...)?

Good point. I will rename this in a separate changelist.

http://codereview.chromium.org/209048/diff/1/2#newcode553
Line 553: __ Move(Operand(scratch1, 0), Factory::the_hole_value());
On 2009/09/22 11:29:34, Lasse Reichstein wrote:
> You should use the scratch3 register for the "hole" value here as well. The
way
> it is done will reload the value into kScratchRegister, using a a 64-bit
> immediate, on every round (adding 10+ bytes to the loop).

Done. Artifacts from the IA32 version.

http://codereview.chromium.org/209048/diff/1/2#newcode674
Line 674: __ j(below, &loop);
On 2009/09/22 11:29:34, Lasse Reichstein wrote:
> Why not put the test before the loop, in "while"-style?

This is the way a number of other loops in this file are written, so it's mainly
derived from copy/paste.

http://codereview.chromium.org/209048/diff/1/2#newcode720
Line 720: Condition cond = __ CheckNotPositiveSmi(rdx);
On 2009/09/22 11:29:34, Lasse Reichstein wrote:
> rename "cond" to not_positive_smi. Makes the jump read much better.
> (Or use JumpIfNotPositiveSmi(rdx, call_generic_code), if it exists).

Done.

http://codereview.chromium.org/209048/diff/1/2#newcode726
Line 726: __ cmpq(rdx, Immediate(JSObject::kInitialMaxFastElementArray <<
kSmiTagSize));
On 2009/09/22 11:29:34, Lasse Reichstein wrote:
> Don't create smis like this (if the immediate is really a smi, and not just
> tagged as a smi).
> Use: 
>   __ JumpIfSmiEqualsConstant(rdx, JSObject::kInitialMxFastElementArray,
> call_generic_code)
> 
> 

Added JumpIfSmiGreaterEqualsConstant to macro assembler to cover this.

http://codereview.chromium.org/209048/diff/1/2#newcode826
Line 826: __ testq(rbx, Immediate(kSmiTagMask));
On 2009/09/22 11:29:34, Lasse Reichstein wrote:
> Use 
>  Condition is_smi = CheckSmi(rbx);
> ... as long as NULL is a smi. Check that with ASSERT_EQ(0, kSmiTag);
> 

Done.

http://codereview.chromium.org/209048/diff/1/2#newcode862
Line 862: __ testq(rbx, Immediate(kSmiTagMask));
On 2009/09/22 11:29:34, Lasse Reichstein wrote:
> Use CheckSmi as above.

Done.

[Lasse Reichstein, 2009-09-23 07:41:53]

LGTM w/ comments.

http://codereview.chromium.org/209048/diff/1/2
File src/x64/builtins-x64.cc (right):

http://codereview.chromium.org/209048/diff/1/2#newcode726
Line 726: __ cmpq(rdx, Immediate(JSObject::kInitialMaxFastElementArray <<
kSmiTagSize));
Good.
In the long run there will be a SmiCompare that allows equal/less_than etc. to
be used, but for now this should suffice.

http://codereview.chromium.org/209048/diff/4002/4005
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/209048/diff/4002/4005#newcode525
Line 525: if (Smi::IsValid(constant)) {
Drop the IsValid test, or change it to an ASSERT (you can be greater than a
non-valid smi, so unlike SmiEqualsConstant, it doesn't rule out that the test
should succeede).

http://codereview.chromium.org/209048/diff/4002/4005#newcode526
Line 526: Condition are_greater_equal = CheckSmiEqualsConstant(src, constant);
This doesn't work. SmiEqualsConstant only checks for equality (as far as you can
assume).
Do the test inline (there is currently no smi compare, but there will be
eventually), so:
  cmpl(src, constant << kSmiTagSize);
  j(greater_equal, on_greater_equals);

[Søren Thygesen Gjesse, 2009-09-23 12:40:00]

Please look at the changes to the macro assembler once more.

http://codereview.chromium.org/209048/diff/4002/4005
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/209048/diff/4002/4005#newcode525
Line 525: if (Smi::IsValid(constant)) {
On 2009/09/23 07:41:53, Lasse Reichstein wrote:
> Drop the IsValid test, or change it to an ASSERT (you can be greater than a
> non-valid smi, so unlike SmiEqualsConstant, it doesn't rule out that the test
> should succeede).

Added a jump if non-smi range constant if below minimum smi.

http://codereview.chromium.org/209048/diff/4002/4005#newcode526
Line 526: Condition are_greater_equal = CheckSmiEqualsConstant(src, constant);
On 2009/09/23 07:41:53, Lasse Reichstein wrote:
> This doesn't work. SmiEqualsConstant only checks for equality (as far as you
can
> assume).
> Do the test inline (there is currently no smi compare, but there will be
> eventually), so:
>   cmpl(src, constant << kSmiTagSize);
>   j(greater_equal, on_greater_equals);

That was supposed to use the CheckSmiGreaterEqualsConstant below - which it does
now. Should we keep the CheckSmiGreaterEqualsConstant or just linine the code
here?

[Lasse Reichstein, 2009-09-23 12:45:30]

LGTM

http://codereview.chromium.org/209048/diff/4002/4005
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/209048/diff/4002/4005#newcode526
Line 526: Condition are_greater_equal = CheckSmiEqualsConstant(src, constant);
Keep it.
I'll refactor it anyway, when I merge it with my current work.

http://codereview.chromium.org/209048/diff/4002/4005#newcode618
Line 618: andl(src, Immediate(static_cast<uint32_t>(0x80000000u)));
andl -> testl

[Lasse Reichstein, 2009-09-23 12:46:46]

http://codereview.chromium.org/209048/diff/4002/4005
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/209048/diff/4002/4005#newcode618
Line 618: andl(src, Immediate(static_cast<uint32_t>(0x80000000u)));
Come to think of it, use:
 testl(src, src);
 return not_negative;

[Søren Thygesen Gjesse, 2009-09-23 13:02:13]

http://codereview.chromium.org/209048/diff/4002/4005
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/209048/diff/4002/4005#newcode526
Line 526: Condition are_greater_equal = CheckSmiEqualsConstant(src, constant);
On 2009/09/23 12:45:30, Lasse Reichstein wrote:
> Keep it.
> I'll refactor it anyway, when I merge it with my current work.

Done.

http://codereview.chromium.org/209048/diff/4002/4005#newcode618
Line 618: andl(src, Immediate(static_cast<uint32_t>(0x80000000u)));
On 2009/09/23 12:46:46, Lasse Reichstein wrote:
> Come to think of it, use:
>  testl(src, src);
>  return not_negative;
> 

Done.