Bug 1054972 - IonMonkey: GVN: Avoid setting UseRemoved flags unnecessarily r=nbp

js/src/jit/IonAnalysis.cpp

@@ -661,7 +661,7 @@ jit::EliminatePhis(MIRGenerator *mir, MIRGraph &graph,
 
             // If the phi is redundant, remove it here.
             if (MDefinition *redundant = IsPhiRedundant(*iter)) {
-                iter->replaceAllUsesWith(redundant);
+                iter->justReplaceAllUsesWith(redundant);
                 iter = block->discardPhiAt(iter);
                 continue;
             }
@@ -699,7 +699,7 @@ jit::EliminatePhis(MIRGenerator *mir, MIRGraph &graph,
                     }
                 }
             }
-            phi->replaceAllUsesWith(redundant);
+            phi->justReplaceAllUsesWith(redundant);
         } else {
             // Otherwise flag them as used.
             phi->setNotUnused();
@@ -1097,7 +1097,7 @@ TypeAnalyzer::replaceRedundantPhi(MPhi *phi)
     MConstant *c = MConstant::New(alloc(), v);
     // The instruction pass will insert the box
     block->insertBefore(*(block->begin()), c);
-    phi->replaceAllUsesWith(c);
+    phi->justReplaceAllUsesWith(c);
 }
 
 bool

js/src/jit/MIR.cpp

@@ -466,13 +466,19 @@ MDefinition::hasLiveDefUses() const
 
 void
 MDefinition::replaceAllUsesWith(MDefinition *dom)
+{
+    for (size_t i = 0, e = numOperands(); i < e; ++i)
+        getOperand(i)->setUseRemovedUnchecked();
+
+    justReplaceAllUsesWith(dom);
+}
+
+void
+MDefinition::justReplaceAllUsesWith(MDefinition *dom)
 {
     JS_ASSERT(dom != nullptr);
     JS_ASSERT(dom != this);
 
-    for (size_t i = 0, e = numOperands(); i < e; i++)
-        getOperand(i)->setUseRemovedUnchecked();
-
     for (MUseIterator i(usesBegin()); i != usesEnd(); i++)
         i->setProducerUnchecked(dom);
     dom->uses_.takeElements(uses_);

js/src/jit/MIR.h

@@ -667,6 +667,9 @@ class MDefinition : public MNode
     }
     void replaceAllUsesWith(MDefinition *dom);
 
+    // Like replaceAllUsesWith, but doesn't set UseRemoved on |this|'s operands.
+    void justReplaceAllUsesWith(MDefinition *dom);
+
     // Mark this instruction as having replaced all uses of ins, as during GVN,
     // returning false if the replacement should not be performed. For use when
     // GVN eliminates instructions which are not equivalent to one another.

js/src/jit/RangeAnalysis.cpp

@@ -272,7 +272,7 @@ RangeAnalysis::removeBetaNodes()
                 MDefinition *op = def->getOperand(0);
                 JitSpew(JitSpew_Range, "Removing beta node %d for %d",
                         def->id(), op->id());
-                def->replaceAllUsesWith(op);
+                def->justReplaceAllUsesWith(op);
                 iter = block->discardDefAt(iter);
             } else {
                 // We only place Beta nodes at the beginning of basic

js/src/jit/ValueNumbering.cpp

@@ -144,14 +144,16 @@ IsDead(const MDefinition *def)
     return !def->hasUses() && DeadIfUnused(def);
 }
 
-// Call MDefinition::replaceAllUsesWith, and add some GVN-specific asserts.
+// Call MDefinition::justReplaceAllUsesWith, and add some GVN-specific asserts.
 static void
 ReplaceAllUsesWith(MDefinition *from, MDefinition *to)
 {
     MOZ_ASSERT(from != to, "GVN shouldn't try to replace a value with itself");
     MOZ_ASSERT(from->type() == to->type(), "Def replacement has different type");
 
-    from->replaceAllUsesWith(to);
+    // We don't need the extra setting of UseRemoved flags that the regular
+    // replaceAllUsesWith does because we do it ourselves.
+    from->justReplaceAllUsesWith(to);
 }
 
 // Test whether succ is a successor of newControl.
@@ -222,7 +224,8 @@ ValueNumberer::deleteDefsRecursively(MDefinition *def)
 // Assuming phi is dead, discard its operands. If an operand which is not
 // dominated by the phi becomes dead, push it to the delete worklist.
 bool
