v8: Replace 2 ARM ldr instructions with one ldrd in the code generated for a SubS...

Replace 2 ARM ldr instructions with one ldrd in the code generated for a SubStringStub and StringCompareStub in the ARM backend. Also changed a comment (line 4154) that seem to have been wrong before."

[Andreas Anyuru, 2010-09-03 12:17:40]

Erik, Rodolph,

I have spent quite some time reading the v8 source code to understand the
existing architecture.

This is an attempt to contribute a very small change to the ARM backend.

Is it possible for you to reivew it?

http://codereview.chromium.org/3341012/show

Regards,
Andreas Anyuru

[Rodolph Perfetta, 2010-09-03 13:10:47]

http://codereview.chromium.org/3341012/diff/1/3
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/3341012/diff/1/3#newcode4146
src/arm/code-stubs-arm.cc:4146: __ ldrd(r6, r7, MemOperand(sp, kToOffset));
ldrd is a v5te instruction, V8 supports v4 so you need to provide code for older
architectures. The simplest way to do so is to use the Ldrd macro instruction
(macro-assembler-arm.*) which handle all this transparently.

ldrd loads the lowest address in the lowest register so in effect your code has
swapped r6 and r7, you need to update the code after that.

[Andreas Anyuru, 2010-09-04 22:51:40]

Hi Rodolph,
Thanks for your very good and valid comments! (Actually I should have caught
these myself.)
Hopefully it looks better now. In addition to fixing your comments I also
changed one comment in the same file (code-stubs-arm.cc line 4154) that seems to
have been wrong before. I also replaced 2 __ ldr with one __ Ldrd in
StringCompareStub::Generate.

On 2010/09/03 13:10:47, Rodolph Perfetta wrote:
> http://codereview.chromium.org/3341012/diff/1/3
> File src/arm/code-stubs-arm.cc (right):
> 
> http://codereview.chromium.org/3341012/diff/1/3#newcode4146
> src/arm/code-stubs-arm.cc:4146: __ ldrd(r6, r7, MemOperand(sp, kToOffset));
> ldrd is a v5te instruction, V8 supports v4 so you need to provide code for
older
> architectures. The simplest way to do so is to use the Ldrd macro instruction
> (macro-assembler-arm.*) which handle all this transparently.
> 
Thanks! I have fixed this.
> ldrd loads the lowest address in the lowest register so in effect your code
has
> swapped r6 and r7, you need to update the code after that.
You are right. Fixed!

Best Regards,
Andreas

[Andreas Anyuru, 2010-09-04 22:57:50]

New snapshot uploaded. Fixed comments from Rodolph.

http://codereview.chromium.org/3341012/show

Best Regards,
Andreas

[Andreas Anyuru, 2010-09-07 17:49:59]

New snapshot uploaded,

Fixed 2 lint errors, that I missed before.

[Rodolph Perfetta, 2010-09-07 22:53:29]

LGTM

[Søren Thygesen Gjesse, 2010-09-08 07:14:03]

Drive by comment

http://codereview.chromium.org/3341012/diff/9001/10002
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/3341012/diff/9001/10002#newcode4146
src/arm/code-stubs-arm.cc:4146: // r6 = to (smi), r7 = from (smi)
Just a suggestion, but as r6 and r7 hold the same values throughout this
function using variables for the to and from registers

  Register to = r6
  Register from = r7

will make things easier to read, and you can avoid having to hassle with keeping
comments in sync with the code.

[Andreas Anyuru, 2010-09-08 08:51:37]

New snapshot uploaded (Patch Set 4), to fix comment from Søren Gjesse. Thanks
for the comment! 
I hope it still looks ok for you Rodolph.

Regards,
Andreas Anyuru   	

On 2010/09/08 07:14:03, Søren Gjesse wrote:
> Drive by comment
> 
> http://codereview.chromium.org/3341012/diff/9001/10002
> File src/arm/code-stubs-arm.cc (right):
> 
> http://codereview.chromium.org/3341012/diff/9001/10002#newcode4146
> src/arm/code-stubs-arm.cc:4146: // r6 = to (smi), r7 = from (smi)
> Just a suggestion, but as r6 and r7 hold the same values throughout this
> function using variables for the to and from registers
> 
>   Register to = r6
>   Register from = r7
> 
> will make things easier to read, and you can avoid having to hassle with
keeping
> comments in sync with the code.
I agree. Done.

