Changing string length field type from int to SMI. It will make it be a regu...

Changing string length field  type from int to SMI. It will make it be a regular field. Code generated in EmitNamedLoad could be patched for faster access to string.length.


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

[Kevin Millikin (Chromium), 2010-04-27 11:25:55]

Drive by.

http://codereview.chromium.org/1706013/diff/50001/51012
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/1706013/diff/50001/51012#newcode8119
src/arm/codegen-arm.cc:8119: // r3: Length of subject string as a SMI
"SMI" ==> "smi".  It is not an acronym.

http://codereview.chromium.org/1706013/diff/50001/51012#newcode9032
src/arm/codegen-arm.cc:9032: // Untag SMI.
smi.

http://codereview.chromium.org/1706013/diff/50001/51010
File src/objects-inl.h (right):

http://codereview.chromium.org/1706013/diff/50001/51010#newcode1654
src/objects-inl.h:1654: int String::length() {
Why not

SMI_ACCESSORS(String, length, kLengthOffset)

[SeRya, 2010-04-27 11:41:00]

http://codereview.chromium.org/1706013/diff/50001/51012
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/1706013/diff/50001/51012#newcode8119
src/arm/codegen-arm.cc:8119: // r3: Length of subject string as a SMI
On 2010/04/27 11:25:55, Kevin Millikin wrote:
> "SMI" ==> "smi".  It is not an acronym.

Done.

http://codereview.chromium.org/1706013/diff/50001/51012#newcode9032
src/arm/codegen-arm.cc:9032: // Untag SMI.
On 2010/04/27 11:25:55, Kevin Millikin wrote:
> smi.

Done.

http://codereview.chromium.org/1706013/diff/50001/51010
File src/objects-inl.h (right):

http://codereview.chromium.org/1706013/diff/50001/51010#newcode1654
src/objects-inl.h:1654: int String::length() {
On 2010/04/27 11:25:55, Kevin Millikin wrote:
> Why not
> 
> SMI_ACCESSORS(String, length, kLengthOffset)

Done.

[Vyacheslav Egorov (Chromium), 2010-04-27 12:27:24]

First round of comments. 

I will do a second pass over sources later [probably tomorrow].

http://codereview.chromium.org/1706013/diff/50001/51013
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/1706013/diff/50001/51013#newcode1025
src/arm/macro-assembler-arm.cc:1025: str(scratch2, FieldMemOperand(result,
String::kLengthOffset));
Is there a reason behind interleaving "defs" and "uses" of registers? Something
like

LoadRoot(scratch1, Heap::kStringMapRootIndex);
mov(scratch2, Operand(length, LSL, kSmiTagSize));
str(scratch2, FieldMemOperand(result, String::kLengthOffset)); 
LoadRoot(scratch1, Heap::kStringMapRootIndex);

would be simpler to read. 

I am a bit unsure how this will affect pipeline though.

http://codereview.chromium.org/1706013/diff/50001/51013#newcode1061
src/arm/macro-assembler-arm.cc:1061: str(scratch2, FieldMemOperand(result,
String::kLengthOffset));
Ditto. 

Also this piece of code is suspicious. We are loading root ascii string map
twice into the same register.

http://codereview.chromium.org/1706013/diff/50001/51004
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/1706013/diff/50001/51004#newcode10786
src/ia32/codegen-ia32.cc:10786: __ j(below, &runtime);
negation of above is below_equal, not below. 

But having below_equal here would contradict comment. Very interesting. Was
there a bug here?

http://codereview.chromium.org/1706013/diff/50001/51004#newcode10917
src/ia32/codegen-ia32.cc:10917: __ lea(ecx, FieldOperand(eax, edi, times_1,
SeqTwoByteString::kHeaderSize));
Please leave a comment here about edi being tagged smi. Otherwise time_1 vs.
times_2 change is not obvious.

Also please assert kSmiTag == 0, kSmiTagSize == 1

http://codereview.chromium.org/1706013/diff/50001/51004#newcode12105
src/ia32/codegen-ia32.cc:12105: index, times_1,
Ditto

http://codereview.chromium.org/1706013/diff/50001/51010
File src/objects-inl.h (right):

http://codereview.chromium.org/1706013/diff/50001/51010#newcode1661
src/objects-inl.h:1661: }
There are SMI_ACCESSORS macro for this.

Also it might be reasonable to add field verification to StringVerify() method
to check that fields is still a smi before and after garbage collection.

http://codereview.chromium.org/1706013/diff/50001/51009
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/1706013/diff/50001/51009#newcode7604
src/x64/codegen-x64.cc:7604: __ SmiToInteger32(rdi, rdi);
Move this up to avoid duplication on both codepaths?

http://codereview.chromium.org/1706013/diff/50001/51009#newcode9748
src/x64/codegen-x64.cc:9748: __ SmiToInteger32(rbx, rbx);
Is it necessary to untag it here? 

It seems possible to move this after label string_add_flat_result and avoid
tagging it back later in ConsString codepath.

[SeRya, 2010-04-27 16:16:30]

http://codereview.chromium.org/1706013/diff/50001/51013
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/1706013/diff/50001/51013#newcode1025
src/arm/macro-assembler-arm.cc:1025: str(scratch2, FieldMemOperand(result,
String::kLengthOffset));
On 2010/04/27 12:27:24, Vyacheslav Egorov wrote:
> Is there a reason behind interleaving "defs" and "uses" of registers?
Something
> like
> 
> LoadRoot(scratch1, Heap::kStringMapRootIndex);
> mov(scratch2, Operand(length, LSL, kSmiTagSize));
> str(scratch2, FieldMemOperand(result, String::kLengthOffset)); 
> LoadRoot(scratch1, Heap::kStringMapRootIndex);
> 
> would be simpler to read. 
> 
> I am a bit unsure how this will affect pipeline though. 

Actually I lent the approach with interleaving from AllocateAsciiString. But
since sequential loading/storing was acceptable here it must be acceptable now
(here and in AllocateAsciiString too).

http://codereview.chromium.org/1706013/diff/50001/51013#newcode1061
src/arm/macro-assembler-arm.cc:1061: str(scratch2, FieldMemOperand(result,
String::kLengthOffset));
On 2010/04/27 12:27:24, Vyacheslav Egorov wrote:
> Ditto. 
> 
> Also this piece of code is suspicious. We are loading root ascii string map
> twice into the same register. 

Redundant load instruction removed.

http://codereview.chromium.org/1706013/diff/50001/51004
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/1706013/diff/50001/51004#newcode10786
src/ia32/codegen-ia32.cc:10786: __ j(below, &runtime);
On 2010/04/27 12:27:24, Vyacheslav Egorov wrote:
> negation of above is below_equal, not below. 
> 
> But having below_equal here would contradict comment. Very interesting. Was
> there a bug here? 
> 
> 

Actually (a < b) is equivalent to (b > a) but not to (b >= a).

I suppose this value is checked again in the regexp code so difference is not
observable. Anyway, going into the runtime when they are equal looks more
logical.

http://codereview.chromium.org/1706013/diff/50001/51004#newcode10917
src/ia32/codegen-ia32.cc:10917: __ lea(ecx, FieldOperand(eax, edi, times_1,
SeqTwoByteString::kHeaderSize));
On 2010/04/27 12:27:24, Vyacheslav Egorov wrote:
> Please leave a comment here about edi being tagged smi. Otherwise time_1 vs.
> times_2 change is not obvious.
> 
> Also please assert kSmiTag == 0, kSmiTagSize == 1

Done.

http://codereview.chromium.org/1706013/diff/50001/51004#newcode12105
src/ia32/codegen-ia32.cc:12105: index, times_1,
On 2010/04/27 12:27:24, Vyacheslav Egorov wrote:
> Ditto

Done.

http://codereview.chromium.org/1706013/diff/50001/51009
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/1706013/diff/50001/51009#newcode7604
src/x64/codegen-x64.cc:7604: __ SmiToInteger32(rdi, rdi);
On 2010/04/27 12:27:24, Vyacheslav Egorov wrote:
> Move this up to avoid duplication on both codepaths?
> 

It couldn't be placed after 'movq' since the next conditional jump relies on the
flags register state set by 'testb'.

http://codereview.chromium.org/1706013/diff/50001/51009#newcode9748
src/x64/codegen-x64.cc:9748: __ SmiToInteger32(rbx, rbx);
On 2010/04/27 12:27:24, Vyacheslav Egorov wrote:
> Is it necessary to untag it here? 
> 
> It seems possible to move this after label string_add_flat_result and avoid
> tagging it back later in ConsString codepath.

Done.

[Erik Corry, 2010-04-28 13:25:26]

http://codereview.chromium.org/1706013/diff/65002/41014
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/1706013/diff/65002/41014#newcode8119
src/arm/codegen-arm.cc:8119: // r3: Length of subject string as a smi
Missing full stop after comment on these lines.

http://codereview.chromium.org/1706013/diff/65002/41014#newcode8122
src/arm/codegen-arm.cc:8122: // Check that the third argument is a positive smi
less than the subject
we don't actually check that the third argument is a Smi.  We just check whether
it is in range or not, then shift out the tag bit.  Not sure whether that is OK,
but the comment should reflect it.

[Vyacheslav Egorov (Chromium), 2010-04-28 13:33:41]

Did a second pass over sources.

http://codereview.chromium.org/1706013/diff/50001/51004
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/1706013/diff/50001/51004#newcode10786
src/ia32/codegen-ia32.cc:10786: __ j(below, &runtime);
On 2010/04/27 16:16:30, SeRya wrote:
> On 2010/04/27 12:27:24, Vyacheslav Egorov wrote:
> > negation of above is below_equal, not below. 
> > 
> > But having below_equal here would contradict comment. Very interesting. Was
> > there a bug here? 
> > 
> > 
> 
> Actually (a < b) is equivalent to (b > a) but not to (b >= a).
> 
> I suppose this value is checked again in the regexp code so difference is not
> observable. Anyway, going into the runtime when they are equal looks more
> logical.

Yeah, my fault again. For some reason I thought that you _negated_ condition.

http://codereview.chromium.org/1706013/diff/50001/51009
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/1706013/diff/50001/51009#newcode7604
src/x64/codegen-x64.cc:7604: __ SmiToInteger32(rdi, rdi);
On 2010/04/27 16:16:30, SeRya wrote:
> On 2010/04/27 12:27:24, Vyacheslav Egorov wrote:
> > Move this up to avoid duplication on both codepaths?
> > 
> 
> It couldn't be placed after 'movq' since the next conditional jump relies on
the
> flags register state set by 'testb'.

Yeah, you are right.

http://codereview.chromium.org/1706013/diff/65002/41014
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/1706013/diff/65002/41014#newcode9029
src/arm/codegen-arm.cc:9029: __ tst(min_length, Operand(min_length));
Testing smi for zero.

ASSERT kSmiTag == 0?

http://codereview.chromium.org/1706013/diff/65002/41014#newcode9144
src/arm/codegen-arm.cc:9144: __ cmp(r2, Operand(0));  // Test if first string is
empty.
Comparing smi with non-smi.

Assert kSmiTag == 0 or use Smi::FromInt() ?

Another interesting question [yes I am aware that you are not the author of the
code] what is faster on ARM tst(x,x) or cmp(x, 0)?

http://codereview.chromium.org/1706013/diff/65002/41014#newcode9146
src/arm/codegen-arm.cc:9146: __ cmp(r3, Operand(0), ne);  // Else test if second
string is empty.
Ditto.

http://codereview.chromium.org/1706013/diff/65002/41015
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/1706013/diff/65002/41015#newcode1060
src/arm/macro-assembler-arm.cc:1060: str(scratch2, FieldMemOperand(result,
String::kLengthOffset));
I talked to Erik and he says that it is reasonable to interleave load with other
operation to utilize pipeline better. 

Sorry for the confusion.

http://codereview.chromium.org/1706013/diff/65002/41006
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/1706013/diff/65002/41006#newcode8824
src/ia32/codegen-ia32.cc:8824: __ test(edx, Operand(edx));
Well as far as I can see edx is not used after this point, only compared with 0.
So remove SmiUntag and assert kSmiTag == 0.

http://codereview.chromium.org/1706013/diff/65002/41006#newcode10917
src/ia32/codegen-ia32.cc:10917: ASSERT(kSmiTag == 0 && kSmiTagSize);  // edi is
smi (powered by 2).
lost: == 1 in assertion.

http://codereview.chromium.org/1706013/diff/65002/41011
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/1706013/diff/65002/41011#newcode9747
src/x64/codegen-x64.cc:9747: 
empty line?

http://codereview.chromium.org/1706013/diff/65002/41011#newcode10318
src/x64/codegen-x64.cc:10318: __ testq(min_length, min_length);
ASSERT kSmiTag == 0 ?

http://codereview.chromium.org/1706013/diff/65002/41011#newcode10353
src/x64/codegen-x64.cc:10353: __ testq(length_difference, length_difference);
ASSERT kSmiTag == 0 ?

[SeRya, 2010-04-28 16:40:31]

http://codereview.chromium.org/1706013/diff/65002/41014
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/1706013/diff/65002/41014#newcode8119
src/arm/codegen-arm.cc:8119: // r3: Length of subject string as a smi
On 2010/04/28 13:25:27, Erik Corry wrote:
> Missing full stop after comment on these lines.

There are a lot of such comments around. May be to fix them separately?

http://codereview.chromium.org/1706013/diff/65002/41014#newcode8122
src/arm/codegen-arm.cc:8122: // Check that the third argument is a positive smi
less than the subject
On 2010/04/28 13:25:27, Erik Corry wrote:
> we don't actually check that the third argument is a Smi.  We just check
whether
> it is in range or not, then shift out the tag bit.  Not sure whether that is
OK,
> but the comment should reflect it.

Runtime_RegExpExec checks for smi and throws an exception if it is not. So I
added a check here.

http://codereview.chromium.org/1706013/diff/65002/41014#newcode9029
src/arm/codegen-arm.cc:9029: __ tst(min_length, Operand(min_length));
On 2010/04/28 13:33:41, Vyacheslav Egorov wrote:
> Testing smi for zero.
> 
> ASSERT kSmiTag == 0?

Done.

http://codereview.chromium.org/1706013/diff/65002/41014#newcode9144
src/arm/codegen-arm.cc:9144: __ cmp(r2, Operand(0));  // Test if first string is
empty.
On 2010/04/28 13:33:41, Vyacheslav Egorov wrote:
> Comparing smi with non-smi.
> 
> Assert kSmiTag == 0 or use Smi::FromInt() ?
> 
> Another interesting question [yes I am aware that you are not the author of
the
> code] what is faster on ARM tst(x,x) or cmp(x, 0)?

Done.

http://codereview.chromium.org/1706013/diff/65002/41014#newcode9146
src/arm/codegen-arm.cc:9146: __ cmp(r3, Operand(0), ne);  // Else test if second
string is empty.
On 2010/04/28 13:33:41, Vyacheslav Egorov wrote:
> Ditto.

Done.

http://codereview.chromium.org/1706013/diff/65002/41015
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/1706013/diff/65002/41015#newcode1060
src/arm/macro-assembler-arm.cc:1060: str(scratch2, FieldMemOperand(result,
String::kLengthOffset));
On 2010/04/28 13:33:41, Vyacheslav Egorov wrote:
> I talked to Erik and he says that it is reasonable to interleave load with
other
> operation to utilize pipeline better. 
> 
> Sorry for the confusion.  
>  

Done.

http://codereview.chromium.org/1706013/diff/65002/41006
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/1706013/diff/65002/41006#newcode8824
src/ia32/codegen-ia32.cc:8824: __ test(edx, Operand(edx));
On 2010/04/28 13:33:41, Vyacheslav Egorov wrote:
> Well as far as I can see edx is not used after this point, only compared with
0.
> So remove SmiUntag and assert kSmiTag == 0.
> 

Done.

http://codereview.chromium.org/1706013/diff/65002/41006#newcode10917
src/ia32/codegen-ia32.cc:10917: ASSERT(kSmiTag == 0 && kSmiTagSize);  // edi is
smi (powered by 2).
On 2010/04/28 13:33:41, Vyacheslav Egorov wrote:
> lost: == 1 in assertion.

Done.

http://codereview.chromium.org/1706013/diff/65002/41011
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/1706013/diff/65002/41011#newcode9747
src/x64/codegen-x64.cc:9747: 
On 2010/04/28 13:33:41, Vyacheslav Egorov wrote:
> empty line?

Done.

http://codereview.chromium.org/1706013/diff/65002/41011#newcode10318
src/x64/codegen-x64.cc:10318: __ testq(min_length, min_length);
On 2010/04/28 13:33:41, Vyacheslav Egorov wrote:
> ASSERT kSmiTag == 0 ?

Done.

http://codereview.chromium.org/1706013/diff/65002/41011#newcode10353
src/x64/codegen-x64.cc:10353: __ testq(length_difference, length_difference);
On 2010/04/28 13:33:41, Vyacheslav Egorov wrote:
> ASSERT kSmiTag == 0 ?

Done.

[Vyacheslav Egorov (Chromium), 2010-04-28 17:10:03]

LGTM (with helper in macro-assembler-arm.cc)

Still you should probably wait for Erik's opinion on ARM staff and his comments.

http://codereview.chromium.org/1706013/diff/35003/48015
File src/arm/macro-assembler-arm.cc (left):

http://codereview.chromium.org/1706013/diff/35003/48015#oldcode1101
src/arm/macro-assembler-arm.cc:1101: }
Well. This is getting hairy. I just noticed that all this pieces of code are the
same. They just have different scratch1, scratch2 usage. Changing them
simultaneously probably annoys you.