-ValueNumberer::discardPhiOperands(MPhi *phi, const MBasicBlock *phiBlock)
+ValueNumberer::discardPhiOperands(MPhi *phi, const MBasicBlock *phiBlock,
+                                  UseRemovedOption useRemovedOption)
 {
     // MPhi saves operands in a vector so we iterate in reverse.
     for (int o = phi->numOperands() - 1; o >= 0; --o) {
@@ -232,7 +235,8 @@ ValueNumberer::discardPhiOperands(MPhi *phi, const MBasicBlock *phiBlock)
             if (!deadDefs_.append(op))
                 return false;
         } else {
-            op->setUseRemovedUnchecked();
+            if (useRemovedOption == SetUseRemoved)
+                op->setUseRemovedUnchecked();
         }
     }
     return true;
@@ -241,7 +245,8 @@ ValueNumberer::discardPhiOperands(MPhi *phi, const MBasicBlock *phiBlock)
 // Assuming ins is dead, discard its operands. If an operand becomes dead, push
 // it to the delete worklist.
 bool
-ValueNumberer::discardInsOperands(MInstruction *ins)
+ValueNumberer::discardInsOperands(MInstruction *ins,
+                                  UseRemovedOption useRemovedOption)
 {
     for (size_t o = 0, e = ins->numOperands(); o != e; ++o) {
         MDefinition *op = ins->getOperand(o);
@@ -250,14 +255,16 @@ ValueNumberer::discardInsOperands(MInstruction *ins)
             if (!deadDefs_.append(op))
                 return false;
         } else {
-            op->setUseRemovedUnchecked();
+            if (useRemovedOption == SetUseRemoved)
+                op->setUseRemovedUnchecked();
         }
     }
     return true;
 }
 
 bool
-ValueNumberer::deleteDef(MDefinition *def)
+ValueNumberer::deleteDef(MDefinition *def,
+                         UseRemovedOption useRemovedOption)
 {
     JitSpew(JitSpew_GVN, "    Deleting %s%u", def->opName(), def->id());
     MOZ_ASSERT(IsDead(def), "Deleting non-dead definition");
@@ -266,13 +273,13 @@ ValueNumberer::deleteDef(MDefinition *def)
     if (def->isPhi()) {
         MPhi *phi = def->toPhi();
         MBasicBlock *phiBlock = phi->block();
-        if (!discardPhiOperands(phi, phiBlock))
+        if (!discardPhiOperands(phi, phiBlock, useRemovedOption))
              return false;
         MPhiIterator at(phiBlock->phisBegin(phi));
         phiBlock->discardPhiAt(at);
     } else {
         MInstruction *ins = def->toInstruction();
-        if (!discardInsOperands(ins))
+        if (!discardInsOperands(ins, useRemovedOption))
              return false;
         ins->block()->discardIgnoreOperands(ins);
     }
@@ -500,7 +507,7 @@ ValueNumberer::visitDefinition(MDefinition *def)
             if (DeadIfUnused(def)) {
                 // deleteDef should not add anything to the deadDefs, as the
                 // redundant operation should have the same input operands.
-                mozilla::DebugOnly<bool> r = deleteDef(def);
+                mozilla::DebugOnly<bool> r = deleteDef(def, DontSetUseRemoved);
                 MOZ_ASSERT(r, "deleteDef shouldn't have tried to add anything to the worklist, "
                               "so it shouldn't have failed");
                 MOZ_ASSERT(deadDefs_.empty(),

js/src/jit/ValueNumbering.h

@@ -72,10 +72,18 @@ class ValueNumberer
     bool updateAliasAnalysis_;        // Do we care about AliasAnalysis?
     bool dependenciesBroken_;         // Have we broken AliasAnalysis?
 
+    enum UseRemovedOption {
+        DontSetUseRemoved,
+        SetUseRemoved
+    };
+
     bool deleteDefsRecursively(MDefinition *def);
-    bool discardPhiOperands(MPhi *phi, const MBasicBlock *phiBlock);
-    bool discardInsOperands(MInstruction *ins);
-    bool deleteDef(MDefinition *def);
+    bool discardPhiOperands(MPhi *phi, const MBasicBlock *phiBlock,
+                            UseRemovedOption useRemovedOption = SetUseRemoved);
+    bool discardInsOperands(MInstruction *ins,
+                            UseRemovedOption useRemovedOption = SetUseRemoved);
+    bool deleteDef(MDefinition *def,
+                   UseRemovedOption useRemovedOption = SetUseRemoved);
     bool processDeadDefs();
 
     bool removePredecessor(MBasicBlock *block, MBasicBlock *pred);