danno, thanks a lot for your review. I am glad to see I have anticipated
some of
your comments and already addressed them in patch set 5 :)
I have uploaded the patch set 6, I will rebase with master this weekend and
deal
with the SHL operation in hydrogen and address your other comment in
hydrogen.
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)
||
Done. Add IsUnsafeSmiOperator function in the macro assembler.
https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc#newcode797
src/x64/code-stubs-x64.cc:797: if (kSmiValueSize == 31) {
On 2013/08/01 16:45:41, danno wrote:
Why can't you put this extra stack manipulation in the 31-bit case
inside the
SmiShiftLeft macro assembler instruction??
Done.
https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc#newcode814
src/x64/code-stubs-x64.cc:814: if (kSmiValueSize == 31) {
On 2013/08/01 16:45:41, danno wrote:
Same here.
Done.
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()) {
The X64 implementation utilizes a fact that when SHR 0 overflow, i.e., a
negative has to be represented as positive, the binary representation
are the same. If we do not use this optimization (we need to add an
overhead of movq(dst, src1) , I could get the code logic nearly the same
for 31-bit SMI and 32-bit SMI.
On 2013/08/01 16:45:41, danno wrote:
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();
On 2013/08/01 16:45:41, danno wrote:
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.
Done.
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,
On 2013/08/01 16:45:41, danno wrote:
start the parameter list on the next line with a 4-char indentations,
it saves
you the weird formatting of result_type below.
Done.
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_);
On 2013/08/01 16:45:41, danno wrote:
strange indentation
Done.
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);
There are two SmiAdd interfaces. SmiAdd (with no fail) is used only when
it's known that overflowing is impossible. I have handled the interface
with fail in macro-assembler. Here it is safe to use __ SmiAdd(rbx, rbx,
rcx, &call_runtime), but we have un-necessary runtime overhead for the
32-bit SMI.
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);
Done. I used the name PushInt64AsTwoSmis and PopInt64AsTwoSmis.
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));
I am OK to change this but this might affect performance. For the
SmiAddConstant with overflow checking, we will movq(kScratchRegister,
rax); add(kScratchRegister, LoadSmiRegister(1)); j(overflow, fail,
near_jump); movq(rax, kScratchRegister). We need 4 instructions in the
normal case than 2 (add, check overflow) inside a loop.
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.