Best solution from my point of view is to create helper:

void InitializeStringFields (Register string, Register length, Register
scratch1, Register scratch2) {
  // Load goes first to improve pipeline utilization.
  LoadRoot(scratch1, Heap::kConsAsciiStringMapRootIndex);
  mov(scratch2, Operand(length, LSL, kSmiTagSize));
  str(scratch2, FieldMemOperand(string, String::kLengthOffset));
  mov(scratch2, Operand(String::kEmptyHashField));
  str(scratch2, FieldMemOperand(string, String::kHashFieldOffset)); 
  str(scratch1, FieldMemOperand(string, HeapObject::kMapOffset));
} 

What do you think? From my point of view it is very readable and not fragile
solution. Will it work?

[Erik Corry, 2010-04-28 18:24:35]

http://codereview.chromium.org/1706013/diff/65002/41014
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/1706013/diff/65002/41014#newcode9144
src/arm/codegen-arm.cc:9144: __ cmp(r2, Operand(0));  // Test if first string is
empty.
On 2010/04/28 13:33:41, Vyacheslav Egorov wrote:
> Comparing smi with non-smi.
> 
> Assert kSmiTag == 0 or use Smi::FromInt() ?
> 
> Another interesting question [yes I am aware that you are not the author of
the
> code] what is faster on ARM tst(x,x) or cmp(x, 0)?

