Bug 845068 - IonMonkey: GVN: Don't make redundant HashSet lookups. r=nbp
authorDan Gohman <sunfish@mozilla.com>
Wed, 16 Jul 2014 10:53:37 -0700
changeset 216420 e0c6d4eb2d4ad55e681b7ba3822c11323767dd47
parent 216419 ed555132e89568a10b95d3bc030ea348f4dc73b9
child 216421 f92f0477237f73dcb6f7c0985e363a75feb9a14c
push id515
push userraliiev@mozilla.com
push dateMon, 06 Oct 2014 12:51:51 +0000
treeherdermozilla-release@267c7a481bef [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnbp
bugs845068
milestone33.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
Bug 845068 - IonMonkey: GVN: Don't make redundant HashSet lookups. r=nbp
js/src/jit/ValueNumbering.cpp
js/src/jit/ValueNumbering.h
--- a/js/src/jit/ValueNumbering.cpp
+++ b/js/src/jit/ValueNumbering.cpp
@@ -114,16 +114,26 @@ ValueNumberer::VisibleValues::forget(con
 
 // Clear all state.
 void
 ValueNumberer::VisibleValues::clear()
 {
     set_.clear();
 }
 
+#ifdef DEBUG
+// Test whether a given value is in the set.
+bool
+ValueNumberer::VisibleValues::has(const MDefinition *def) const
+{
+    Ptr p = set_.lookup(def);
+    return p && *p == def;
+}
+#endif
+
 // Test whether the value would be needed if it had no uses.
 static bool
 DeadIfUnused(const MDefinition *def)
 {
     return !def->isEffectful() && !def->isGuard() && !def->isControlInstruction() &&
            (!def->isInstruction() || !def->toInstruction()->resumePoint());
 }
 
@@ -210,18 +220,17 @@ IsDominatorRefined(MBasicBlock *block)
     return false;
 }
 
 // Delete the given instruction and anything in its use-def subtree which is no
 // longer needed.
 bool
 ValueNumberer::deleteDefsRecursively(MDefinition *def)
 {
-    def->setInWorklist();
-    return deadDefs_.append(def) && processDeadDefs();
+    return deleteDef(def) && processDeadDefs();
 }
 
 // Assuming phi is dead, push each dead operand of phi not dominated by the phi
 // to the delete worklist.
 bool
 ValueNumberer::pushDeadPhiOperands(MPhi *phi, const MBasicBlock *phiBlock)
 {
     for (size_t o = 0, e = phi->numOperands(); o != e; ++o) {
@@ -251,41 +260,50 @@ ValueNumberer::pushDeadInsOperands(MInst
                 return false;
         } else {
            op->setUseRemovedUnchecked();
         }
     }
     return true;
 }
 
+bool
+ValueNumberer::deleteDef(MDefinition *def)
+{
+    IonSpew(IonSpew_GVN, "    Deleting %s%u", def->opName(), def->id());
+    MOZ_ASSERT(IsDead(def), "Deleting non-dead definition");
+    MOZ_ASSERT(!values_.has(def), "Deleting an instruction still in the set");
+
+    if (def->isPhi()) {
+        MPhi *phi = def->toPhi();
+        MBasicBlock *phiBlock = phi->block();
+        if (!pushDeadPhiOperands(phi, phiBlock))
+             return false;
+        MPhiIterator at(phiBlock->phisBegin(phi));
+        phiBlock->discardPhiAt(at);
+    } else {
+        MInstruction *ins = def->toInstruction();
+        if (!pushDeadInsOperands(ins))
+             return false;
+        ins->block()->discard(ins);
+    }
+    return true;
+}
+
 // Recursively delete all the defs on the deadDefs_ worklist.
 bool
 ValueNumberer::processDeadDefs()
 {
     while (!deadDefs_.empty()) {
         MDefinition *def = deadDefs_.popCopy();
-        IonSpew(IonSpew_GVN, "    Deleting %s%u", def->opName(), def->id());
         MOZ_ASSERT(def->isInWorklist(), "Deleting value not on the worklist");
-        MOZ_ASSERT(IsDead(def), "Deleting non-dead definition");
 
         values_.forget(def);
-
-        if (def->isPhi()) {
-            MPhi *phi = def->toPhi();
-            MBasicBlock *phiBlock = phi->block();
-            if (!pushDeadPhiOperands(phi, phiBlock))
-                 return false;
-            MPhiIterator at(phiBlock->phisBegin(phi));
-            phiBlock->discardPhiAt(at);
-        } else {
-            MInstruction *ins = def->toInstruction();
-            if (!pushDeadInsOperands(ins))
-                 return false;
-            ins->block()->discard(ins);
-        }
+        if (!deleteDef(def))
+            return false;
     }
     return true;
 }
 
 // Delete an edge from the CFG. Return true if the block becomes unreachable.
 bool
 ValueNumberer::removePredecessor(MBasicBlock *block, MBasicBlock *pred)
 {
--- a/js/src/jit/ValueNumbering.h
+++ b/js/src/jit/ValueNumbering.h
@@ -47,16 +47,19 @@ class ValueNumberer
         typedef ValueSet::AddPtr AddPtr;
 
         Ptr findLeader(const MDefinition *def) const;
         AddPtr findLeaderForAdd(MDefinition *def);
         bool insert(AddPtr p, MDefinition *def);
         void overwrite(AddPtr p, MDefinition *def);
         void forget(const MDefinition *def);
         void clear();
+#ifdef DEBUG
+        bool has(const MDefinition *def) const;
+#endif
     };
 
     typedef Vector<MBasicBlock *, 4, IonAllocPolicy> BlockWorklist;
     typedef Vector<MDefinition *, 4, IonAllocPolicy> DefWorklist;
 
     MIRGenerator *const mir_;
     MIRGraph &graph_;
     VisibleValues values_;            // Numbered values
@@ -67,16 +70,17 @@ class ValueNumberer
     bool rerun_;                      // Should we run another GVN iteration?
     bool blocksRemoved_;              // Have any blocks been removed?
     bool updateAliasAnalysis_;        // Do we care about AliasAnalysis?
     bool dependenciesBroken_;         // Have we broken AliasAnalysis?
 
     bool deleteDefsRecursively(MDefinition *def);
     bool pushDeadPhiOperands(MPhi *phi, const MBasicBlock *phiBlock);
     bool pushDeadInsOperands(MInstruction *ins);
+    bool deleteDef(MDefinition *def);
     bool processDeadDefs();
 
     bool removePredecessor(MBasicBlock *block, MBasicBlock *pred);
     bool removeBlocksRecursively(MBasicBlock *block, const MBasicBlock *root);
 
     MDefinition *simplified(MDefinition *def) const;
     MDefinition *leader(MDefinition *def);
     bool hasLeader(const MPhi *phi, const MBasicBlock *phiBlock) const;