Make removal of negative zero checks independent from resume point locations, bug 816786. r=h4writer
authorBrian Hackett <bhackett1024@gmail.com>
Fri, 30 Nov 2012 11:24:36 -0700
changeset 114624 743f9704f2337035e978c65efeeaca63c22eb42a
parent 114623 d188dbaa49b1ec2d7fc560635f5169726cf2b3eb
child 114625 98b22b52150e39f1144c2fa350e93183866ad981
push id18881
push userbhackett@mozilla.com
push dateFri, 30 Nov 2012 18:25:07 +0000
treeherdermozilla-inbound@743f9704f233 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersh4writer
bugs816786
milestone20.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Make removal of negative zero checks independent from resume point locations, bug 816786. r=h4writer
js/src/ion/EdgeCaseAnalysis.cpp
js/src/ion/Ion.cpp
js/src/ion/MIR.cpp
js/src/jit-test/tests/ion/bug816786.js
--- a/js/src/ion/EdgeCaseAnalysis.cpp
+++ b/js/src/ion/EdgeCaseAnalysis.cpp
@@ -20,21 +20,26 @@ using namespace js::ion;
 EdgeCaseAnalysis::EdgeCaseAnalysis(MIRGenerator *mir, MIRGraph &graph)
   : mir(mir), graph(graph)
 {
 }
 
 bool
 EdgeCaseAnalysis::analyzeLate()
 {
+    // Renumber definitions for NeedNegativeZeroCheck under analyzeEdgeCasesBackward.
+    uint32 nextId = 1;
+
     for (ReversePostorderIterator block(graph.rpoBegin()); block != graph.rpoEnd(); block++) {
         if (mir->shouldCancel("Analyze Late (first loop)"))
             return false;
-        for (MDefinitionIterator iter(*block); iter; iter++)
+        for (MDefinitionIterator iter(*block); iter; iter++) {
+            iter->setId(nextId++);
             iter->analyzeEdgeCasesForward();
+        }
     }
 
     for (PostorderIterator block(graph.poBegin()); block != graph.poEnd(); block++) {
         if (mir->shouldCancel("Analyze Late (second loop)"))
             return false;
         for (MInstructionReverseIterator riter(block->rbegin()); riter != block->rend(); riter++)
             riter->analyzeEdgeCasesBackward();
     }
--- a/js/src/ion/Ion.cpp
+++ b/js/src/ion/Ion.cpp
@@ -914,16 +914,19 @@ CompileBackEnd(MIRGenerator *mir)
     if (!EliminateDeadCode(mir, graph))
         return NULL;
     IonSpewPass("DCE");
     AssertGraphCoherency(graph);
 
     if (mir->shouldCancel("DCE"))
         return NULL;
 
+    // Passes after this point must not move instructions; these analyses
+    // depend on knowing the final order in which instructions will execute.
+
     if (js_IonOptions.edgeCaseAnalysis) {
         EdgeCaseAnalysis edgeCaseAnalysis(mir, graph);
         if (!edgeCaseAnalysis.analyzeLate())
             return NULL;
         IonSpewPass("Edge Case Analysis (Late)");
         AssertGraphCoherency(graph);
 
         if (mir->shouldCancel("Edge Case Analysis (Late)"))
--- a/js/src/ion/MIR.cpp
+++ b/js/src/ion/MIR.cpp
@@ -586,53 +586,63 @@ MUrsh::infer(const TypeOracle::BinaryTyp
 }
 
 static inline bool
 NeedNegativeZeroCheck(MDefinition *def)
 {
     // Test if all uses have the same symantic for -0 and 0
     for (MUseIterator use = def->usesBegin(); use != def->usesEnd(); use++) {
         if (use->node()->isResumePoint())
-            return true;
+            continue;
 
         MDefinition *use_def = use->node()->toDefinition();
         switch (use_def->op()) {
           case MDefinition::Op_Add: {
             // x + y gives -0, when both x and y are -0
-            // - When other operand can't produce -0 (i.e. all opcodes, except Mul/Div/ToInt32)
-            //   Remove negative zero check on this operand 
-            // - When both operands can produce -0 (both Mul/Div/ToInt32 opcode)
-            //   We can remove the check eagerly on this operand.
-            MDefinition *operand = use_def->getOperand(0);
-            if (operand == def) {
-                operand = use_def->getOperand(1);
 
-                // Don't remove check when both operands are same definition
-                // As removing it from one operand, will remove it from both.
-                if (operand == def)
-                    return true;
+            // Figure out the order in which the addition's operands will
+            // execute. EdgeCaseAnalysis::analyzeLate has renumbered the MIR
+            // definitions for us so that this just requires comparing ids.
+            MDefinition *first = use_def->getOperand(0);
+            MDefinition *second = use_def->getOperand(1);
+            if (first->id() > second->id()) {
+                MDefinition *temp = first;
+                first = second;
+                second = temp;
             }
 
-            // Check if check is possibly eagerly removed on other operand
-            // and don't remove check eagerly on this operand in that case.
-            if (operand->isMul()) {
-                MMul *mul = operand->toMul();
-                if (!mul->canBeNegativeZero())
+            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;
-            } else if (operand->isDiv()) {
-                MDiv *div = operand->toDiv();
-                if (!div->canBeNegativeZero())
-                    return true;
-            } else if (operand->isToInt32()) {
-                MToInt32 *int32 = operand->toToInt32();
-                if (!int32->canBeNegativeZero())
-                    return true;
-            } else if (operand->isPhi()) {
-                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_StoreElement:
           case MDefinition::Op_StoreElementHole:
           case MDefinition::Op_LoadElement:
           case MDefinition::Op_LoadElementHole:
           case MDefinition::Op_LoadTypedArrayElement:
           case MDefinition::Op_LoadTypedArrayElementHole:
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/ion/bug816786.js
@@ -0,0 +1,38 @@
+var g;
+function test(a, b) {
+
+    g = 0;
+    for(var i=0; i<100; i++) {
+        g += i
+    }
+
+    var t = a*b;
+
+    for(var i=0; i<100; i++) {
+        t = x.y + t;
+        return t;
+    }
+
+}
+
+function negzero(x) {
+  return x===0 && (1/x)===-Infinity;
+}
+
+
+var x = {y:0};
+var a = 0;
+var b = 0;
+for(var i=0; i<58; i++) {
+    var o = test(a, b);
+
+    // Test returns
+    // * 0, if i < 50
+    // * -0, if i >= 50
+    assertEq(negzero(o), i>50);
+
+    if (i == 50) {
+        a = -1
+        x.y = -0
+    }
+}