LGTM, only minor comments.
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); I would use movzxwl for loading instead, to avoid dependencies on the higher bits. 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); so NO_STRING_CHECK_IN_STUB means that the arguments are known to be strings. Maybe rename it to ARGUMENTS_ARE_STRINGS. http://codereview.chromium.org/460109/diff/3001/3006#newcode7931 src/x64/codegen-x64.cc:7931: // edx: second string Maybe comment on r8/r9 potential content here. 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 "where" -> "were" (or "are"), comma after "strings", "where where"->"were" http://codereview.chromium.org/460109/diff/3001/3006#newcode7953 src/x64/codegen-x64.cc:7953: ASSERT((String::kMaxLength & 0x80000000) == 0); It should be safe to assume that kMaxLength isn't negative :) 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 comma after "flat". http://codereview.chromium.org/460109/diff/3001/3006#newcode7966 src/x64/codegen-x64.cc:7966: __ and_(rcx, r9); Seems to assume that kAsciiStringTag == kStringEncodingMask, and that it's only one bit (which is true, but could you assert it?) http://codereview.chromium.org/460109/diff/3001/3006#newcode8001 src/x64/codegen-x64.cc:8001: __ j(equal, &string_add_runtime); 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). http://codereview.chromium.org/460109/diff/3001/3006#newcode8030 src/x64/codegen-x64.cc:8030: //__ movq(rdx, Operand(rsp, 1 * kPointerSize)); Code in comment? http://codereview.chromium.org/460109/diff/3001/3006#newcode8089 src/x64/codegen-x64.cc:8089: bool ascii) { 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. 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. Is the "0" in "1 << 0" a significant constant? If it is, give it a name, otherwise just use "1". http://codereview.chromium.org/460109/diff/3001/3007#newcode763 src/x64/codegen-x64.h:763: int MinorKey() { return string_check_ ? 0 : 1; } Why not use the flag value directly? 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)); lea(scratch1, Operand(length, length, times_1, 0)) does the above two operations in one :) http://codereview.chromium.org/460109/diff/3001/3002#newcode2296 src/x64/macro-assembler-x64.cc:2296: movsxlq(scratch1, length); Sign extending shouldn't be necessary, since length should not be negative. http://codereview.chromium.org/460109 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
