[Arguments] Port fast arguments creation stubs to X64 and ARM.

[Arguments] Port fast arguments creation stubs to X64 and ARM.

Also remove an unimpleted code assertion and add printing for arguments objects.

[Lasse Reichstein, 2011-06-07 12:20:19]

Driveby comments on X64 code (style only, haven't checked what it does).

http://codereview.chromium.org/7024047/diff/2007/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/7024047/diff/2007/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:1970: // rbx = parameter count (untagged)
I would move this comment to after the code that ensures the condition holds.

http://codereview.chromium.org/7024047/diff/2007/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:1972: __ SmiToInteger64(rbx, rbx);
There is a SmiToInteger64(Register,Operand) macro that can do this in one
operation.

http://codereview.chromium.org/7024047/diff/2007/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:1989: __ SmiToInteger64(rcx, rcx);
Ditto here.

http://codereview.chromium.org/7024047/diff/2007/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:2013: __ testq(rbx, rbx);
You could get away with using just testl, since rbx is known to be a
sign-extended 32-bit number.
However, that would require a comment that's probably more bother than the extra
byte of code.

http://codereview.chromium.org/7024047/diff/2007/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:2037: __ SmiToInteger64(rbx, rbx);
And another 64-bit load.

http://codereview.chromium.org/7024047/diff/2007/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:2073: __ Integer64PlusConstantToSmi(rcx, rcx, 0);
Just use Integer32ToSmi when the costant is zero. The macro will truncate the
value to a signed 32-bit value after addition anyway.
It probably should check for the constant being zero.
It really is a stupid macro which doesn't provide any good optimizations with
the current smi implementation.

http://codereview.chromium.org/7024047/diff/2007/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:2154: __ testq(rax, rax);
If rax is a smi, use SmiTest(rax); for readability.
It'll do the same thing :)

http://codereview.chromium.org/7024047/diff/2007/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:2205: __ Integer64PlusConstantToSmi(rcx, rcx, 0);
Integer32ToSmi.

http://codereview.chromium.org/7024047/diff/2007/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:2315: __ SmiToInteger64(rcx, rcx);
It seems rcx could be just a 32-bit value here. It's only used as a counter, not
as a part of an address calculation.

[Kevin Millikin (Chromium), 2011-06-07 13:54:10]

I've just looked at the ARM code so far.  I have a few suggestions for cleanup
(the code is hairy, so I could be reading it wrong).

I'll have a closer look and look at the rest when I get a chance.

http://codereview.chromium.org/7024047/diff/2007/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/7024047/diff/2007/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:3974: // r1 = parameter count (untagged)
(untagged) ==> (tagged)

http://codereview.chromium.org/7024047/diff/2007/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4011: __ cmp(r1, Operand(0));
Since r1 is tagged, can we use Smi::FromInt(0)?

http://codereview.chromium.org/7024047/diff/2007/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4036: __ SmiUntag(r1);
Hmm, if I correctly follow this, the untagged r1 is used in a couple of compares
to 0 and then to compute the length of the parameter map, which is tagged before
storing it in the fixed array.

So I think we could avoid untagging it here, compare to Smi::FromInt(0), and
compute the tagged length directly.

http://codereview.chromium.org/7024047/diff/2007/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4043: // r2 = argument count (untagged)
(untagged) ==> (tagged)

http://codereview.chromium.org/7024047/diff/2007/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4074: __ mov(r6, r0);
This mov might be unnecessary.  Can r6 be used instead of r0 below, preserving
the object in r0?

http://codereview.chromium.org/7024047/diff/2007/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4105: __ mov(r3, r4);
Can we avoid this mov by using r3 instead of r4 below:

__ add(r3, r4, Operand(r5, LSL, 2));
__ add(r3, r3, Operand(kParameterMapHeaderSize));
...

http://codereview.chromium.org/7024047/diff/2007/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4107: __ SmiUntag(r5);
Is this untagged and retagged at its use?  Maybe my ARM code is rusty, but I
thought:

// No mov(r5, r0) or SmiUntag(r5).
__ add(r4, r4, Operand(r0, LSL, 1));
__ add(r4, r4, Operand(kParameterMapHeaderSize));

http://codereview.chromium.org/7024047/diff/2007/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4121: __ mov(r5, Operand(r0, LSL, 1));
The computation of this value in r5 is duplicated just below.  Can we avoid it
with:

__ mov(r5, Operand(r0, LSL, 1));
__ add(r5, r5, Operand(kParameterMapHeaderSize - kHeapObjectTag));
__ str(r1, MemOperand(r3, r5));
__ sub(r5, r5, Operand(kParameterMapHeaderSize - FixedArray::kHeaderSize));
__ str(r7, MemOperand(r4, r5));

[Karl Klose, 2011-06-10 11:32:22]

http://codereview.chromium.org/7024047/diff/2007/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/7024047/diff/2007/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:3974: // r1 = parameter count (untagged)
On 2011/06/07 13:54:10, Kevin Millikin wrote:
> (untagged) ==> (tagged)

Done.

http://codereview.chromium.org/7024047/diff/2007/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4011: __ cmp(r1, Operand(0));
On 2011/06/07 13:54:10, Kevin Millikin wrote:
> Since r1 is tagged, can we use Smi::FromInt(0)?

Done.

http://codereview.chromium.org/7024047/diff/2007/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4036: __ SmiUntag(r1);
On 2011/06/07 13:54:10, Kevin Millikin wrote:
> Hmm, if I correctly follow this, the untagged r1 is used in a couple of
compares
> to 0 and then to compute the length of the parameter map, which is tagged
before
> storing it in the fixed array.
> 
> So I think we could avoid untagging it here, compare to Smi::FromInt(0), and
> compute the tagged length directly.

Done.

http://codereview.chromium.org/7024047/diff/2007/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4043: // r2 = argument count (untagged)
On 2011/06/07 13:54:10, Kevin Millikin wrote:
> (untagged) ==> (tagged)

Done.

http://codereview.chromium.org/7024047/diff/2007/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4105: __ mov(r3, r4);
Done. The tricky part is that this code can be jumped over, so I had to
introduce a conditional move at line 4079.

http://codereview.chromium.org/7024047/diff/2007/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4107: __ SmiUntag(r5);
You are right, this is unnecessary.

http://codereview.chromium.org/7024047/diff/2007/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4121: __ mov(r5, Operand(r0, LSL, 1));
Done. It makes the code slightly harder to understand, but it should be ok here.

http://codereview.chromium.org/7024047/diff/2007/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/7024047/diff/2007/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:1970: // rbx = parameter count (untagged)
On 2011/06/07 12:20:19, Lasse Reichstein wrote:
> I would move this comment to after the code that ensures the condition holds.

Done.

http://codereview.chromium.org/7024047/diff/2007/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:1972: __ SmiToInteger64(rbx, rbx);
On 2011/06/07 12:20:19, Lasse Reichstein wrote:
> There is a SmiToInteger64(Register,Operand) macro that can do this in one
> operation.

Done.

http://codereview.chromium.org/7024047/diff/2007/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:2013: __ testq(rbx, rbx);
I will keep the current code.

http://codereview.chromium.org/7024047/diff/2007/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:2037: __ SmiToInteger64(rbx, rbx);
On 2011/06/07 12:20:19, Lasse Reichstein wrote:
> And another 64-bit load.

Done.

http://codereview.chromium.org/7024047/diff/2007/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:2073: __ Integer64PlusConstantToSmi(rcx, rcx, 0);
On 2011/06/07 12:20:19, Lasse Reichstein wrote:
> Just use Integer32ToSmi when the costant is zero. The macro will truncate the
> value to a signed 32-bit value after addition anyway.
> It probably should check for the constant being zero.
> It really is a stupid macro which doesn't provide any good optimizations with
> the current smi implementation.

Done.

http://codereview.chromium.org/7024047/diff/2007/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:2154: __ testq(rax, rax);
On 2011/06/07 12:20:19, Lasse Reichstein wrote:
> If rax is a smi, use SmiTest(rax); for readability.
> It'll do the same thing :)

Done.

http://codereview.chromium.org/7024047/diff/2007/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:2205: __ Integer64PlusConstantToSmi(rcx, rcx, 0);
On 2011/06/07 12:20:19, Lasse Reichstein wrote:
> Integer32ToSmi.

Done.

[Kevin Millikin (Chromium), 2011-06-10 11:51:52]

LGTM, with some small comments below.

http://codereview.chromium.org/7024047/diff/8001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/7024047/diff/8001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:3975: // r1 = parameter count (untagged)
Still says untagged in the change set.

http://codereview.chromium.org/7024047/diff/8001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4004: __ mov(r9, r1);
Not a big deal, but you could save the mov(r1, r9) below by using (and
destroying) r9 instead of r1.

http://codereview.chromium.org/7024047/diff/8001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4036: __ cmp(r1, Operand(0));
Operand(Smi::FromInt(0))?

http://codereview.chromium.org/7024047/diff/8001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/7024047/diff/8001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:2006: __ push(rbx);
I don't know what the register story on x64 is (Bill or Lasse would know).  I
think there are available registers to avoid this push.

http://codereview.chromium.org/7024047/diff/8001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:2091: __ push(rax);
May be able to avoid this push.

http://codereview.chromium.org/7024047/diff/8001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:2116: __ push(rcx);
Also here.

[Lasse Reichstein, 2011-06-10 12:15:22]

http://codereview.chromium.org/7024047/diff/8001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/7024047/diff/8001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:2006: __ push(rbx);
Unless the stub uses them for something else (and I doubt it does), there should
be plenty of registers left to use, at least r8, r9, r11, r14 and r15.
(r10 is kScratchRegister, r12 holds Smi 1, and r13 is the root register).

[Karl Klose, 2011-06-14 12:56:28]

http://codereview.chromium.org/7024047/diff/8001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/7024047/diff/8001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:3975: // r1 = parameter count (untagged)
Should be done now.

http://codereview.chromium.org/7024047/diff/8001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4004: __ mov(r9, r1);
On 2011/06/10 11:51:52, Kevin Millikin wrote:
> Not a big deal, but you could save the mov(r1, r9) below by using (and
> destroying) r9 instead of r1.

Done.

http://codereview.chromium.org/7024047/diff/8001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/7024047/diff/8001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:2006: __ push(rbx);
I have removed the push instructions, the code is now more similar to the ARM
version.

http://codereview.chromium.org/7024047/diff/8001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:2091: __ push(rax);
On 2011/06/10 11:51:52, Kevin Millikin wrote:
> May be able to avoid this push.

Done.

http://codereview.chromium.org/7024047/diff/8001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:2116: __ push(rcx);
On 2011/06/10 11:51:52, Kevin Millikin wrote:
> Also here.

Done.

[Alexandre, 2011-06-15 13:27:56]

A few small comments.

I haven't tested the patch yet. How does it behave on ARM?

Regards,

Alexandre

http://codereview.chromium.org/7024047/diff/8001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/7024047/diff/8001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:3956: __ add(r3, r3, Operand(r2, LSL, 1));
Here and below, '1' standing for 'kPointerSizeLog2 - kSmiTagSize' in shift
operations. Should we use the verbose description?

http://codereview.chromium.org/7024047/diff/8001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4017: __ add(r1, r1,
Operand(FixedArray::kHeaderSize));
This add and the following one could be merged into only one instruction.