Both take 1 cycle.

In the future I suppose that cmp(x, 0) might be faster, since it affects all the
flags so there's no false dependency on the previous state of the flags.

[Lasse Reichstein, 2010-04-29 07:04:06]

In X64 code, never assume anything about the representation of smis outside of
the Smi-macros in macro-assembler-x64.cc. Always use the Smi-macros for
operations on smis that are anything except moving them around uninterpreted
(movq is ok for that).
If there is no Smi-macro that does what you need, just implement one and
document its behavior (anything that isn't documented in the comments in
macro-assembler-x64.h is allowed to change without notice).

It's likely that we will try other tagging strategies in the  future, so we
don't want any implicit dependencies on the current representation.

http://codereview.chromium.org/1706013/diff/35003/48011
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/1706013/diff/35003/48011#newcode3924
src/x64/codegen-x64.cc:3924: __ cmpq(index.reg(), FieldOperand(object.reg(),
String::kLengthOffset));
Use SmiCompare(Operand,Register) instead.
Don't make any operation that assumes anything about Smi representation directly
in X64 code, use the SmiXXX macros instead.

http://codereview.chromium.org/1706013/diff/35003/48011#newcode5614
src/x64/codegen-x64.cc:5614: __ SmiSubConstant(temp2.reg(), temp2.reg(),
Smi::FromInt(1));
It's undocumented that SmiSubConstant sets flags like sub does. If you rely on
it, please add it to the documentation for SmiSubConstant in
macro-assembler-x64.h, so any rewrite of the function will preserve that.

