[v8-dev] Fixed flooring division by a power of 2, once again... (issue 324403003)

Wed, 11 Jun 2014 06:11:55 -0700 

Reviewers: Yang,

Description:
Fixed flooring division by a power of 2, once again...

Avoid right shifts by zero bits: On ARM it actually means shifting by
32 bits (correctness issue) and on other platforms they are useless
(performance issue). This is fix for the fix in r20544.

BUG=v8:3259
LOG=y

Please review this at https://codereview.chromium.org/324403003/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files (+34, -30 lines):
  M src/arm/lithium-codegen-arm.cc
  M src/arm64/lithium-codegen-arm64.cc
  M src/ia32/lithium-codegen-ia32.cc
  M src/x64/lithium-codegen-x64.cc
  M test/mjsunit/mjsunit.status


Index: src/arm/lithium-codegen-arm.cc
diff --git a/src/arm/lithium-codegen-arm.cc b/src/arm/lithium-codegen-arm.cc
index 800f67b7172f2eac7ae2f5354e01c4d3b6bc06bf..249c22f77bf3176b3e23583f3c29f1c02c274681 100644 
--- a/src/arm/lithium-codegen-arm.cc
+++ b/src/arm/lithium-codegen-arm.cc
@@ -1469,20 +1469,22 @@ void LCodeGen::DoFlooringDivByPowerOf2I(LFlooringDivByPowerOf2I* instr) { 
     DeoptimizeIf(eq, instr->environment());
   }

