Bug 1029830 - IonMonkey: GVN: Misc cleanups r=nbp
authorDan Gohman <sunfish@mozilla.com>
Wed, 17 Sep 2014 10:27:24 -0700
changeset 205739 f7f1732c82097f169fc862bb6979527c9ae0ad36
parent 205738 f212bee81c3ce73390ec63c0e6ee02ef9e41f522
child 205740 b0f729f0e0d38840fcdda9296b62a3d598cc62d0
push id49264
push userdgohman@mozilla.com
push dateWed, 17 Sep 2014 17:27:46 +0000
treeherdermozilla-inbound@ce0a75f9481b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnbp
bugs1029830
milestone35.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 1029830 - IonMonkey: GVN: Misc cleanups r=nbp
js/src/jit/IonAnalysis.cpp
js/src/jit/MIR.cpp
js/src/jit/MIR.h
js/src/jit/MIRGraph.cpp
js/src/jit/MIRGraph.h
js/src/jit/ValueNumbering.cpp
js/src/jit/ValueNumbering.h
--- a/js/src/jit/IonAnalysis.cpp
+++ b/js/src/jit/IonAnalysis.cpp
@@ -1866,17 +1866,18 @@ jit::AssertBasicGraphCoherency(MIRGraph 
 #endif
 }
 
 #ifdef DEBUG
 static void
 AssertReversePostorder(MIRGraph &graph)
 {
     // Check that every block is visited after all its predecessors (except backedges).
-    for (ReversePostorderIterator block(graph.rpoBegin()); block != graph.rpoEnd(); block++) {
+    for (ReversePostorderIterator iter(graph.rpoBegin()); iter != graph.rpoEnd(); ++iter) {
+        MBasicBlock *block = *iter;
         JS_ASSERT(!block->isMarked());
 
         for (size_t i = 0; i < block->numPredecessors(); i++) {
             MBasicBlock *pred = block->getPredecessor(i);
             if (!pred->isMarked()) {
                 JS_ASSERT(pred->isLoopBackedge());
                 JS_ASSERT(block->backedge() == pred);
             }
@@ -3149,18 +3150,18 @@ jit::MarkLoopBlocks(MIRGraph &graph, MBa
 #ifdef DEBUG
     for (ReversePostorderIterator i = graph.rpoBegin(), e = graph.rpoEnd(); i != e; ++i)
         MOZ_ASSERT(!i->isMarked(), "Some blocks already marked");
 #endif
 
     MBasicBlock *osrBlock = graph.osrBlock();
     *canOsr = false;
 
-    // The blocks are in RPO; start at the loop backedge, which is marks the
-    // bottom of the loop, and walk up until we get to the header. Loops may be
+    // The blocks are in RPO; start at the loop backedge, which marks the bottom
+    // of the loop, and walk up until we get to the header. Loops may be
     // discontiguous, so we trace predecessors to determine which blocks are
     // actually part of the loop. The backedge is always part of the loop, and
     // so are its predecessors, transitively, up to the loop header or an OSR
     // entry.
     MBasicBlock *backedge = header->backedge();
     backedge->mark();
     size_t numMarked = 1;
     for (PostorderIterator i = graph.poBegin(backedge); ; ++i) {
--- a/js/src/jit/MIR.cpp
+++ b/js/src/jit/MIR.cpp
@@ -348,17 +348,20 @@ MTest::filtersUndefinedOrNull(bool trueB
 }
 
 void
 MDefinition::printOpcode(FILE *fp) const
 {
     PrintOpcodeName(fp, op());
     for (size_t j = 0, e = numOperands(); j < e; j++) {
         fprintf(fp, " ");
-        getOperand(j)->printName(fp);
+        if (getUseFor(j)->hasProducer())
+            getOperand(j)->printName(fp);
+        else
+            fprintf(fp, "(null)");
     }
 }
 
 void
 MDefinition::dump(FILE *fp) const
 {
     printName(fp);
     fprintf(fp, " = ");
--- a/js/src/jit/MIR.h
+++ b/js/src/jit/MIR.h
@@ -57,17 +57,17 @@ MIRType MIRTypeFromValue(const js::Value
     }
     return MIRTypeFromValueType(vp.extractNonDoubleType());
 }
 
 #define MIR_FLAG_LIST(_)                                                        \
     _(InWorklist)                                                               \
     _(EmittedAtUses)                                                            \
     _(Commutative)                                                              \
-    _(Movable)       /* Allow LICM and GVN to move this instruction */          \
+    _(Movable)       /* Allow passes like LICM to move this instruction */      \
     _(Lowered)       /* (Debug only) has a virtual register */                  \
     _(Guard)         /* Not removable if uses == 0 */                           \
                                                                                 \
     /* Keep the flagged instruction in resume points and do not substitute this
      * instruction by an UndefinedValue. This might be used by call inlining
      * when a function argument is not used by the inlined instructions.
      */                                                                         \
     _(ImplicitlyUsed)                                                           \
@@ -184,18 +184,16 @@ typedef InlineList<MUse>::iterator MUseI
 //   MInstruction: an instruction which appears in the IR stream.
 //   MResumePoint: a list of instructions that correspond to the state of the
 //                 interpreter/Baseline stack.
 //
 // Nodes can hold references to MDefinitions. Each MDefinition has a list of
 // nodes holding such a reference (its use chain).
 class MNode : public TempObject
 {
-    friend class MDefinition;
-
   protected:
     MBasicBlock *block_;    // Containing basic block.
 
   public:
     enum Kind {
         Definition,
         ResumePoint
     };
--- a/js/src/jit/MIRGraph.cpp
+++ b/js/src/jit/MIRGraph.cpp
@@ -1445,17 +1445,20 @@ MIRGraph::dump()
 {
     dump(stderr);
 }
 
 void
 MBasicBlock::dump(FILE *fp)
 {
 #ifdef DEBUG
-    fprintf(fp, "block%u:\n", id());
+    fprintf(fp, "block%u:%s%s%s\n", id(),
+            isLoopHeader() ? " (loop header)" : "",
+            unreachable() ? " (unreachable)" : "",
+            isMarked() ? " (marked)" : "");
     if (MResumePoint *resume = entryResumePoint()) {
         resume->dump();
     }
     for (MPhiIterator iter(phisBegin()); iter != phisEnd(); iter++) {
         iter->dump(fp);
     }
     for (MInstructionIterator iter(begin()); iter != end(); iter++) {
         iter->dump(fp);
--- a/js/src/jit/MIRGraph.h
+++ b/js/src/jit/MIRGraph.h
@@ -798,38 +798,39 @@ class MDefinitionIterator
     }
 
     MDefinition *getIns() {
         if (atPhi())
             return *phiIter_;
         return *iter_;
     }
 
-    void next() {
-        if (atPhi())
-            phiIter_++;
-        else
-            iter_++;
-    }
-
     bool more() const {
         return atPhi() || (*iter_) != block_->lastIns();
     }
 
   public:
     explicit MDefinitionIterator(MBasicBlock *block)
       : block_(block),
         phiIter_(block->phisBegin()),
         iter_(block->begin())
     { }
 
+    MDefinitionIterator operator ++() {
+        MOZ_ASSERT(more());
+        if (atPhi())
+            ++phiIter_;
+        else
+            ++iter_;
+        return *this;
+    }
+
     MDefinitionIterator operator ++(int) {
         MDefinitionIterator old(*this);
-        if (more())
-            next();
+        operator++ ();
         return old;
     }
 
     operator bool() const {
         return more();
     }
 
     MDefinition *operator *() {
--- a/js/src/jit/ValueNumbering.cpp
+++ b/js/src/jit/ValueNumbering.cpp
@@ -9,36 +9,36 @@
 #include "jit/AliasAnalysis.h"
 #include "jit/IonAnalysis.h"
 #include "jit/JitSpewer.h"
 #include "jit/MIRGenerator.h"
 
 using namespace js;
 using namespace js::jit;
 
-/**
+/*
  * Some notes on the main algorithm here:
  *  - The SSA identifier id() is the value number. We do replaceAllUsesWith as
  *    we go, so there's always at most one visible value with a given number.
  *
  *  - Consequently, the GVN algorithm is effectively pessimistic. This means it
  *    is not as powerful as an optimistic GVN would be, but it is simpler and
  *    faster.
  *
  *  - We iterate in RPO, so that when visiting a block, we've already optimized
  *    and hashed all values in dominating blocks. With occasional exceptions,
  *    this allows us to do everything in a single pass.
  *
  *  - When we do use multiple passes, we just re-run the algorithm on the whole
  *    graph instead of doing sparse propagation. This is a tradeoff to keep the
  *    algorithm simpler and lighter on inputs that don't have a lot of
  *    interesting unreachable blocks or degenerate loop induction variables, at
- *    the expense of being slower on inputs that do. The loop for this
- *    always terminates, because it only iterates when code is or will be
- *    removed, so eventually it must stop iterating.
+ *    the expense of being slower on inputs that do. The loop for this always
+ *    terminates, because it only iterates when code is or will be removed, so
+ *    eventually it must stop iterating.
  *
  *  - Values are not immediately removed from the hash set when they go out of
  *    scope. Instead, we check for dominance after a lookup. If the dominance
  *    check fails, the value is removed.
  */
 
 HashNumber
 ValueNumberer::VisibleValues::ValueHasher::hash(Lookup ins)
@@ -65,31 +65,31 @@ ValueNumberer::VisibleValues::ValueHashe
 {
     k = newKey;
 }
 
 ValueNumberer::VisibleValues::VisibleValues(TempAllocator &alloc)
   : set_(alloc)
 {}
 
-// Initialize the set in preparation for holding num elements.
+// Initialize the set.
 bool
 ValueNumberer::VisibleValues::init()
 {
     return set_.init();
 }
 
-// Look up the first entry for the given def.
+// Look up the first entry for |def|.
 ValueNumberer::VisibleValues::Ptr
 ValueNumberer::VisibleValues::findLeader(const MDefinition *def) const
 {
     return set_.lookup(def);
 }
 
-// Look up the first entry for the given def.
+// Look up the first entry for |def|.
 ValueNumberer::VisibleValues::AddPtr
 ValueNumberer::VisibleValues::findLeaderForAdd(MDefinition *def)
 {
     return set_.lookupForAdd(def);
 }
 
 // Insert a value into the set.
 bool
@@ -100,51 +100,51 @@ ValueNumberer::VisibleValues::add(AddPtr
 
 // Insert a value onto the set overwriting any existing entry.
 void
 ValueNumberer::VisibleValues::overwrite(AddPtr p, MDefinition *def)
 {
     set_.rekeyInPlace(p, def);
 }
 
-// The given def will be discarded, so remove it from any sets.
+// |def| will be discarded, so remove it from any sets.
 void
 ValueNumberer::VisibleValues::forget(const MDefinition *def)
 {
     Ptr p = set_.lookup(def);
     if (p && *p == def)
         set_.remove(p);
 }
 
 // Clear all state.
 void
 ValueNumberer::VisibleValues::clear()
 {
     set_.clear();
 }
 
 #ifdef DEBUG
-// Test whether a given value is in the set.
+// Test whether |def| 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());
 }
 
-// Test whether the given definition is no longer needed.
+// Test whether |def| is no longer needed.
 static bool
 IsDead(const MDefinition *def)
 {
     return !def->hasUses() && DeadIfUnused(def);
 }
 
 // Call MDefinition::justReplaceAllUsesWith, and add some GVN-specific asserts.
 static void
@@ -153,35 +153,35 @@ ReplaceAllUsesWith(MDefinition *from, MD
     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");
 
     // 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.
+// Test whether |succ| is a successor of |block|.
 static bool
-HasSuccessor(const MControlInstruction *newControl, const MBasicBlock *succ)
+HasSuccessor(const MControlInstruction *block, const MBasicBlock *succ)
 {
-    for (size_t i = 0, e = newControl->numSuccessors(); i != e; ++i) {
-        if (newControl->getSuccessor(i) == succ)
+    for (size_t i = 0, e = block->numSuccessors(); i != e; ++i) {
+        if (block->getSuccessor(i) == succ)
             return true;
     }
     return false;
 }
 
-// Given a block which has had predecessors removed but is still reachable,
-// test whether the block's new dominator will be closer than its old one
-// and whether it will expose potential optimization opportunities.
+// Given a block which has had predecessors removed but is still reachable, test
+// whether the block's new dominator will be closer than its old one and whether
+// it will expose potential optimization opportunities.
 static MBasicBlock *
 ComputeNewDominator(MBasicBlock *block, MBasicBlock *old)
 {
     MBasicBlock *now = block->getPredecessor(0);
-    for (size_t i = 1, e = block->numPredecessors(); i != e; ++i) {
+    for (size_t i = 1, e = block->numPredecessors(); i < e; ++i) {
         MBasicBlock *pred = block->getPredecessor(i);
         // Note that dominators haven't been recomputed yet, so we have to check
         // whether now dominates pred, not block.
         while (!now->dominates(pred)) {
             MBasicBlock *next = now->immediateDominator();
             if (next == old)
                 return old;
             if (next == now) {
@@ -190,19 +190,19 @@ ComputeNewDominator(MBasicBlock *block, 
             }
             now = next;
         }
     }
     MOZ_ASSERT(old != block || old != now, "Missed self-dominating block staying self-dominating");
     return now;
 }
 
-// Given a block which has had predecessors removed but is still reachable,
-// test whether the block's new dominator will be closer than its old one
-// and whether it will expose potential optimization opportunities.
+// Given a block which has had predecessors removed but is still reachable, test
+// whether the block's new dominator will be closer than its old one and whether
+// it will expose potential optimization opportunities.
 static bool
 IsDominatorRefined(MBasicBlock *block)
 {
     MBasicBlock *old = block->immediateDominator();
     MBasicBlock *now = ComputeNewDominator(block, old);
 
     // We've computed block's new dominator. Test whether there are any
     // newly-dominating definitions which look interesting.
@@ -210,26 +210,27 @@ IsDominatorRefined(MBasicBlock *block)
     for (MBasicBlock *i = now; i != old; i = i->immediateDominator()) {
         if (!i->phisEmpty() || *i->begin() != i->lastIns())
             return true;
     }
 
     return false;
 }
 
-// Discard the given instruction and anything in its use-def subtree which is no
-// longer needed.
+// Discard |def| and anything in its use-def subtree which is no longer needed.
 bool
 ValueNumberer::discardDefsRecursively(MDefinition *def)
 {
+    MOZ_ASSERT(deadDefs_.empty(), "deadDefs_ not cleared");
+
     return discardDef(def) && processDeadDefs();
 }
 
-// Assuming phi is dead, release its operands. If an operand which is not
-// dominated by the phi becomes dead, push it to the discard worklist.
+// Assuming |phi| is dead, release its operands. If an operand which is not
+// dominated by |phi| becomes dead, push it to the discard worklist.
 bool
 ValueNumberer::releasePhiOperands(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) {
         MDefinition *op = phi->getOperand(o);
         phi->removeOperand(o);
@@ -239,36 +240,37 @@ ValueNumberer::releasePhiOperands(MPhi *
         } else {
             if (useRemovedOption == SetUseRemoved)
                 op->setUseRemovedUnchecked();
         }
     }
     return true;
 }
 
-// Assuming ins is dead, release its operands. If an operand becomes dead, push
-// it to the discard worklist.
+// Assuming |ins| is dead, release its operands. If an operand becomes dead,
+// push it to the discard worklist.
 bool
 ValueNumberer::releaseInsOperands(MInstruction *ins,
                                   UseRemovedOption useRemovedOption)
 {
-    for (size_t o = 0, e = ins->numOperands(); o != e; ++o) {
+    for (size_t o = 0, e = ins->numOperands(); o < e; ++o) {
         MDefinition *op = ins->getOperand(o);
         ins->releaseOperand(o);
         if (IsDead(op)) {
             if (!deadDefs_.append(op))
                 return false;
         } else {
             if (useRemovedOption == SetUseRemoved)
                 op->setUseRemovedUnchecked();
         }
     }
     return true;
 }
 
+// Discard |def| and mine its operands for any subsequently dead defs.
 bool
 ValueNumberer::discardDef(MDefinition *def,
                          UseRemovedOption useRemovedOption)
 {
     JitSpew(JitSpew_GVN, "    Discarding %s%u", def->opName(), def->id());
     MOZ_ASSERT(IsDead(def), "Discarding non-dead definition");
     MOZ_ASSERT(!values_.has(def), "Discarding an instruction still in the set");
 
@@ -306,33 +308,35 @@ ValueNumberer::processDeadDefs()
 bool
 ValueNumberer::removePredecessor(MBasicBlock *block, MBasicBlock *pred)
 {
     bool isUnreachableLoop = false;
     if (block->isLoopHeader()) {
         if (block->loopPredecessor() == pred) {
             // Discarding the entry into the loop makes the loop unreachable.
             isUnreachableLoop = true;
-            JitSpew(JitSpew_GVN, "    Loop with header block%u is no longer reachable", block->id());
+            JitSpew(JitSpew_GVN, "    Loop with header block%u is no longer reachable",
+                    block->id());
 #ifdef DEBUG
         } else if (block->hasUniqueBackedge() && block->backedge() == pred) {
-            JitSpew(JitSpew_GVN, "    Loop with header block%u is no longer a loop", block->id());
+            JitSpew(JitSpew_GVN, "    Loop with header block%u is no longer a loop",
+                    block->id());
 #endif
         }
     }
 
     // TODO: Removing a predecessor removes operands from phis, and these
-    // operands may become dead. We should detect this and discard them.
-    // In practice though, when this happens, we often end up re-running GVN
-    // for other reasons anyway.
+    // operands may become dead. We should detect this and discard them. In
+    // practice though, when this happens, we often end up re-running GVN for
+    // other reasons anyway.
     block->removePredecessor(pred);
     return block->numPredecessors() == 0 || isUnreachableLoop;
 }
 
-// Discard the given block and any block in its dominator subtree.
+// Discard |start| and any block in its dominator subtree.
 bool
 ValueNumberer::removeBlocksRecursively(MBasicBlock *start, const MBasicBlock *dominatorRoot)
 {
     MOZ_ASSERT(start != graph_.entryBlock(), "Removing normal entry block");
     MOZ_ASSERT(start != graph_.osrBlock(), "Removing OSR entry block");
 
     // Remove this block from its dominator parent's subtree. This is the only
     // immediately-dominated-block information we need to manually update,
@@ -344,35 +348,35 @@ ValueNumberer::removeBlocksRecursively(M
     if (!unreachableBlocks_.append(start))
         return false;
     do {
         MBasicBlock *block = unreachableBlocks_.popCopy();
         if (block->isDead())
             continue;
 
         // If a block is removed while it is on the worklist, skip it.
-        for (size_t i = 0, e = block->numSuccessors(); i != e; ++i) {
+        for (size_t i = 0, e = block->numSuccessors(); i < e; ++i) {
             MBasicBlock *succ = block->getSuccessor(i);
-            if (!succ->isDead()) {
-                if (removePredecessor(succ, block)) {
-                    if (!unreachableBlocks_.append(succ))
-                        return false;
-                } else if (!rerun_) {
-                    if (!remainingBlocks_.append(succ))
-                        return false;
-                }
+            if (succ->isDead())
+                continue;
+            if (removePredecessor(succ, block)) {
+                if (!unreachableBlocks_.append(succ))
+                    return false;
+            } else if (!rerun_) {
+                if (!remainingBlocks_.append(succ))
+                    return false;
             }
         }
 
 #ifdef DEBUG
         JitSpew(JitSpew_GVN, "    Discarding block%u%s%s%s", block->id(),
                 block->isLoopHeader() ? " (loop header)" : "",
                 block->isSplitEdge() ? " (split edge)" : "",
                 block->immediateDominator() == block ? " (dominator root)" : "");
-        for (MDefinitionIterator iter(block); iter; iter++) {
+        for (MDefinitionIterator iter(block); iter; ++iter) {
             MDefinition *def = *iter;
             JitSpew(JitSpew_GVN, "      Discarding %s%u", def->opName(), def->id());
         }
         MControlInstruction *control = block->lastIns();
         JitSpew(JitSpew_GVN, "      Discarding %s%u", control->opName(), control->id());
 #endif
 
         // Keep track of how many blocks within dominatorRoot's tree have been discarded.
@@ -385,25 +389,25 @@ ValueNumberer::removeBlocksRecursively(M
         // we often end up re-running GVN for other reasons anyway (bug 1031412).
         graph_.removeBlockIncludingPhis(block);
         blocksRemoved_ = true;
     } while (!unreachableBlocks_.empty());
 
     return true;
 }
 
-// Return a simplified form of the given definition, if we can.
+// Return a simplified form of |def|, if we can.
 MDefinition *
 ValueNumberer::simplified(MDefinition *def) const
 {
     return def->foldsTo(graph_.alloc());
 }
 
 // If an equivalent and dominating value already exists in the set, return it.
-// Otherwise insert the given definition into the set and return it.
+// Otherwise insert |def| into the set and return it.
 MDefinition *
 ValueNumberer::leader(MDefinition *def)
 {
     // If the value isn't suitable for eliminating, don't bother hashing it. The
     // convention is that congruentTo returns false for node kinds that wish to
     // opt out of redundance elimination.
     // TODO: It'd be nice to clean up that convention (bug 1031406).
     if (!def->isEffectful() && def->congruentTo(def)) {
@@ -424,58 +428,58 @@ ValueNumberer::leader(MDefinition *def)
             if (!values_.add(p, def))
                 return nullptr;
         }
     }
 
     return def;
 }
 
-// Test whether the given phi is dominated by a congruent phi.
+// Test whether |phi| is dominated by a congruent phi.
 bool
 ValueNumberer::hasLeader(const MPhi *phi, const MBasicBlock *phiBlock) const
 {
     if (VisibleValues::Ptr p = values_.findLeader(phi)) {
         const MDefinition *rep = *p;
         return rep != phi && rep->block()->dominates(phiBlock);
     }
     return false;
 }
 
-// Test whether there are any phis in the backedge's loop header which are
-// newly optimizable, as a result of optimizations done inside the loop. This
-// is not a sparse approach, but restarting is rare enough in practice.
-// Termination is ensured by discarding the phi triggering the iteration.
+// Test whether there are any phis in |backedge|'s loop header which are newly
+// optimizable, as a result of optimizations done inside the loop. This is not a
+// sparse approach, but restarting is rare enough in practice. Termination is
+// ensured by discarding the phi triggering the iteration.
 bool
 ValueNumberer::loopHasOptimizablePhi(MBasicBlock *backedge) const
 {
     // Rescan the phis for any that can be simplified, since they may be reading
     // values from backedges.
     MBasicBlock *header = backedge->loopHeaderOfBackedge();
     for (MPhiIterator iter(header->phisBegin()), end(header->phisEnd()); iter != end; ++iter) {
         MPhi *phi = *iter;
         MOZ_ASSERT(phi->hasUses(), "Missed an unused phi");
 
         if (phi->operandIfRedundant() || hasLeader(phi, header))
             return true; // Phi can be simplified.
     }
     return false;
 }
 
-// Visit the given definition.
+// Visit |def|.
 bool
 ValueNumberer::visitDefinition(MDefinition *def)
 {
-    // Look for a simplified form of this def.
+    // Look for a simplified form of |def|.
     MDefinition *sim = simplified(def);
     if (sim != def) {
         if (sim == nullptr)
             return false;
 
-        // If sim doesn't belong to a block, insert it next to def.
+        // If |sim| doesn't belong to a block, insert it next to |def|.
         if (sim->block() == nullptr)
             def->block()->insertAfter(def->toInstruction(), sim->toInstruction());
 
         JitSpew(JitSpew_GVN, "    Folded %s%u to %s%u",
                 def->opName(), def->id(), sim->opName(), sim->id());
         ReplaceAllUsesWith(def, sim);
 
         // The node's foldsTo said |def| can be replaced by |rep|. If |def| is a
@@ -485,17 +489,17 @@ ValueNumberer::visitDefinition(MDefiniti
 
         if (DeadIfUnused(def)) {
             if (!discardDefsRecursively(def))
                 return false;
         }
         def = sim;
     }
 
-    // Look for a dominating def which makes this def redundant.
+    // Look for a dominating def which makes |def| redundant.
     MDefinition *rep = leader(def);
     if (rep != def) {
         if (rep == nullptr)
             return false;
         if (rep->updateForReplacement(def)) {
             JitSpew(JitSpew_GVN,
                     "    Replacing %s%u with %s%u",
                     def->opName(), def->id(), rep->opName(), rep->id());
@@ -527,17 +531,17 @@ ValueNumberer::visitDefinition(MDefiniti
             JitSpew(JitSpew_GVN, "    AliasAnalysis invalidated; will recompute!");
             dependenciesBroken_ = true;
         }
     }
 
     return true;
 }
 
-// Visit the control instruction at the end of the given block.
+// Visit the control instruction at the end of |block|.
 bool
 ValueNumberer::visitControlInstruction(MBasicBlock *block, const MBasicBlock *dominatorRoot)
 {
     // Look for a simplified form of the control instruction.
     MControlInstruction *control = block->lastIns();
     MDefinition *rep = simplified(control);
     if (rep == control)
         return true;
@@ -554,36 +558,36 @@ ValueNumberer::visitControlInstruction(M
     // If the simplification removes any CFG edges, update the CFG and remove
     // any blocks that become dead.
     size_t oldNumSuccs = control->numSuccessors();
     size_t newNumSuccs = newControl->numSuccessors();
     if (newNumSuccs != oldNumSuccs) {
         MOZ_ASSERT(newNumSuccs < oldNumSuccs, "New control instruction has too many successors");
         for (size_t i = 0; i != oldNumSuccs; ++i) {
             MBasicBlock *succ = control->getSuccessor(i);
-            if (!HasSuccessor(newControl, succ)) {
-                if (removePredecessor(succ, block)) {
-                    if (!removeBlocksRecursively(succ, dominatorRoot))
-                        return false;
-                } else if (!rerun_) {
-                    if (!remainingBlocks_.append(succ))
-                        return false;
-                }
+            if (HasSuccessor(newControl, succ))
+                continue;
+            if (removePredecessor(succ, block)) {
+                if (!removeBlocksRecursively(succ, dominatorRoot))
+                    return false;
+            } else if (!rerun_) {
+                if (!remainingBlocks_.append(succ))
+                    return false;
             }
         }
     }
 
     if (!releaseInsOperands(control))
         return false;
     block->discardIgnoreOperands(control);
     block->end(newControl);
     return processDeadDefs();
 }
 
-// Visit all the phis and instructions in the given block.
+// Visit all the phis and instructions |block|.
 bool
 ValueNumberer::visitBlock(MBasicBlock *block, const MBasicBlock *dominatorRoot)
 {
     MOZ_ASSERT(!block->unreachable(), "Blocks marked unreachable during GVN");
     MOZ_ASSERT(!block->isDead(), "Block to visit is already dead");
 
     // Visit the definitions in the block top-down.
     for (MDefinitionIterator iter(block); iter; ) {
@@ -611,33 +615,34 @@ ValueNumberer::visitDominatorTree(MBasic
             uint64_t(dominatorRoot->numDominated()), dominatorRoot->id(),
             dominatorRoot == graph_.entryBlock() ? " (normal entry block)" :
             dominatorRoot == graph_.osrBlock() ? " (OSR entry block)" :
             " (normal entry and OSR entry merge point)");
     MOZ_ASSERT(numBlocksDiscarded_ == 0, "numBlocksDiscarded_ wasn't reset");
     MOZ_ASSERT(dominatorRoot->immediateDominator() == dominatorRoot,
             "root is not a dominator tree root");
 
-    // Visit all blocks dominated by dominatorRoot, in RPO. This has the nice property
-    // that we'll always visit a block before any block it dominates, so we can
-    // make a single pass through the list and see every full redundance.
+    // Visit all blocks dominated by dominatorRoot, in RPO. This has the nice
+    // property that we'll always visit a block before any block it dominates,
+    // so we can make a single pass through the list and see every full
+    // redundance.
     size_t numVisited = 0;
     for (ReversePostorderIterator iter(graph_.rpoBegin(dominatorRoot)); ; ++iter) {
         MOZ_ASSERT(iter != graph_.rpoEnd(), "Inconsistent dominator information");
         MBasicBlock *block = *iter;
         // We're only visiting blocks in dominatorRoot's tree right now.
         if (!dominatorRoot->dominates(block))
             continue;
         // Visit the block!
         if (!visitBlock(block, dominatorRoot))
             return false;
         // If this was the end of a loop, check for optimization in the header.
         if (!rerun_ && block->isLoopBackedge() && loopHasOptimizablePhi(block)) {
             JitSpew(JitSpew_GVN, "    Loop phi in block%u can now be optimized; will re-run GVN!",
-                    block->id());
+                    block->loopHeaderOfBackedge()->id());
             rerun_ = true;
             remainingBlocks_.clear();
         }
         ++numVisited;
         MOZ_ASSERT(numVisited <= dominatorRoot->numDominated() - numBlocksDiscarded_,
                    "Visited blocks too many times");
         if (numVisited >= dominatorRoot->numDominated() - numBlocksDiscarded_)
             break;
@@ -687,21 +692,21 @@ ValueNumberer::ValueNumberer(MIRGenerato
 {}
 
 bool
 ValueNumberer::run(UpdateAliasAnalysisFlag updateAliasAnalysis)
 {
     updateAliasAnalysis_ = updateAliasAnalysis == UpdateAliasAnalysis;
 
     // Initialize the value set. It's tempting to pass in a size here of some
-    // function of graph_.getNumInstructionIds(), however if we start out with
-    // a large capacity, it will be far larger than the actual element count
-    // for most of the pass, so when we remove elements, it would often think
-    // it needs to compact itself. Empirically, just letting the HashTable grow
-    // as needed on its own seems to work pretty well.
+    // function of graph_.getNumInstructionIds(), however if we start out with a
+    // large capacity, it will be far larger than the actual element count for
+    // most of the pass, so when we remove elements, it would often think it
+    // needs to compact itself. Empirically, just letting the HashTable grow as
+    // needed on its own seems to work pretty well.
     if (!values_.init())
         return false;
 
     JitSpew(JitSpew_GVN, "Running GVN on graph (with %llu blocks)",
             uint64_t(graph_.numBlocks()));
 
     // Top level non-sparse iteration loop. If an iteration performs a
     // significant change, such as discarding a block which changes the
@@ -743,17 +748,17 @@ ValueNumberer::run(UpdateAliasAnalysisFl
         JitSpew(JitSpew_GVN, "Re-running GVN on graph (run %d, now with %llu blocks)",
                 runs, uint64_t(graph_.numBlocks()));
         rerun_ = false;
 
         // Enforce an arbitrary iteration limit. This is rarely reached, and
         // isn't even strictly necessary, as the algorithm is guaranteed to
         // terminate on its own in a finite amount of time (since every time we
         // re-run we discard the construct which triggered the re-run), but it
-        // does help avoid slow compile times on pathlogical code.
+        // does help avoid slow compile times on pathological code.
         ++runs;
         if (runs == 6) {
             JitSpew(JitSpew_GVN, "Re-run cutoff reached. Terminating GVN!");
             break;
         }
     }
 
     return true;
--- a/js/src/jit/ValueNumbering.h
+++ b/js/src/jit/ValueNumbering.h
@@ -100,17 +100,17 @@ class ValueNumberer
     bool visitDominatorTree(MBasicBlock *root, size_t *totalNumVisited);
     bool visitGraph();
 
   public:
     ValueNumberer(MIRGenerator *mir, MIRGraph &graph);
 
     enum UpdateAliasAnalysisFlag {
         DontUpdateAliasAnalysis,
-        UpdateAliasAnalysis,
+        UpdateAliasAnalysis
     };
 
     // Optimize the graph, performing expression simplification and
     // canonicalization, eliminating statically fully-redundant expressions,
     // deleting dead instructions, and removing unreachable blocks.
     bool run(UpdateAliasAnalysisFlag updateAliasAnalysis);
 };