http://codereview.chromium.org/1706013/diff/35003/48011#newcode7449
src/x64/codegen-x64.cc:7449: __ cmpq(rax, rbx);
Use SmiCompare

http://codereview.chromium.org/1706013/diff/35003/48011#newcode9685
src/x64/codegen-x64.cc:9685: __ testq(rcx, rcx);
Use SmiTest(rcx).
This one is important (don't assume that the tag or filler bits are zero! This
is likely to change).

http://codereview.chromium.org/1706013/diff/35003/48011#newcode9692
src/x64/codegen-x64.cc:9692: __ testq(rbx, rbx);
SmiTest again.

http://codereview.chromium.org/1706013/diff/35003/48011#newcode9721
src/x64/codegen-x64.cc:9721: __ addq(rbx, rcx);
Use SmiAdd when adding smis.

http://codereview.chromium.org/1706013/diff/35003/48011#newcode10303
src/x64/codegen-x64.cc:10303: __ subq(scratch4, FieldOperand(right,
String::kLengthOffset));
Use SmiSub for subtracting smis.

There is no SmiSub(Register, Operand), so either use movq to load from memory
and SmiSub(Register,Register), or add a SmiSub(Register,Operand) macro.,

http://codereview.chromium.org/1706013/diff/35003/48011#newcode10311
src/x64/codegen-x64.cc:10311: __ subq(scratch1, length_difference);
Use SmiSub.

