danno, thanks a lot for the review. All the comments are addressed. Please
take
another look.
https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):
https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc#newcode904
src/x64/code-stubs-x64.cc:904: if (result_type <= BinaryOpIC::INT32) {
Actually this code was taken from
https://chromiumcodereview.appspot.com/10937013 when we fixed the
deopt-minus-zero fail issue for X32. It is a little out-dated. Now I
migrate the latest IA32 part of code here to deal with 31-bit SMI. From
debugging deopt-minus-zero, I think this code could verify whether the
result is -0 also as -0 could not be represented as int32.
https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc#newcode905
src/x64/code-stubs-x64.cc:905: __ cvttsd2si(kScratchRegister, xmm0);
The test here is for Int32, instead of SMI.
https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc#newcode910
src/x64/code-stubs-x64.cc:910: __ j(zero, non_int32_failure);
The main logic is at
https://code.google.com/p/v8/source/browse/branches/bleeding_edge/src/ic.cc#2681.
For 32-bit SMI, the result type is heap number as int32 and SMI are the
same for 32-bit SMI.
https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc#newcode935
src/x64/code-stubs-x64.cc:935: __ movq(r11, rax);
On 2013/08/07 18:41:26, danno wrote:
Can you give r11 an alias, like saved_right?
Done.
https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc#newcode967
src/x64/code-stubs-x64.cc:967: ASSERT(op == Token::SHR);
On 2013/08/07 18:41:26, danno wrote:
It looks like all three four below are identical (including the
cvtqsi2sd)
except for a little prologue. Please combine them, only adding the
additional
prologue step if needed depending on mode_ and kSmiValueSize/op.
Done.
https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc#newcode1132
src/x64/code-stubs-x64.cc:1132: if (kSmiValueSize == 32) {
On 2013/08/07 18:41:26, danno wrote:
Remove the if and the "then" part of this code, just put the else
version in
straight-line, which already asserts "kSmiValueSize == 31"
Done.
https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc#newcode4669
src/x64/code-stubs-x64.cc:4669: __ SmiAdd(rbx, rbx, rcx, &call_runtime);
On 2013/08/07 18:41:26, danno wrote:
I still think it's OK to use the overflow-checking version. This is
because full
code gen is not super performance critical anyway, any critical code
will be
Crankshafted. I would opt to use the version that is more maintainable
and
readable, i.e. the "__ SmiAdd(rbx, rbx, rcx, &call_runtime);"
version.
Done.
https://codereview.chromium.org/21014003/diff/34001/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):
https://codereview.chromium.org/21014003/diff/34001/src/x64/lithium-codegen-x64.cc#newcode1402
src/x64/lithium-codegen-x64.cc:1402: if
(instr->hydrogen()->representation().IsSmi()) {
It is really correct. This code utilizes a hardware feature that
immediate32 will be signed extend to 64-bit. The full bitwise operation
(and_, or_ and xor_) works for both 32-bit SMI and 31-bit SMI. For
31-bit SMI, from 63rd to 30th bit are all the same for an operand,
"bitop_(src1, src2)" has the same result of "bitl(src1, src2);
movsxlq(src1, src1);".
https://codereview.chromium.org/21014003/diff/34001/src/x64/lithium-codegen-x64.cc#newcode1410
src/x64/lithium-codegen-x64.cc:1410: __ or_(ToRegister(left),
Immediate(right_operand));
Explained above.
https://codereview.chromium.org/21014003/diff/34001/src/x64/lithium-codegen-x64.cc#newcode1416
src/x64/lithium-codegen-x64.cc:1416: if
(instr->hydrogen()->representation().IsSmi()) {
Explained above.
https://codereview.chromium.org/21014003/diff/34001/src/x64/lithium-codegen-x64.cc#newcode1755
src/x64/lithium-codegen-x64.cc:1755: __ lea(ToRegister(instr->result()),
On 2013/08/07 18:41:26, danno wrote:
Here and below, I think all Smi operations should be in the
MacroAssembler.
Done.
https://codereview.chromium.org/21014003/diff/34001/src/x64/macro-assembler-x64.h
File src/x64/macro-assembler-x64.h (right):
https://codereview.chromium.org/21014003/diff/34001/src/x64/macro-assembler-x64.h#newcode378
src/x64/macro-assembler-x64.h:378: class SmiFunctionInvoker {
Thanks for the recommendation. I used a slightly different version:
class SmiInstructionWrapper {
public:
SmiInstructionWrapper() { }
virtual ~SmiInstructionWrapper() { }
virtual bool NeedsCheckOverflow() const = 0;
virtual bool NeedsCheckMinusZero() const = 0;
virtual bool NeedsKeepSourceOperandsIntact() const = 0;
virtual void BailoutIf(Condition cc) const = 0;
};
It seems that BailoutIf(Condition cc) is more generally. I have used it
to handle SmiShiftLeftConstant, SmiAdd and SmiSub. It should be able to
handle SmiMul (a work in progress at
https://codereview.chromium.org/22600005/).
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.
