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.
http://codereview.chromium.org/1706013/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