http://codereview.chromium.org/1706013/diff/35003/48011#newcode10319
src/x64/codegen-x64.cc:10319: __ testq(min_length, min_length);
Use SmiTest

http://codereview.chromium.org/1706013/diff/35003/48011#newcode10354
src/x64/codegen-x64.cc:10354: ASSERT(kSmiTag == 0);
Don't ASSERT or assume anything about the smi tag value. Use Smi-macros instead
(SmiTest here).

[SeRya, 2010-04-29 12:16:35]

http://codereview.chromium.org/1706013/diff/65002/41014
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/1706013/diff/65002/41014#newcode9144
src/arm/codegen-arm.cc:9144: __ cmp(r2, Operand(0));  // Test if first string is
empty.
On 2010/04/28 18:24:35, Erik Corry wrote:
> On 2010/04/28 13:33:41, Vyacheslav Egorov wrote:
> > Comparing smi with non-smi.
> > 
> > Assert kSmiTag == 0 or use Smi::FromInt() ?
> > 
> > Another interesting question [yes I am aware that you are not the author of
> the
> > code] what is faster on ARM tst(x,x) or cmp(x, 0)?
> 
> Both take 1 cycle.
> 
> In the future I suppose that cmp(x, 0) might be faster, since it affects all
the
> flags so there's no false dependency on the previous state of the flags.
> 
> 

