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=
R=ulan@chromium.org

Review URL: https://codereview.chromium.org/487913005

git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@23243 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/arm64/full-codegen-arm64.cc b/src/arm64/full-codegen-arm64.cc
index 24b1049..840634f 100644 (file)
--- 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);

diff --git a/src/arm64/lithium-codegen-arm64.cc b/src/arm64/lithium-codegen-arm64.cc
index 9b52372..02cfbb8 100644 (file)
--- 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:
+        __ 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 @@ 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();
     }

diff --git a/test/mjsunit/compiler/shift-shr.js b/test/mjsunit/compiler/shift-shr.js
new file mode 100644 (file)
index 0000000..a300b2a
--- /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));
+}

diff --git a/test/mjsunit/mjsunit.status b/test/mjsunit/mjsunit.status
index 1305a0b..c325454 100644 (file)
--- 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.