[Erik Corry, 2010-09-08 11:02:53]

http://codereview.chromium.org/3341012/diff/14001/15002
File src/arm/code-stubs-arm.cc (left):

http://codereview.chromium.org/3341012/diff/14001/15002#oldcode4142
src/arm/code-stubs-arm.cc:4142: static const int kFromOffset = 1 * kPointerSize;
Please leave this constant there because it serves to document the layout on the
stack.

http://codereview.chromium.org/3341012/diff/14001/15002
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/3341012/diff/14001/15002#newcode4149
src/arm/code-stubs-arm.cc:4149: __ Ldrd(to, from, MemOperand(sp, kToOffset));
Here you should statically assert that the FromOffset is the ToOffset + 4.  That
way if someone tries to rearrange things they will get an error.

http://codereview.chromium.org/3341012/diff/14001/15002#newcode4234
src/arm/code-stubs-arm.cc:4234: masm, r3, r4, r1, r5, to, from, r9,
&make_two_character_string);
To and from are just being used as scratch registers here.  Therefore it is
misleading to use their names.  I would rather just use r6 and r7 on this line.

http://codereview.chromium.org/3341012/diff/14001/15002#newcode4267
src/arm/code-stubs-arm.cc:4267: StringHelper::GenerateCopyCharactersLong(masm,
r1, r5, r2, r3, r4, to, from,
Likewise here.

http://codereview.chromium.org/3341012/diff/14001/15002#newcode4268
src/arm/code-stubs-arm.cc:4268: r9, COPY_ASCII | DEST_ALWAYS_ALIGNED);
Indentation is wrong here now.

http://codereview.chromium.org/3341012/diff/14001/15002#newcode4276
src/arm/code-stubs-arm.cc:4276: // Check for flat two byte string.
Since the value in the 'from' register is used lower down it is still live.  It
is problematic to remove the comment here and similar ones above since they
documented that fact.

http://codereview.chromium.org/3341012/diff/14001/15002#newcode4297
src/arm/code-stubs-arm.cc:4297: StringHelper::GenerateCopyCharactersLong(masm,
r1, r5, r2, r3, r4, to, from,
Again, these are just scratch registers.  The names 'from' and 'to' are
misleading.

http://codereview.chromium.org/3341012/diff/14001/15002#newcode4374
src/arm/code-stubs-arm.cc:4374: __ Ldrd(r0 , r1, MemOperand(sp));  // Load right
in r0, left in r1
Full stop/period at the end of comments.

[Andreas Anyuru, 2010-09-08 19:14:06]

Eric,
Uploaded new snapshot after I fixed your comments.
Rodolph and Søren, I hope you are still satisfied after these new (minor)
changes.

http://codereview.chromium.org/3341012/diff/9001/10002
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/3341012/diff/9001/10002#newcode4146
src/arm/code-stubs-arm.cc:4146: // r6 = to (smi), r7 = from (smi)
On 2010/09/08 07:14:03, Søren Gjesse wrote:
> Just a suggestion, but as r6 and r7 hold the same values throughout this
> function using variables for the to and from registers
> 
>   Register to = r6
>   Register from = r7
> 
> will make things easier to read, and you can avoid having to hassle with
keeping
> comments in sync with the code.

Done.

http://codereview.chromium.org/3341012/diff/14001/15002
File src/arm/code-stubs-arm.cc (left):

http://codereview.chromium.org/3341012/diff/14001/15002#oldcode4142
src/arm/code-stubs-arm.cc:4142: static const int kFromOffset = 1 * kPointerSize;
On 2010/09/08 11:02:54, Erik Corry wrote:
> Please leave this constant there because it serves to document the layout on
the
> stack.

Done.

http://codereview.chromium.org/3341012/diff/14001/15002
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/3341012/diff/14001/15002#newcode4149
src/arm/code-stubs-arm.cc:4149: __ Ldrd(to, from, MemOperand(sp, kToOffset));
On 2010/09/08 11:02:54, Erik Corry wrote:
> Here you should statically assert that the FromOffset is the ToOffset + 4. 
That
> way if someone tries to rearrange things they will get an error.

Done.

http://codereview.chromium.org/3341012/diff/14001/15002#newcode4234
src/arm/code-stubs-arm.cc:4234: masm, r3, r4, r1, r5, to, from, r9,
&make_two_character_string);
On 2010/09/08 11:02:54, Erik Corry wrote:
> To and from are just being used as scratch registers here.  Therefore it is
> misleading to use their names.  I would rather just use r6 and r7 on this
line.


