http://codereview.chromium.org/6366028/diff/6010/src/x64/code-stubs-x64.cc File src/x64/code-stubs-x64.cc (right):
http://codereview.chromium.org/6366028/diff/6010/src/x64/code-stubs-x64.cc#newcode1151 src/x64/code-stubs-x64.cc:1151: __ SmiMod(rax, left, right, &restore_MOD_registers); No. Eliminated. On 2011/02/03 08:00:43, Søren Gjesse wrote:
Do you need the label "restore_MOD_registers"? It is bound the same
place as
"use_fp_on_smis" where there is a check for both DIV and MOD.
http://codereview.chromium.org/6366028/diff/6010/src/x64/code-stubs-x64.cc#newcode1205 src/x64/code-stubs-x64.cc:1205: if (generate_inline_heapnumber_results) { On 2011/02/03 08:00:43, Søren Gjesse wrote:
Can't you take mode_ into account here and reuse an overwritable heap
number if
any?
We only reach here if the inputs were both Smis. Otherwise, we skip to the end of the Smi code. http://codereview.chromium.org/6366028/diff/6010/src/x64/code-stubs-x64.cc#newcode1226 src/x64/code-stubs-x64.cc:1226: if (op_ == Token::BIT_OR) { On 2011/02/03 08:00:43, Søren Gjesse wrote:
rax -> right?
Done. http://codereview.chromium.org/6366028/diff/6010/src/x64/code-stubs-x64.cc#newcode1300 src/x64/code-stubs-x64.cc:1300: // Not using AllocateHeapNumber macro in order to reuse On 2011/02/03 08:00:43, Søren Gjesse wrote:
How about creating an AllocateHeapNumber in macro assembler which
takes an
additional register with the heap number map?
It is only used this one place, and it is really just two lines plus a debug check. So I don't think it is worth it. http://codereview.chromium.org/6366028/diff/6010/src/x64/code-stubs-x64.cc#newcode1347 src/x64/code-stubs-x64.cc:1347: Condition is_smi; On 2011/02/03 08:00:43, Søren Gjesse wrote:
Please remove this debugging code.
Done. http://codereview.chromium.org/6366028/diff/6010/src/x64/code-stubs-x64.cc#newcode1350 src/x64/code-stubs-x64.cc:1350: is_smi = masm->CheckSmi(lhs); On 2011/02/03 08:00:43, Søren Gjesse wrote:
The code pattern for checking for a string is repeated three times.
How about
JumpIfNotString in macro assembler?
In one of the three places, the smi check jumps to a different place than the string check. We already have a macro for the Smi check, so the remaining string check becomes 2 lines. So I used the macro for the smi check. http://codereview.chromium.org/6366028/diff/6010/src/x64/code-stubs-x64.cc#newcode1451 src/x64/code-stubs-x64.cc:1451: __ bind(&call_runtime); There should be no ALLOW_HEAPNUMBER_RESULTS case on X64, except when generating the generic stub. Removed. On 2011/02/03 08:00:43, Søren Gjesse wrote:
Should there be a separate exit for failed heap allocation in the ALLOW_HEAPNUMBER_RESULTS case which does not perform a type
transition, but just
calls runtime?
http://codereview.chromium.org/6366028/diff/6010/src/x64/code-stubs-x64.cc#newcode1465 src/x64/code-stubs-x64.cc:1465: Label call_runtime, type_transition; On 2011/02/03 08:00:43, Søren Gjesse wrote:
Maybe rename the labels
call_runtime -> gc_required type_transition -> not_number
to make it more clear why the exits are called.
Done. http://codereview.chromium.org/6366028/diff/6010/src/x64/full-codegen-x64.cc File src/x64/full-codegen-x64.cc (right): http://codereview.chromium.org/6366028/diff/6010/src/x64/full-codegen-x64.cc#newcode3209 src/x64/full-codegen-x64.cc:3209: __ j(is_smi, &done); On 2011/02/03 08:00:43, Søren Gjesse wrote:
Remove empty line?
I like an empty line before a new section of generated code, that does not expect fall-through in most cases. http://codereview.chromium.org/6366028/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