-  // If the negation could not overflow, simply shifting is OK.
-  if (!instr->hydrogen()->CheckFlag(HValue::kLeftCanBeMinInt)) {
-    __ mov(result, Operand(dividend, ASR, shift));
+  // Dividing by -1 is basically negation, unless we overflow.
+  if (divisor == -1) {
+    if (instr->hydrogen()->CheckFlag(HValue::kLeftCanBeMinInt)) {
+      DeoptimizeIf(vs, instr->environment());
+    }
     return;
   }

-  // Dividing by -1 is basically negation, unless we overflow.
-  if (divisor == -1) {
-    DeoptimizeIf(vs, instr->environment());
+  // If the negation could not overflow, simply shifting is OK.
+  if (!instr->hydrogen()->CheckFlag(HValue::kLeftCanBeMinInt)) {
+    __ mov(result, Operand(result, ASR, shift));
     return;
   }

   __ mov(result, Operand(kMinInt / divisor), LeaveCC, vs);
-  __ mov(result, Operand(dividend, ASR, shift), LeaveCC, vc);
+  __ mov(result, Operand(result, ASR, shift), LeaveCC, vc);
 }


Index: src/arm64/lithium-codegen-arm64.cc
diff --git a/src/arm64/lithium-codegen-arm64.cc b/src/arm64/lithium-codegen-arm64.cc index f301c8b765853ccc204cd04361757f68ac65f6cd..c86efbd3a424b2f01f82e104ba9059a5289d842f 100644 
--- a/src/arm64/lithium-codegen-arm64.cc
+++ b/src/arm64/lithium-codegen-arm64.cc
@@ -3933,19 +3933,21 @@ void LCodeGen::DoFlooringDivByPowerOf2I(LFlooringDivByPowerOf2I* instr) { 
     DeoptimizeIf(eq, instr->environment());
   }

-  // If the negation could not overflow, simply shifting is OK.
-  if (!instr->hydrogen()->CheckFlag(HValue::kLeftCanBeMinInt)) {
-    __ Mov(result, Operand(dividend, ASR, shift));
+  // Dividing by -1 is basically negation, unless we overflow.
+  if (divisor == -1) {
+    if (instr->hydrogen()->CheckFlag(HValue::kLeftCanBeMinInt)) {
+      DeoptimizeIf(vs, instr->environment());
+    }
     return;
   }

-  // Dividing by -1 is basically negation, unless we overflow.
-  if (divisor == -1) {
-    DeoptimizeIf(vs, instr->environment());
+  // If the negation could not overflow, simply shifting is OK.
+  if (!instr->hydrogen()->CheckFlag(HValue::kLeftCanBeMinInt)) {
+    __ Mov(result, Operand(dividend, ASR, shift));
     return;
   }

-  __ Asr(result, dividend, shift);
+  __ Asr(result, result, shift);
   __ Csel(result, result, kMinInt / divisor, vc);
 }

Index: src/ia32/lithium-codegen-ia32.cc
diff --git a/src/ia32/lithium-codegen-ia32.cc b/src/ia32/lithium-codegen-ia32.cc index 791faa2660a3517911c2edb926543349408745c9..1e4f7561be995bdb27f7cf8b69c24b1edd1752f5 100644 
--- a/src/ia32/lithium-codegen-ia32.cc
+++ b/src/ia32/lithium-codegen-ia32.cc
@@ -1353,14 +1353,17 @@ void LCodeGen::DoFlooringDivByPowerOf2I(LFlooringDivByPowerOf2I* instr) { 
     DeoptimizeIf(zero, instr->environment());
   }

-  if (!instr->hydrogen()->CheckFlag(HValue::kLeftCanBeMinInt)) {
-    __ sar(dividend, shift);
+  // Dividing by -1 is basically negation, unless we overflow.
+  if (divisor == -1) {
+    if (instr->hydrogen()->CheckFlag(HValue::kLeftCanBeMinInt)) {
+      DeoptimizeIf(overflow, instr->environment());
+    }
     return;
   }

-  // Dividing by -1 is basically negation, unless we overflow.
-  if (divisor == -1) {
-    DeoptimizeIf(overflow, instr->environment());
+  // If the negation could not overflow, simply shifting is OK.
+  if (!instr->hydrogen()->CheckFlag(HValue::kLeftCanBeMinInt)) {
+    __ sar(dividend, shift);
     return;
   }

Index: src/x64/lithium-codegen-x64.cc
diff --git a/src/x64/lithium-codegen-x64.cc b/src/x64/lithium-codegen-x64.cc
index 41321340e01ca4e302e91291bc54e864e47ee647..809df5d9327435b1022b2bd48a85510f9eccf75f 100644 
--- a/src/x64/lithium-codegen-x64.cc
+++ b/src/x64/lithium-codegen-x64.cc
@@ -1134,16 +1134,17 @@ void LCodeGen::DoFlooringDivByPowerOf2I(LFlooringDivByPowerOf2I* instr) { 
     DeoptimizeIf(zero, instr->environment());
   }

-  // If the negation could not overflow, simply shifting is OK.
-  if (!instr->hydrogen()->CheckFlag(HValue::kLeftCanBeMinInt)) {
-    __ sarl(dividend, Immediate(shift));
+  // Dividing by -1 is basically negation, unless we overflow.
+  if (divisor == -1) {
+    if (instr->hydrogen()->CheckFlag(HValue::kLeftCanBeMinInt)) {
+      DeoptimizeIf(overflow, instr->environment());
+    }
     return;
   }

-  // Note that we could emit branch-free code, but that would need one more
-  // register.
-  if (divisor == -1) {
-    DeoptimizeIf(overflow, instr->environment());
+  // If the negation could not overflow, simply shifting is OK.
+  if (!instr->hydrogen()->CheckFlag(HValue::kLeftCanBeMinInt)) {
+    __ sarl(dividend, Immediate(shift));
     return;
   }

Index: test/mjsunit/mjsunit.status
diff --git a/test/mjsunit/mjsunit.status b/test/mjsunit/mjsunit.status
index 70653f657bc3c6685a4ab59767d102ab7bea53f2..3a6b28136b8f8db2874480a95a6aaf060ae37eb5 100644 
--- a/test/mjsunit/mjsunit.status
+++ b/test/mjsunit/mjsunit.status
@@ -308,10 +308,6 @@
   # Currently always deopt on minus zero
   'math-floor-of-div-minus-zero': [SKIP],

-  # Issue 3259.
-  'math-floor-of-div-nosudiv': [PASS, FAIL],
-  'math-floor-of-div': [PASS, FAIL],
-
############################################################################ 
   # Slow tests.
   'regress/regress-2185-2': [PASS, SLOW],


--
--
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.

Reply via email to