Revision: 23243
Author: [email protected]
Date: Wed Aug 20 14:58:18 2014 UTC
Log: 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=
[email protected]
Review URL: https://codereview.chromium.org/487913005
http://code.google.com/p/v8/source/detail?r=23243
Added:
/branches/bleeding_edge/test/mjsunit/compiler/shift-shr.js
Modified:
/branches/bleeding_edge/src/arm64/full-codegen-arm64.cc
/branches/bleeding_edge/src/arm64/lithium-codegen-arm64.cc
/branches/bleeding_edge/test/mjsunit/mjsunit.status
=======================================
--- /dev/null
+++ /branches/bleeding_edge/test/mjsunit/compiler/shift-shr.js Wed Aug 20
14:58:18 2014 UTC
@@ -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));
+}
=======================================
--- /branches/bleeding_edge/src/arm64/full-codegen-arm64.cc Mon Aug 18
07:54:19 2014 UTC
+++ /branches/bleeding_edge/src/arm64/full-codegen-arm64.cc Wed Aug 20
14:58:18 2014 UTC
@@ -2019,16 +2019,14 @@
__ 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);
=======================================
--- /branches/bleeding_edge/src/arm64/lithium-codegen-arm64.cc Tue Aug 19
13:32:24 2014 UTC
+++ /branches/bleeding_edge/src/arm64/lithium-codegen-arm64.cc Wed Aug 20
14:58:18 2014 UTC
@@ -4891,13 +4891,12 @@
case Token::SAR: __ Asr(result, left, right); break;
case Token::SHL: __ Lsl(result, left, right); break;
case Token::SHR:
+ __ Lsr(result, left, right);
if (instr->can_deopt()) {
- Label right_not_zero;
- __ Cbnz(right, &right_not_zero);
- DeoptimizeIfNegative(left, instr->environment());
- __ Bind(&right_not_zero);
+ // If `left >>> right` >= 0x80000000, the result is not
representable
+ // in a signed 32-bit smi.
+ DeoptimizeIfNegative(result, instr->environment());
}
- __ Lsr(result, left, right);
break;
default: UNREACHABLE();
}
@@ -4952,15 +4951,14 @@
__ 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();
}
=======================================
--- /branches/bleeding_edge/test/mjsunit/mjsunit.status Thu Aug 14 07:11:44
2014 UTC
+++ /branches/bleeding_edge/test/mjsunit/mjsunit.status Wed Aug 20 14:58:18
2014 UTC
@@ -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.
