Reviewers: ulan, jochen (OOO until Sept),
Message:
If there's a better way to exercise this code without relying on
--noopt-safe-uint32-operations, please let me know. (I couldn't find one.)
Description:
ARM64: Fix SHR logic error.
The `right == 0` checks only worked for `0 <= right < 32`. This patch
replaces the checks with simple tests for negative results.
The attached test can detect this error, but the test relies on a broken
flag (--noopt-safe-uint32-operations), so it is skipped for now. See
issue 3487 for details.
BUG=
Please review this at https://codereview.chromium.org/487913005/
SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge
Affected files (+46, -20 lines):
M src/arm64/full-codegen-arm64.cc
M src/arm64/lithium-codegen-arm64.cc
A test/mjsunit/compiler/shift-shr.js
M test/mjsunit/mjsunit.status
Index: src/arm64/full-codegen-arm64.cc
diff --git a/src/arm64/full-codegen-arm64.cc
b/src/arm64/full-codegen-arm64.cc
index
24b10492e1d1eb249b7fe823bd06da3ff82f1516..840634febdf93c79be82197c7156d4e86b2c0e82
100644
--- a/src/arm64/full-codegen-arm64.cc
+++ b/src/arm64/full-codegen-arm64.cc
@@ -2019,16 +2019,14 @@ void
FullCodeGenerator::EmitInlineSmiBinaryOp(BinaryOperation* expr,
__ Ubfx(right, right, kSmiShift, 5);
__ Lsl(result, left, right);
break;
- case Token::SHR: {
- Label right_not_zero;
- __ Cbnz(right, &right_not_zero);
- __ Tbnz(left, kXSignBit, &stub_call);
- __ Bind(&right_not_zero);
+ case Token::SHR:
+ // If `left >>> right` >= 0x80000000, the result is not
representable in a
+ // signed 32-bit smi.
__ Ubfx(right, right, kSmiShift, 5);
- __ Lsr(result, left, right);
- __ Bic(result, result, kSmiShiftMask);
+ __ Lsr(x10, left, right);
+ __ Tbnz(x10, kXSignBit, &stub_call);
+ __ Bic(result, x10, kSmiShiftMask);
break;
- }
case Token::ADD:
__ Adds(x10, left, right);
__ B(vs, &stub_call);
Index: src/arm64/lithium-codegen-arm64.cc
diff --git a/src/arm64/lithium-codegen-arm64.cc
b/src/arm64/lithium-codegen-arm64.cc
index
9b523722d8a7c3b61641af4c71df69d0b862db91..02cfbb8155104c58a64a18f86967d358fac722c6
100644
--- a/src/arm64/lithium-codegen-arm64.cc
+++ b/src/arm64/lithium-codegen-arm64.cc
@@ -4891,13 +4891,12 @@ void LCodeGen::DoShiftI(LShiftI* instr) {
case Token::SAR: __ Asr(result, left, right); break;
case Token::SHL: __ Lsl(result, left, right); break;
case Token::SHR:
- if (instr->can_deopt()) {
- Label right_not_zero;
- __ Cbnz(right, &right_not_zero);
- DeoptimizeIfNegative(left, instr->environment());
- __ Bind(&right_not_zero);
- }
__ Lsr(result, left, right);
+ if (instr->can_deopt()) {
+ // If `left >>> right` >= 0x80000000, the result is not
representable
+ // in a signed 32-bit smi.
+ DeoptimizeIfNegative(result, instr->environment());
+ }
break;
default: UNREACHABLE();
}
@@ -4952,15 +4951,14 @@ void LCodeGen::DoShiftS(LShiftS* instr) {
__ Lsl(result, left, result);
break;
case Token::SHR:
- if (instr->can_deopt()) {
- Label right_not_zero;
- __ Cbnz(right, &right_not_zero);
- DeoptimizeIfNegative(left, instr->environment());
- __ Bind(&right_not_zero);
- }
__ Ubfx(result, right, kSmiShift, 5);
__ Lsr(result, left, result);
__ Bic(result, result, kSmiShiftMask);
+ if (instr->can_deopt()) {
+ // If `left >>> right` >= 0x80000000, the result is not
representable
+ // in a signed 32-bit smi.
+ DeoptimizeIfNegative(result, instr->environment());
+ }
break;
default: UNREACHABLE();
}
Index: test/mjsunit/compiler/shift-shr.js
diff --git a/test/mjsunit/compiler/shift-shr.js
b/test/mjsunit/compiler/shift-shr.js
new file mode 100644
index
0000000000000000000000000000000000000000..a300b2a5c98dd253c521d8a4097de8576af81c24
--- /dev/null
+++ b/test/mjsunit/compiler/shift-shr.js
@@ -0,0 +1,26 @@
+// Copyright 2014 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax --noopt-safe-uint32-operations
+
+// Check the results of `left >>> right`. The result is always unsigned
(and
+// therefore positive).
+function test_shr(left) {
+ var errors = 0;
+
+ for (var i = 1; i < 1024; i++) {
+ var temp = left >>> i;
+ if (temp < 0) {
+ errors++;
+ }
+ }
+
+ return errors;
+}
+
+assertEquals(0, test_shr(1));
+%OptimizeFunctionOnNextCall(test_shr);
+for (var i = 5; i >= -5; i--) {
+ assertEquals(0, test_shr(i));
+}
Index: test/mjsunit/mjsunit.status
diff --git a/test/mjsunit/mjsunit.status b/test/mjsunit/mjsunit.status
index
1305a0b9ee699a0e7a1f0f6ad224323b6a645920..c3254546316ea19785df87f8f3a211366ed65da9
100644
--- a/test/mjsunit/mjsunit.status
+++ b/test/mjsunit/mjsunit.status
@@ -51,6 +51,10 @@
# Issue 3389: deopt_every_n_garbage_collections is unsafe
'regress/regress-2653': [SKIP],
+ # This test relies on --noopt-safe-uint32-operations, which is broken.
See
+ # issue 3487 for details.
+ 'compiler/shift-shr': [SKIP],
+
##############################################################################
# TurboFan compiler failures.
--
--
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/d/optout.
