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