Changed back to cmp and added Smi::FromInt.

http://codereview.chromium.org/1706013/diff/35003/48015
File src/arm/macro-assembler-arm.cc (left):

http://codereview.chromium.org/1706013/diff/35003/48015#oldcode1101
src/arm/macro-assembler-arm.cc:1101: }
On 2010/04/28 17:10:04, Vyacheslav Egorov wrote:
> Well. This is getting hairy. I just noticed that all this pieces of code are
the
> same. They just have different scratch1, scratch2 usage. Changing them
> simultaneously probably annoys you.
> 
> Best solution from my point of view is to create helper:
> 
> void InitializeStringFields (Register string, Register length, Register
> scratch1, Register scratch2) {
>   // Load goes first to improve pipeline utilization.
>   LoadRoot(scratch1, Heap::kConsAsciiStringMapRootIndex);
>   mov(scratch2, Operand(length, LSL, kSmiTagSize));
>   str(scratch2, FieldMemOperand(string, String::kLengthOffset));
>   mov(scratch2, Operand(String::kEmptyHashField));
>   str(scratch2, FieldMemOperand(string, String::kHashFieldOffset)); 
>   str(scratch1, FieldMemOperand(string, HeapObject::kMapOffset));
> } 
> 
> What do you think? From my point of view it is very readable and not fragile
> solution. Will it work?

