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