http://codereview.chromium.org/7024047/diff/8001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4114: __ jmp(&parameters_test);
I don't know how performance sensitive this loop is, but I think it could be
optimized to something like this:


Label loop, end;

cmp r6, 0
beq end

// Setup.
// Use r6 as the counter, already shifted to kPointerSize.
mov r6, r6 LSL 1
// Compute base addresses of stores. (extracting loop invariant code)
add r5, r4, kParameterMapHeaderSize - kHeapObjectTag
add r3, r3, FixedArray::kHeaderSize - kHeapObjectTag

bind(loop);
subs r6, r6, kPointerSize   // Decrement the counter and set the flags.
// Stores.
str r1, [r5, r6]
add r1, r1, Smi::FromInt(1)
str r7, [r3, r6]
bgt, loop

// Restore address of backing store.
sub r3, r3, FixedArray::kHeaderSize - kHeapObjectTag

bind(end);

http://codereview.chromium.org/7024047/diff/8001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4138: __ mov(r1, r9);
If necessary I think this loop could also be refactored.

- If r4 is not used outside the loop the test to enter the loop can be moved at
the very beginning.
- Using r1 as a counter
    - aligned on kPointerSize (with a 'sub' setting flags)
    - 'r1 LSL 1' shift can be moved to the initial 'mov(r1, r9)'

http://codereview.chromium.org/7024047/diff/8001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4144: __ sub(r4, r4, Operand(kPointerSize));
The sub can be merged with the ldr using NegPreIndex.

http://codereview.chromium.org/7024047/diff/8001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4239: __ mov(r1, Operand(r1, LSR, kSmiTagSize));
If r1 is only used as a counter and not used later there is no need to untag it.
It can be decremented by Smi::FromInt(1) instead.

http://codereview.chromium.org/7024047/diff/8001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4252: __ cmp(r1, Operand(0, RelocInfo::NONE));
Could merge the 'sub' and 'cmp' instructions into a 'subs'.