Bug 1140890 - Make sure the first argument cannot bail in between negative zero removal and creating result in substraction. r=nbp, a=sledru
authorHannes Verschore <hv1989@gmail.com>
Wed, 18 Mar 2015 10:08:37 +0100
changeset 260225 d55fdde73ac8
parent 260224 1cd478c3e0b5
child 260226 5f0e381a7afd
push id723
push userryanvm@gmail.com
push date2015-04-22 14:15 +0000
treeherdermozilla-release@22f8fa3a9273 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnbp, sledru
bugs1140890
milestone38.0
Bug 1140890 - Make sure the first argument cannot bail in between negative zero removal and creating result in substraction. r=nbp, a=sledru
js/src/jit-test/tests/ion/bug1140890.js
js/src/jit/MIR.cpp
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/ion/bug1140890.js
@@ -0,0 +1,11 @@
+function isNegativeZero(x) {
+    return x===0 && (1/x)===-Infinity;
+}
+function f(y) {
+    return -(0 != 1 / y) - -Math.imul(1, !y)
+}
+x = [-0, Infinity]
+for (var k = 0; k < 2; ++k) {
+    assertEq(isNegativeZero(f(x[k])), false);
+}
+
--- a/js/src/jit/MIR.cpp
+++ b/js/src/jit/MIR.cpp
@@ -1890,16 +1890,36 @@ MUrsh::infer(BaselineInspector* inspecto
         return;
     }
 
     specialization_ = MIRType_Int32;
     setResultType(MIRType_Int32);
 }
 
 static inline bool
+CanProduceNegativeZero(MDefinition* def) {
+    // Test if this instruction can produce negative zero even when bailing out
+    // and changing types.
+    switch (def->op()) {
+        case MDefinition::Op_Constant:
+            if (def->type() == MIRType_Double && def->constantValue().toDouble() == -0.0)
+                return true;
+        case MDefinition::Op_BitAnd:
+        case MDefinition::Op_BitOr:
+        case MDefinition::Op_BitXor:
+        case MDefinition::Op_BitNot:
+        case MDefinition::Op_Lsh:
+        case MDefinition::Op_Rsh:
+            return false;
+        default:
+            return true;
+    }
+}
+
+static inline bool
 NeedNegativeZeroCheck(MDefinition* def)
 {
     // Test if all uses have the same semantics for -0 and 0
     for (MUseIterator use = def->usesBegin(); use != def->usesEnd(); use++) {
         if (use->consumer()->isResumePoint())
             continue;
 
         MDefinition* use_def = use->consumer()->toDefinition();
@@ -1917,52 +1937,57 @@ NeedNegativeZeroCheck(MDefinition* def)
             MDefinition* first = use_def->toAdd()->getOperand(0);
             MDefinition* second = use_def->toAdd()->getOperand(1);
             if (first->id() > second->id()) {
                 MDefinition* temp = first;
                 first = second;
                 second = temp;
             }
 
-            if (def == first) {
-                // Negative zero checks can be removed on the first executed
-                // operand only if it is guaranteed the second executed operand
-                // will produce a value other than -0. While the second is
-                // typed as an int32, a bailout taken between execution of the
-                // operands may change that type and cause a -0 to flow to the
-                // second.
-                //
-                // There is no way to test whether there are any bailouts
-                // between execution of the operands, so remove negative
-                // zero checks from the first only if the second's type is
-                // independent from type changes that may occur after bailing.
-                switch (second->op()) {
-                  case MDefinition::Op_Constant:
-                  case MDefinition::Op_BitAnd:
-                  case MDefinition::Op_BitOr:
-                  case MDefinition::Op_BitXor:
-                  case MDefinition::Op_BitNot:
-                  case MDefinition::Op_Lsh:
-                  case MDefinition::Op_Rsh:
-                    break;
-                  default:
-                    return true;
-                }
-            }
+            // Negative zero checks can be removed on the first executed
+            // operand only if it is guaranteed the second executed operand
+            // will produce a value other than -0. While the second is
+            // typed as an int32, a bailout taken between execution of the
+            // operands may change that type and cause a -0 to flow to the
+            // second.
+            //
+            // There is no way to test whether there are any bailouts
+            // between execution of the operands, so remove negative
+            // zero checks from the first only if the second's type is
+            // independent from type changes that may occur after bailing.
+            if (def == first && CanProduceNegativeZero(second))
+                return true;
 
             // The negative zero check can always be removed on the second
             // executed operand; by the time this executes the first will have
             // been evaluated as int32 and the addition's result cannot be -0.
             break;
           }
-          case MDefinition::Op_Sub:
+          case MDefinition::Op_Sub: {
             // If sub is truncating -0 and 0 are observed as the same
             if (use_def->toSub()->isTruncated())
                 break;
+
+            // x + y gives -0, when x is -0 and y is 0
+
+            // We can remove the negative zero check on the rhs, only if we
+            // are sure the lhs isn't negative zero.
+
+            // The lhs is typed as integer (i.e. not -0.0), but it can bailout
+            // and change type. This should be fine if the lhs is executed
+            // first. However if the rhs is executed first, the lhs can bail,
+            // change type and become -0.0 while the rhs has already been
+            // optimized to not make a difference between zero and negative zero.
+            MDefinition* lhs = use_def->toSub()->lhs();
+            MDefinition* rhs = use_def->toSub()->rhs();
+            if (rhs->id() < lhs->id() && CanProduceNegativeZero(lhs))
+                return true;
+
             /* Fall through...  */
+          }
           case MDefinition::Op_StoreElement:
           case MDefinition::Op_StoreElementHole:
           case MDefinition::Op_LoadElement:
           case MDefinition::Op_LoadElementHole:
           case MDefinition::Op_LoadTypedArrayElement:
           case MDefinition::Op_LoadTypedArrayElementHole:
           case MDefinition::Op_CharCodeAt:
           case MDefinition::Op_Mod: