http://codereview.chromium.org/460109/diff/3001/3005 File src/x64/assembler-x64.h (right):
http://codereview.chromium.org/460109/diff/3001/3005#newcode537 src/x64/assembler-x64.h:537: void movw(Register src, const Operand& dst); On 2009/12/08 14:16:11, Lasse Reichstein wrote: > I would use movzxwl for loading instead, to avoid dependencies on the higher > bits. Good point. Changed the use of movw to movzxwl in the string copy code. Removed this instruction from the assembler again, as it is not used then and probably it will not be useful. http://codereview.chromium.org/460109/diff/3001/3006 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/460109/diff/3001/3006#newcode7800 src/x64/codegen-x64.cc:7800: StringAddStub stub(NO_STRING_CHECK_IN_STUB); On 2009/12/08 14:16:11, Lasse Reichstein wrote: > so NO_STRING_CHECK_IN_STUB means that the arguments are known to be strings. > Maybe rename it to ARGUMENTS_ARE_STRINGS. This follows the naming convention used by other stubs e.g. NO_SMI_CHECK_IN_STUB, so I will leave it like this. http://codereview.chromium.org/460109/diff/3001/3006#newcode7931 src/x64/codegen-x64.cc:7931: // edx: second string On 2009/12/08 14:16:11, Lasse Reichstein wrote: > Maybe comment on r8/r9 potential content here. Done. http://codereview.chromium.org/460109/diff/3001/3006#newcode7940 src/x64/codegen-x64.cc:7940: // If arguments where known to be strings maps where where not loaded to On 2009/12/08 14:16:11, Lasse Reichstein wrote: > "where" -> "were" (or "are"), comma after "strings", "where where"->"were" Done. http://codereview.chromium.org/460109/diff/3001/3006#newcode7953 src/x64/codegen-x64.cc:7953: ASSERT((String::kMaxLength & 0x80000000) == 0); On 2009/12/08 14:16:11, Lasse Reichstein wrote: > It should be safe to assume that kMaxLength isn't negative :) String::kMaxLength has type int so I am not sure thats right. Is 0x80000000 negative if sizeof(int) is 8? The cmpl below should be changed to cmpq then. http://codereview.chromium.org/460109/diff/3001/3006#newcode7957 src/x64/codegen-x64.cc:7957: // If result is not supposed to be flat allocate a cons string object. If both On 2009/12/08 14:16:11, Lasse Reichstein wrote: > comma after "flat". Done. http://codereview.chromium.org/460109/diff/3001/3006#newcode7966 src/x64/codegen-x64.cc:7966: __ and_(rcx, r9); On 2009/12/08 14:16:11, Lasse Reichstein wrote: > Seems to assume that kAsciiStringTag == kStringEncodingMask, and that it's only > one bit (which is true, but could you assert it?) Added ASSERT_EQ(kStringEncodingMask, kAsciiStringTag); and changed ASSERT(kAsciiStringTag != 0); below as well. Same changes to codegen-ia32.cc. http://codereview.chromium.org/460109/diff/3001/3006#newcode8001 src/x64/codegen-x64.cc:8001: __ j(equal, &string_add_runtime); On 2009/12/08 14:16:11, Lasse Reichstein wrote: > Is there a plan to make it work for external strings (shouldn't be that hard, > they are known to be flat so it's just finding a different place to copy from > than for sequential strings). The problem with this is that the external string has a pointer to a C++ object which provides the address and length of the string buffer through virtual methods implemented by the embedder. http://codereview.chromium.org/460109/diff/3001/3006#newcode8030 src/x64/codegen-x64.cc:8030: //__ movq(rdx, Operand(rsp, 1 * kPointerSize)); On 2009/12/08 14:16:11, Lasse Reichstein wrote: > Code in comment? Deleted. http://codereview.chromium.org/460109/diff/3001/3006#newcode8089 src/x64/codegen-x64.cc:8089: bool ascii) { On 2009/12/08 14:16:11, Lasse Reichstein wrote: > You are really moving (count * (ascii ? 1 : 2)) bytes. > Maybe just do that, and avoid the movw operations. > You could use the counter as index in the operands and avoid updating dst and > src in the loop. > But again, it's just for short loops. I agree that this is a rather brain-dead implementation. I am quite sure we are going to be copying longer strings soon, so I will leave it like this for now. http://codereview.chromium.org/460109/diff/3001/3007 File src/x64/codegen-x64.h (right): http://codereview.chromium.org/460109/diff/3001/3007#newcode751 src/x64/codegen-x64.h:751: NO_STRING_CHECK_IN_STUB = 1 << 0 // Omit string check in stub. On 2009/12/08 14:16:11, Lasse Reichstein wrote: > Is the "0" in "1 << 0" a significant constant? If it is, give it a name, > otherwise just use "1". This way of specifying bits for flag sets as well (even when only one flag is used), so I will leave it like this. http://codereview.chromium.org/460109/diff/3001/3007#newcode763 src/x64/codegen-x64.h:763: int MinorKey() { return string_check_ ? 0 : 1; } On 2009/12/08 14:16:11, Lasse Reichstein wrote: > Why not use the flag value directly? The code stub minor key needs to be an integer. http://codereview.chromium.org/460109/diff/3001/3002 File src/x64/macro-assembler-x64.cc (right): http://codereview.chromium.org/460109/diff/3001/3002#newcode2264 src/x64/macro-assembler-x64.cc:2264: shl(scratch1, Immediate(1)); On 2009/12/08 14:16:11, Lasse Reichstein wrote: > lea(scratch1, Operand(length, length, times_1, 0)) > does the above two operations in one :) Good point, changed it to lea(scratch1, Operand(length, length, times_1, kObjectAlignmentMask)); and got rid of the addq below as well. Same change to macro-assembler-ia32.cc. http://codereview.chromium.org/460109/diff/3001/3002#newcode2296 src/x64/macro-assembler-x64.cc:2296: movsxlq(scratch1, length); On 2009/12/08 14:16:11, Lasse Reichstein wrote: > Sign extending shouldn't be necessary, since length should not be negative. Right, I was missing movzxlq which turns out to be just movl which is now used. http://codereview.chromium.org/460109 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