Done.

http://codereview.chromium.org/1706013/diff/35003/48011
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/1706013/diff/35003/48011#newcode3924
src/x64/codegen-x64.cc:3924: __ cmpq(index.reg(), FieldOperand(object.reg(),
String::kLengthOffset));
On 2010/04/29 07:04:19, Lasse Reichstein wrote:
> Use SmiCompare(Operand,Register) instead.
> Don't make any operation that assumes anything about Smi representation
directly
> in X64 code, use the SmiXXX macros instead.

Done.

http://codereview.chromium.org/1706013/diff/35003/48011#newcode5614
src/x64/codegen-x64.cc:5614: __ SmiSubConstant(temp2.reg(), temp2.reg(),
Smi::FromInt(1));
On 2010/04/29 07:04:19, Lasse Reichstein wrote:
> It's undocumented that SmiSubConstant sets flags like sub does. If you rely on
> it, please add it to the documentation for SmiSubConstant in
> macro-assembler-x64.h, so any rewrite of the function will preserve that.

Done.

http://codereview.chromium.org/1706013/diff/35003/48011#newcode7449
src/x64/codegen-x64.cc:7449: __ cmpq(rax, rbx);
On 2010/04/29 07:04:19, Lasse Reichstein wrote:
> Use SmiCompare

Done.

http://codereview.chromium.org/1706013/diff/35003/48011#newcode9685
src/x64/codegen-x64.cc:9685: __ testq(rcx, rcx);
On 2010/04/29 07:04:19, Lasse Reichstein wrote:
> Use SmiTest(rcx).
> This one is important (don't assume that the tag or filler bits are zero! This
> is likely to change).

Done.

http://codereview.chromium.org/1706013/diff/35003/48011#newcode9692
src/x64/codegen-x64.cc:9692: __ testq(rbx, rbx);
On 2010/04/29 07:04:19, Lasse Reichstein wrote:
> SmiTest again.

Done.

http://codereview.chromium.org/1706013/diff/35003/48011#newcode9721
src/x64/codegen-x64.cc:9721: __ addq(rbx, rcx);
On 2010/04/29 07:04:19, Lasse Reichstein wrote:
> Use SmiAdd when adding smis.

Done.

http://codereview.chromium.org/1706013/diff/35003/48011#newcode10303
src/x64/codegen-x64.cc:10303: __ subq(scratch4, FieldOperand(right,
String::kLengthOffset));
On 2010/04/29 07:04:19, Lasse Reichstein wrote:
> Use SmiSub for subtracting smis.
> 
> There is no SmiSub(Register, Operand), so either use movq to load from memory
> and SmiSub(Register,Register), or add a SmiSub(Register,Operand) macro.,

Done.

http://codereview.chromium.org/1706013/diff/35003/48011#newcode10311
src/x64/codegen-x64.cc:10311: __ subq(scratch1, length_difference);
On 2010/04/29 07:04:19, Lasse Reichstein wrote:
> Use SmiSub.

Done.

http://codereview.chromium.org/1706013/diff/35003/48011#newcode10319
src/x64/codegen-x64.cc:10319: __ testq(min_length, min_length);
On 2010/04/29 07:04:19, Lasse Reichstein wrote:
> Use SmiTest

Done.

http://codereview.chromium.org/1706013/diff/35003/48011#newcode10354
src/x64/codegen-x64.cc:10354: ASSERT(kSmiTag == 0);
On 2010/04/29 07:04:19, Lasse Reichstein wrote:
> Don't ASSERT or assume anything about the smi tag value. Use Smi-macros
instead
> (SmiTest here).

Done.