http://codereview.chromium.org/661469/diff/3001/2010 File src/arm/codegen-arm.cc (right):
http://codereview.chromium.org/661469/diff/3001/2010#newcode7273 src/arm/codegen-arm.cc:7273: // If check failed combine both characters into signle halfword. On 2010/03/05 10:28:50, Mads Ager wrote:
signle -> single
Done. http://codereview.chromium.org/661469/diff/3001/2010#newcode7273 src/arm/codegen-arm.cc:7273: // If check failed combine both characters into signle halfword. On 2010/03/05 10:25:16, Søren Gjesse wrote:
signle -> single. Please add to the comment why the branch is after
the orr. Done. http://codereview.chromium.org/661469/diff/3001/2010#newcode7284 src/arm/codegen-arm.cc:7284: // Collect the two characters in a register. On 2010/03/05 10:25:16, Søren Gjesse wrote:
Isn't this already done above?
No it is not. It is done above only if check fails. It cannot be done above unconditionally because both characters are required to calculate hash. http://codereview.chromium.org/661469/diff/3001/2010#newcode7304 src/arm/codegen-arm.cc:7304: __ sub(mask, mask, Operand(1)); On 2010/03/05 10:25:16, Søren Gjesse wrote:
Remove one of the empty lines.
Done. http://codereview.chromium.org/661469/diff/3001/2010#newcode7305 src/arm/codegen-arm.cc:7305: On 2010/03/05 10:28:50, Mads Ager wrote:
Remove extra empty line?
Done. http://codereview.chromium.org/661469/diff/3001/2010#newcode7310 src/arm/codegen-arm.cc:7310: Operand(SymbolTable::kElementsStartOffset - kHeapObjectTag)); On 2010/03/05 10:25:16, Søren Gjesse wrote:
Please remove two of the empty lines.
Done. http://codereview.chromium.org/661469/diff/3001/2010#newcode7310 src/arm/codegen-arm.cc:7310: Operand(SymbolTable::kElementsStartOffset - kHeapObjectTag)); On 2010/03/05 10:28:50, Mads Ager wrote:
Indentation one space off here?
Done. http://codereview.chromium.org/661469/diff/3001/2010#newcode7311 src/arm/codegen-arm.cc:7311: On 2010/03/05 10:28:50, Mads Ager wrote:
Remove extra empty lines.
Done. http://codereview.chromium.org/661469/diff/3001/2010#newcode7317 src/arm/codegen-arm.cc:7317: // symbol_table: symbol table On 2010/03/05 10:28:50, Mads Ager wrote:
symbol_table is not used below, so you should describe
symbol_table_element
here.
Done. http://codereview.chromium.org/661469/diff/3001/2010#newcode7317 src/arm/codegen-arm.cc:7317: // symbol_table: symbol table On 2010/03/05 10:25:16, Søren Gjesse wrote:
This is now first_symbol_table_element and it does not contain the
symbol table. Done. http://codereview.chromium.org/661469/diff/3001/2010#newcode7324 src/arm/codegen-arm.cc:7324: Label next_probe[kProbes], next_probe_pop_mask[kProbes]; On 2010/03/05 10:28:50, Mads Ager wrote:
I don't think you need the pop_mask variant since you do not push the
mask in
this version.
Done (in x64 version too) http://codereview.chromium.org/661469/diff/3001/2010#newcode7362 src/arm/codegen-arm.cc:7362: __ ldrh(scratch, FieldMemOperand(candidate, SeqAsciiString::kHeaderSize)); On 2010/03/05 10:28:50, Mads Ager wrote:
Loading a half-word puts zeros in the other halfword, right?
So it seems. I could not get a hold on official ARM Instruction Set Reference, but everything I managed to find indicates that ldrh/ldrb do zero-extension of loaded halfwords/bytes. http://codereview.chromium.org/661469/diff/3001/2010#newcode7542 src/arm/codegen-arm.cc:7542: __ strh(r3, MemOperand(r1)); On 2010/03/05 10:28:50, Mads Ager wrote:
Can you combine the add and the strh here and use a FieldMemOperand?
strh(r3, FieldMemOperand(r0, SeqAsciiString::kHeaderSize))?
Done. http://codereview.chromium.org/661469 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
