This seems to be heading in the right direction, but there is a lot more
work to
do... this is a first round of comments but there will be more to come as I
make
more progress on the review
https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):
https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc#newcode733
src/x64/code-stubs-x64.cc:733: (kSmiValueSize == 31 && op == Token::SHL)
||
Is think you should turn this into a predicate:
SmisAreUint32()
that does this test.
https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc#newcode797
src/x64/code-stubs-x64.cc:797: if (kSmiValueSize == 31) {
Why can't you put this extra stack manipulation in the 31-bit case
inside the SmiShiftLeft macro assembler instruction??
https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc#newcode814
src/x64/code-stubs-x64.cc:814: if (kSmiValueSize == 31) {
Same here.
https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc#newcode832
src/x64/code-stubs-x64.cc:832: if (use_fp_on_smis.is_linked()) {
All of this use_fp_on_smis code can be encapsulated in the macro
assembler, can't it? You shouldn't even have to pass use_fp_on_smi as a
label, only the fail label, since the fp-handling case only needs to
jump to that code if it overflow the smi range. I know that's a bigger
change, but if you just pass generate_inline_heapnumber_results to the
macro assembler instructions for all of the math operations and the fail
label, I think you can handle everything there.
https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc#newcode887
src/x64/code-stubs-x64.cc:887: default: UNREACHABLE();
Also, I think you can share much of this code with the version in the
32-bit version. Only the stack pop restore at the end needs to be added
in the 31-bit case.
https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc#newcode942
src/x64/code-stubs-x64.cc:942: static void
BinaryOpStub_GenerateFloatingPointCode(MacroAssembler* masm,
start the parameter list on the next line with a 4-char indentations, it
saves you the weird formatting of result_type below.
https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc#newcode1241
src/x64/code-stubs-x64.cc:1241: op_, result_type_, ¬_int32, mode_);
strange indentation
https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc#newcode4766
src/x64/code-stubs-x64.cc:4766: __ SmiAdd(rbx, rbx, rcx);
As noted above, I think you should change SmiAdd to handle the 31-bit
cases in the macro-assmebler and have a single interface
https://codereview.chromium.org/21014003/diff/17001/src/x64/debug-x64.cc
File src/x64/debug-x64.cc (right):
https://codereview.chromium.org/21014003/diff/17001/src/x64/debug-x64.cc#newcode129
src/x64/debug-x64.cc:129: __ movq(kScratchRegister, reg);
Watch out. You have change the semantic here slightly, since 64-bits can
be encoded in two 31-bit values. However, in practice, all the uses of
this code actually only expect integers (32 bits) to be encoded, so I
this you can change the semantic to only pack the 32 lower bits sign
extended.
I think this code sequence (and the restore sequence in this file)
should be moved into the macro assembler:
__ PushInt32AsTwoSmis(reg, kScratchRegister)
__ PopInt32AsTwoSmis(reg, kScratchRegister);
and then hide the smi size check in the macro assembler.
https://codereview.chromium.org/21014003/diff/17001/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):
https://codereview.chromium.org/21014003/diff/17001/src/x64/full-codegen-x64.cc#newcode4456
src/x64/full-codegen-x64.cc:4456: __ SmiAddConstant(rax, rax,
Smi::FromInt(1));
Handle the overflow case inside the macro assembler, passing the
stub-call label always
https://codereview.chromium.org/21014003/diff/17001/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):
https://codereview.chromium.org/21014003/diff/17001/src/x64/lithium-codegen-x64.cc#newcode1314
src/x64/lithium-codegen-x64.cc:1314: __ imul(left, ToOperand(right));
Move this switching into the macro assembler, you will need to introduce
__ SmiIMul, etc.
https://codereview.chromium.org/21014003/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
