Revision: 4598
Author: [email protected]
Date: Thu May 6 01:15:15 2010
Log: Correct bug with left shift on X64 platform from change 4571
(http://code.google.com/p/v8/source/detail?r=4571). Speed up left shift
with a constant left hand side on X64 platform. Add unit test for this
bug. Remove unused failure target argument from
MacroAssembler::SmiShiftLeft and MacroAssembler::SmiShiftLeftConstant.
Review URL: http://codereview.chromium.org/1934004
http://code.google.com/p/v8/source/detail?r=4598
Modified:
/branches/bleeding_edge/src/x64/codegen-x64.cc
/branches/bleeding_edge/src/x64/macro-assembler-x64.cc
/branches/bleeding_edge/src/x64/macro-assembler-x64.h
/branches/bleeding_edge/test/cctest/test-macro-assembler-x64.cc
/branches/bleeding_edge/test/mjsunit/smi-ops.js
=======================================
--- /branches/bleeding_edge/src/x64/codegen-x64.cc Wed May 5 01:56:16 2010
+++ /branches/bleeding_edge/src/x64/codegen-x64.cc Thu May 6 01:15:15 2010
@@ -6526,43 +6526,37 @@
case Token::SHL:
if (reversed) {
- // Move operand into rcx and also into a second register.
- // If operand is already in a register, take advantage of that.
- // This lets us modify rcx, but still bail out to deferred code.
- Result right;
- Result right_copy_in_rcx;
- TypeInfo right_type_info = operand->type_info();
operand->ToRegister();
+
+ // We need rcx to be available to hold operand, and to be spilled.
+ // SmiShiftLeft implicitly modifies rcx.
if (operand->reg().is(rcx)) {
- right = allocator()->Allocate();
- __ movq(right.reg(), rcx);
- frame_->Spill(rcx);
- right_copy_in_rcx = *operand;
+ frame_->Spill(operand->reg());
+ answer = allocator()->Allocate();
} else {
- right_copy_in_rcx = allocator()->Allocate(rcx);
- __ movq(rcx, operand->reg());
- right = *operand;
- }
- operand->Unuse();
-
- answer = allocator()->Allocate();
+ Result rcx_reg = allocator()->Allocate(rcx);
+ // answer must not be rcx.
+ answer = allocator()->Allocate();
+ // rcx_reg goes out of scope.
+ }
+
DeferredInlineSmiOperationReversed* deferred =
new DeferredInlineSmiOperationReversed(op,
answer.reg(),
smi_value,
- right.reg(),
+ operand->reg(),
overwrite_mode);
- __ movq(answer.reg(), Immediate(int_value));
- __ SmiToInteger32(rcx, rcx);
- if (!right_type_info.IsSmi()) {
- Condition is_smi = masm_->CheckSmi(right.reg());
+ if (!operand->type_info().IsSmi()) {
+ Condition is_smi = masm_->CheckSmi(operand->reg());
deferred->Branch(NegateCondition(is_smi));
} else if (FLAG_debug_code) {
- __ AbortIfNotSmi(right.reg(),
+ __ AbortIfNotSmi(operand->reg(),
"Static type info claims non-smi is smi in (const SHL
smi).");
}
- __ shl_cl(answer.reg());
- __ Integer32ToSmi(answer.reg(), answer.reg());
+
+ __ Move(answer.reg(), smi_value);
+ __ SmiShiftLeft(answer.reg(), answer.reg(), operand->reg());
+ operand->Unuse();
deferred->BindExit();
} else {
@@ -6595,8 +6589,7 @@
__ JumpIfNotSmi(operand->reg(), deferred->entry_label());
__ SmiShiftLeftConstant(answer.reg(),
operand->reg(),
- shift_value,
- deferred->entry_label());
+ shift_value);
deferred->BindExit();
operand->Unuse();
}
@@ -6837,8 +6830,7 @@
case Token::SHL: {
__ SmiShiftLeft(answer.reg(),
left->reg(),
- rcx,
- deferred->entry_label());
+ rcx);
break;
}
default:
@@ -9934,7 +9926,7 @@
__ SmiShiftLogicalRight(left, left, right, slow);
break;
case Token::SHL:
- __ SmiShiftLeft(left, left, right, slow);
+ __ SmiShiftLeft(left, left, right);
break;
default:
UNREACHABLE();
=======================================
--- /branches/bleeding_edge/src/x64/macro-assembler-x64.cc Tue May 4
07:49:50 2010
+++ /branches/bleeding_edge/src/x64/macro-assembler-x64.cc Thu May 6
01:15:15 2010
@@ -1227,8 +1227,7 @@
void MacroAssembler::SmiShiftLeftConstant(Register dst,
Register src,
- int shift_value,
- Label* on_not_smi_result) {
+ int shift_value) {
if (!dst.is(src)) {
movq(dst, src);
}
@@ -1240,8 +1239,7 @@
void MacroAssembler::SmiShiftLeft(Register dst,
Register src1,
- Register src2,
- Label* on_not_smi_result) {
+ Register src2) {
ASSERT(!dst.is(rcx));
Label result_ok;
// Untag shift amount.
=======================================
--- /branches/bleeding_edge/src/x64/macro-assembler-x64.h Tue May 4
07:49:50 2010
+++ /branches/bleeding_edge/src/x64/macro-assembler-x64.h Thu May 6
01:15:15 2010
@@ -374,8 +374,7 @@
void SmiShiftLeftConstant(Register dst,
Register src,
- int shift_value,
- Label* on_not_smi_result);
+ int shift_value);
void SmiShiftLogicalRightConstant(Register dst,
Register src,
int shift_value,
@@ -388,8 +387,7 @@
// Uses and clobbers rcx, so dst may not be rcx.
void SmiShiftLeft(Register dst,
Register src1,
- Register src2,
- Label* on_not_smi_result);
+ Register src2);
// Shifts a smi value to the right, shifting in zero bits at the top, and
// returns the unsigned intepretation of the result if that is a smi.
// Uses and clobbers rcx, so dst may not be rcx.
=======================================
--- /branches/bleeding_edge/test/cctest/test-macro-assembler-x64.cc Thu Dec
10 09:46:45 2009
+++ /branches/bleeding_edge/test/cctest/test-macro-assembler-x64.cc Thu
May 6 01:15:15 2010
@@ -1737,99 +1737,51 @@
// rax == id + i * 10.
int shift = shifts[i];
int result = x << shift;
- if (Smi::IsValid(result)) {
- __ Move(r8, Smi::FromInt(result));
- __ Move(rcx, Smi::FromInt(x));
- __ SmiShiftLeftConstant(r9, rcx, shift, exit);
-
- __ incq(rax);
- __ SmiCompare(r9, r8);
- __ j(not_equal, exit);
-
- __ incq(rax);
- __ Move(rcx, Smi::FromInt(x));
- __ SmiShiftLeftConstant(rcx, rcx, shift, exit);
-
- __ incq(rax);
- __ SmiCompare(rcx, r8);
- __ j(not_equal, exit);
-
- __ incq(rax);
- __ Move(rdx, Smi::FromInt(x));
- __ Move(rcx, Smi::FromInt(shift));
- __ SmiShiftLeft(r9, rdx, rcx, exit);
-
- __ incq(rax);
- __ SmiCompare(r9, r8);
- __ j(not_equal, exit);
-
- __ incq(rax);
- __ Move(rdx, Smi::FromInt(x));
- __ Move(r11, Smi::FromInt(shift));
- __ SmiShiftLeft(r9, rdx, r11, exit);
-
- __ incq(rax);
- __ SmiCompare(r9, r8);
- __ j(not_equal, exit);
-
- __ incq(rax);
- __ Move(rdx, Smi::FromInt(x));
- __ Move(r11, Smi::FromInt(shift));
- __ SmiShiftLeft(rdx, rdx, r11, exit);
-
- __ incq(rax);
- __ SmiCompare(rdx, r8);
- __ j(not_equal, exit);
-
- __ incq(rax);
- } else {
- // Cannot happen with long smis.
- Label fail_ok;
- __ Move(rcx, Smi::FromInt(x));
- __ movq(r11, rcx);
- __ SmiShiftLeftConstant(r9, rcx, shift, &fail_ok);
- __ jmp(exit);
- __ bind(&fail_ok);
-
- __ incq(rax);
- __ SmiCompare(rcx, r11);
- __ j(not_equal, exit);
-
- __ incq(rax);
- Label fail_ok2;
- __ SmiShiftLeftConstant(rcx, rcx, shift, &fail_ok2);
- __ jmp(exit);
- __ bind(&fail_ok2);
-
- __ incq(rax);
- __ SmiCompare(rcx, r11);
- __ j(not_equal, exit);
-
- __ incq(rax);
- __ Move(r8, Smi::FromInt(shift));
- Label fail_ok3;
- __ SmiShiftLeft(r9, rcx, r8, &fail_ok3);
- __ jmp(exit);
- __ bind(&fail_ok3);
-
- __ incq(rax);
- __ SmiCompare(rcx, r11);
- __ j(not_equal, exit);
-
- __ incq(rax);
- __ Move(r8, Smi::FromInt(shift));
- __ movq(rdx, r11);
- Label fail_ok4;
- __ SmiShiftLeft(rdx, rdx, r8, &fail_ok4);
- __ jmp(exit);
- __ bind(&fail_ok4);
-
- __ incq(rax);
- __ SmiCompare(rdx, r11);
- __ j(not_equal, exit);
-
- __ addq(rax, Immediate(3));
- }
+ CHECK(Smi::IsValid(result));
+ __ Move(r8, Smi::FromInt(result));
+ __ Move(rcx, Smi::FromInt(x));
+ __ SmiShiftLeftConstant(r9, rcx, shift);
+
+ __ incq(rax);
+ __ SmiCompare(r9, r8);
+ __ j(not_equal, exit);
+
+ __ incq(rax);
+ __ Move(rcx, Smi::FromInt(x));
+ __ SmiShiftLeftConstant(rcx, rcx, shift);
+
+ __ incq(rax);
+ __ SmiCompare(rcx, r8);
+ __ j(not_equal, exit);
+
+ __ incq(rax);
+ __ Move(rdx, Smi::FromInt(x));
+ __ Move(rcx, Smi::FromInt(shift));
+ __ SmiShiftLeft(r9, rdx, rcx);
+
+ __ incq(rax);
+ __ SmiCompare(r9, r8);
+ __ j(not_equal, exit);
+
+ __ incq(rax);
+ __ Move(rdx, Smi::FromInt(x));
+ __ Move(r11, Smi::FromInt(shift));
+ __ SmiShiftLeft(r9, rdx, r11);
+
+ __ incq(rax);
+ __ SmiCompare(r9, r8);
+ __ j(not_equal, exit);
+
+ __ incq(rax);
+ __ Move(rdx, Smi::FromInt(x));
+ __ Move(r11, Smi::FromInt(shift));
+ __ SmiShiftLeft(rdx, rdx, r11);
+
+ __ incq(rax);
+ __ SmiCompare(rdx, r8);
+ __ j(not_equal, exit);
+
+ __ incq(rax);
}
}
=======================================
--- /branches/bleeding_edge/test/mjsunit/smi-ops.js Thu Apr 22 02:02:10 2010
+++ /branches/bleeding_edge/test/mjsunit/smi-ops.js Thu May 6 01:15:15 2010
@@ -678,3 +678,10 @@
assertEquals(4589934592, LogicalShiftRightByMultipleOf32(-2000000000));
assertEquals(4589934592, LogicalShiftRightByMultipleOf32(-2000000000));
+
+// Verify that the shift amount is reduced modulo 32, not modulo 64.
+function LeftShiftThreeBy(x) {return 3 << x;}
+assertEquals(24, LeftShiftThreeBy(3));
+assertEquals(24, LeftShiftThreeBy(35));
+assertEquals(24, LeftShiftThreeBy(67));
+assertEquals(24, LeftShiftThreeBy(-29));
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