Done.
(I had the same thought from the beginning, but then I thought it was better to
be consistent and not mix r6, r7, 'to' and 'from' in this function. That way it
would be more obvious that it is actually 'to' and 'from' that was used as
scratch registers. The reader of the code would only need to keep track of
either r6 and r7 or 'to' and 'from', not both. I thought that you anyway need to
look at the prototype for GenerateTwoCharacterSymbolTableProbe() to know which
registers are scratch registers. Anyway I changed according your comment now,
since I also see your point.)

http://codereview.chromium.org/3341012/diff/14001/15002#newcode4267
src/arm/code-stubs-arm.cc:4267: StringHelper::GenerateCopyCharactersLong(masm,
r1, r5, r2, r3, r4, to, from,
On 2010/09/08 11:02:54, Erik Corry wrote:
> Likewise here.

Done.

http://codereview.chromium.org/3341012/diff/14001/15002#newcode4268
src/arm/code-stubs-arm.cc:4268: r9, COPY_ASCII | DEST_ALWAYS_ALIGNED);
On 2010/09/08 11:02:54, Erik Corry wrote:
> Indentation is wrong here now.

Done.

http://codereview.chromium.org/3341012/diff/14001/15002#newcode4276
src/arm/code-stubs-arm.cc:4276: // Check for flat two byte string.
On 2010/09/08 11:02:54, Erik Corry wrote:
> Since the value in the 'from' register is used lower down it is still live. 
It
> is problematic to remove the comment here and similar ones above since they
> documented that fact.

Done.
I added this comment again and the ones above where r6 or r7 where used further
down. Please tell me if you wanted me to keep all the comments for the
registers. I did not interpret your comment that way. So I only put them in when
r6 or r7 where used in the code below the comment. Actually I interpreted
Søren's comment as if he wanted me to remove the comments when I replaced r6
with 'to' and r7 with 'from'. Anyway I see your point and hope I have changed
according your comment.

http://codereview.chromium.org/3341012/diff/14001/15002#newcode4297
src/arm/code-stubs-arm.cc:4297: StringHelper::GenerateCopyCharactersLong(masm,
r1, r5, r2, r3, r4, to, from,
On 2010/09/08 11:02:54, Erik Corry wrote:
> Again, these are just scratch registers.  The names 'from' and 'to' are
> misleading.

Done.

http://codereview.chromium.org/3341012/diff/14001/15002#newcode4374
src/arm/code-stubs-arm.cc:4374: __ Ldrd(r0 , r1, MemOperand(sp));  // Load right
in r0, left in r1
On 2010/09/08 11:02:54, Erik Corry wrote:
> Full stop/period at the end of comments.

Done.

[Erik Corry, 2010-09-15 10:23:52]

Committed as bleeding edge revision 5457.  Thankyou.

http://codereview.chromium.org/3341012/diff/2002/18002
File src/arm/code-stubs-arm.cc (left):

http://codereview.chromium.org/3341012/diff/2002/18002#oldcode4169
src/arm/code-stubs-arm.cc:4169: // r7: to (smi)
I put these comments back.

http://codereview.chromium.org/3341012/diff/2002/18002#oldcode4183
src/arm/code-stubs-arm.cc:4183: // r6: from (smi)
And these

http://codereview.chromium.org/3341012/diff/2002/18002#oldcode4211
src/arm/code-stubs-arm.cc:4211: // r7: to (smi)
And I put 'to' back here.

http://codereview.chromium.org/3341012/diff/2002/18002#oldcode4220
src/arm/code-stubs-arm.cc:4220: // r6: from offset (smi)
We use 'from' below so it is still live here, so I put this back.

http://codereview.chromium.org/3341012/diff/2002/18002
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/3341012/diff/2002/18002#newcode4147
src/arm/code-stubs-arm.cc:4147: // Both to and from are smis.
We don't know that yet, so I moved this comment down.