If an alias is dead and so is its aliasee, then globaldce would
[oota-llvm.git] / lib / Transforms / Scalar / GVN.cpp
index 6e0296d2106ffde4e5d034befec1dd5de9186241..727d26555df9abbb849fddc824bc73735e007792 100644 (file)
@@ -25,6 +25,7 @@
 #include "llvm/Value.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DepthFirstIterator.h"
+#include "llvm/ADT/PostOrderIterator.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Statistic.h"
 #include <cstdio>
 using namespace llvm;
 
-STATISTIC(NumGVNInstr, "Number of instructions deleted");
-STATISTIC(NumGVNLoad, "Number of loads deleted");
-STATISTIC(NumGVNPRE, "Number of instructions PRE'd");
+STATISTIC(NumGVNInstr,  "Number of instructions deleted");
+STATISTIC(NumGVNLoad,   "Number of loads deleted");
+STATISTIC(NumGVNPRE,    "Number of instructions PRE'd");
 STATISTIC(NumGVNBlocks, "Number of blocks merged");
-STATISTIC(NumPRELoad, "Number of loads PRE'd");
+STATISTIC(NumPRELoad,   "Number of loads PRE'd");
 
 static cl::opt<bool> EnablePRE("enable-pre",
                                cl::init(true), cl::Hidden);
@@ -109,30 +110,7 @@ namespace {
     }
   
     bool operator!=(const Expression &other) const {
-      if (opcode != other.opcode)
-        return true;
-      else if (opcode == EMPTY || opcode == TOMBSTONE)
-        return false;
-      else if (type != other.type)
-        return true;
-      else if (function != other.function)
-        return true;
-      else if (firstVN != other.firstVN)
-        return true;
-      else if (secondVN != other.secondVN)
-        return true;
-      else if (thirdVN != other.thirdVN)
-        return true;
-      else {
-        if (varargs.size() != other.varargs.size())
-          return true;
-      
-        for (size_t i = 0; i < varargs.size(); ++i)
-          if (varargs[i] != other.varargs[i])
-            return true;
-    
-          return false;
-      }
+      return !(*this == other);
     }
   };
   
@@ -172,6 +150,7 @@ namespace {
       void setMemDep(MemoryDependenceAnalysis* M) { MD = M; }
       void setDomTree(DominatorTree* D) { DT = D; }
       uint32_t getNextUnusedValueNumber() { return nextValueNumber; }
+      void verifyRemoved(const Value *) const;
   };
 }
 
@@ -461,30 +440,19 @@ uint32_t ValueTable::lookup_or_add(Value* V) {
       
       MemDepResult local_dep = MD->getDependency(C);
       
-      if (local_dep.isNone()) {
+      if (!local_dep.isDef() && !local_dep.isNonLocal()) {
         valueNumbering.insert(std::make_pair(V, nextValueNumber));
         return nextValueNumber++;
       }
-      
-      if (Instruction *LocalDepInst = local_dep.getInst()) {
-        if (!isa<CallInst>(LocalDepInst)) {
-          valueNumbering.insert(std::make_pair(V, nextValueNumber));
-          return nextValueNumber++;
-        }
-        
-        CallInst* local_cdep = cast<CallInst>(LocalDepInst);
-        
-        if (local_cdep->getCalledFunction() != C->getCalledFunction() ||
-            local_cdep->getNumOperands() != C->getNumOperands()) {
-          valueNumbering.insert(std::make_pair(V, nextValueNumber));
-          return nextValueNumber++;
-        }
+
+      if (local_dep.isDef()) {
+        CallInst* local_cdep = cast<CallInst>(local_dep.getInst());
         
-        if (!C->getCalledFunction()) { 
+        if (local_cdep->getNumOperands() != C->getNumOperands()) {
           valueNumbering.insert(std::make_pair(V, nextValueNumber));
           return nextValueNumber++;
         }
-        
+          
         for (unsigned i = 1; i < C->getNumOperands(); ++i) {
           uint32_t c_vn = lookup_or_add(C->getOperand(i));
           uint32_t cd_vn = lookup_or_add(local_cdep->getOperand(i));
@@ -498,10 +466,12 @@ uint32_t ValueTable::lookup_or_add(Value* V) {
         valueNumbering.insert(std::make_pair(V, v));
         return v;
       }
-      
 
+      // Non-local case.
       const MemoryDependenceAnalysis::NonLocalDepInfo &deps = 
-        MD->getNonLocalDependency(C);
+        MD->getNonLocalCallDependency(CallSite(C));
+      // FIXME: call/call dependencies for readonly calls should return def, not
+      // clobber!  Move the checking logic to MemDep!
       CallInst* cdep = 0;
       
       // Check to see if we have a single dominating call instruction that is
@@ -514,7 +484,7 @@ uint32_t ValueTable::lookup_or_add(Value* V) {
 
         // We don't handle non-depedencies.  If we already have a call, reject
         // instruction dependencies.
-        if (I->second.isNone() || cdep != 0) {
+        if (I->second.isClobber() || cdep != 0) {
           cdep = 0;
           break;
         }
@@ -535,12 +505,7 @@ uint32_t ValueTable::lookup_or_add(Value* V) {
         return nextValueNumber++;
       }
       
-      if (cdep->getCalledFunction() != C->getCalledFunction() ||
-          cdep->getNumOperands() != C->getNumOperands()) {
-        valueNumbering.insert(std::make_pair(V, nextValueNumber));
-        return nextValueNumber++;
-      }
-      if (!C->getCalledFunction()) { 
+      if (cdep->getNumOperands() != C->getNumOperands()) {
         valueNumbering.insert(std::make_pair(V, nextValueNumber));
         return nextValueNumber++;
       }
@@ -691,8 +656,17 @@ void ValueTable::erase(Value* V) {
   valueNumbering.erase(V);
 }
 
+/// verifyRemoved - Verify that the value is removed from all internal data
+/// structures.
+void ValueTable::verifyRemoved(const Value *V) const {
+  for (DenseMap<Value*, uint32_t>::iterator
+         I = valueNumbering.begin(), E = valueNumbering.end(); I != E; ++I) {
+    assert(I->first != V && "Inst still occurs in value numbering map!");
+  }
+}
+
 //===----------------------------------------------------------------------===//
-//                         GVN Pass
+//                                GVN Pass
 //===----------------------------------------------------------------------===//
 
 namespace {
@@ -736,15 +710,13 @@ namespace {
     // Helper fuctions
     // FIXME: eliminate or document these better
     bool processLoad(LoadInst* L,
-                     DenseMap<Value*, LoadInst*> &lastLoad,
                      SmallVectorImpl<Instruction*> &toErase);
     bool processInstruction(Instruction* I,
-                            DenseMap<Value*, LoadInst*>& lastSeenLoad,
                             SmallVectorImpl<Instruction*> &toErase);
     bool processNonLocalLoad(LoadInst* L,
                              SmallVectorImpl<Instruction*> &toErase);
-    bool processBlock(DomTreeNode* DTN);
-    Value *GetValueForBlock(BasicBlock *BB, LoadInst* orig,
+    bool processBlock(BasicBlock* BB);
+    Value *GetValueForBlock(BasicBlock *BB, Instruction* orig,
                             DenseMap<BasicBlock*, Value*> &Phis,
                             bool top_level = false);
     void dump(DenseMap<uint32_t, Value*>& d);
@@ -754,7 +726,9 @@ namespace {
     bool performPRE(Function& F);
     Value* lookupNumber(BasicBlock* BB, uint32_t num);
     bool mergeBlockIntoPredecessor(BasicBlock* BB);
+    Value* AttemptRedundancyElimination(Instruction* orig, unsigned valno);
     void cleanupGlobalSets();
+    void verifyRemoved(const Instruction *I) const;
   };
   
   char GVN::ID = 0;
@@ -805,7 +779,7 @@ bool GVN::isSafeReplacement(PHINode* p, Instruction* inst) {
 
 /// GetValueForBlock - Get the value to use within the specified basic block.
 /// available values are in Phis.
-Value *GVN::GetValueForBlock(BasicBlock *BB, LoadInst* orig,
+Value *GVN::GetValueForBlock(BasicBlock *BB, Instruction* orig,
                              DenseMap<BasicBlock*, Value*> &Phis,
                              bool top_level) { 
                                  
@@ -818,21 +792,28 @@ Value *GVN::GetValueForBlock(BasicBlock *BB, LoadInst* orig,
   if (!DT->isReachableFromEntry(BB))
     return Phis[BB] = UndefValue::get(orig->getType());
   
-  BasicBlock* singlePred = BB->getSinglePredecessor();
-  if (singlePred) {
-    Value *ret = GetValueForBlock(singlePred, orig, Phis);
+  if (BasicBlock *Pred = BB->getSinglePredecessor()) {
+    Value *ret = GetValueForBlock(Pred, orig, Phis);
     Phis[BB] = ret;
     return ret;
   }
+
+  // Get the number of predecessors of this block so we can reserve space later.
+  // If there is already a PHI in it, use the #preds from it, otherwise count.
+  // Getting it from the PHI is constant time.
+  unsigned NumPreds;
+  if (PHINode *ExistingPN = dyn_cast<PHINode>(BB->begin()))
+    NumPreds = ExistingPN->getNumIncomingValues();
+  else
+    NumPreds = std::distance(pred_begin(BB), pred_end(BB));
   
   // Otherwise, the idom is the loop, so we need to insert a PHI node.  Do so
   // now, then get values to fill in the incoming values for the PHI.
   PHINode *PN = PHINode::Create(orig->getType(), orig->getName()+".rle",
                                 BB->begin());
-  PN->reserveOperandSpace(std::distance(pred_begin(BB), pred_end(BB)));
+  PN->reserveOperandSpace(NumPreds);
   
-  if (Phis.count(BB) == 0)
-    Phis.insert(std::make_pair(BB, PN));
+  Phis.insert(std::make_pair(BB, PN));
   
   // Fill in the incoming values for the block.
   for (pred_iterator PI = pred_begin(BB), E = pred_end(BB); PI != E; ++PI) {
@@ -846,11 +827,17 @@ Value *GVN::GetValueForBlock(BasicBlock *BB, LoadInst* orig,
   Value* v = CollapsePhi(PN);
   if (!v) {
     // Cache our phi construction results
-    phiMap[orig->getPointerOperand()].insert(PN);
+    if (LoadInst* L = dyn_cast<LoadInst>(orig))
+      phiMap[L->getPointerOperand()].insert(PN);
+    else
+      phiMap[orig].insert(PN);
+    
     return PN;
   }
     
   PN->replaceAllUsesWith(v);
+  if (isa<PointerType>(v->getType()))
+    MD->invalidateCachedPointerInfo(v);
 
   for (DenseMap<BasicBlock*, Value*>::iterator I = Phis.begin(),
        E = Phis.end(); I != E; ++I)
@@ -860,6 +847,7 @@ Value *GVN::GetValueForBlock(BasicBlock *BB, LoadInst* orig,
   DEBUG(cerr << "GVN removed: " << *PN);
   MD->removeInstruction(PN);
   PN->eraseFromParent();
+  DEBUG(verifyRemoved(PN));
 
   Phis[BB] = v;
   return v;
@@ -947,17 +935,21 @@ SpeculationFailure:
 bool GVN::processNonLocalLoad(LoadInst *LI,
                               SmallVectorImpl<Instruction*> &toErase) {
   // Find the non-local dependencies of the load.
-  const MemoryDependenceAnalysis::NonLocalDepInfo &deps = 
-    MD->getNonLocalDependency(LI);
-  //DEBUG(cerr << "INVESTIGATING NONLOCAL LOAD: " << deps.size() << *LI);
+  SmallVector<MemoryDependenceAnalysis::NonLocalDepEntry, 64> Deps; 
+  MD->getNonLocalPointerDependency(LI->getOperand(0), true, LI->getParent(),
+                                   Deps);
+  //DEBUG(cerr << "INVESTIGATING NONLOCAL LOAD: " << Deps.size() << *LI);
   
   // If we had to process more than one hundred blocks to find the
   // dependencies, this load isn't worth worrying about.  Optimizing
   // it will be too expensive.
-  if (deps.size() > 100)
+  if (Deps.size() > 100)
+    return false;
+
+  // If we had a phi translation failure, we'll have a single entry which is a
+  // clobber in the current block.  Reject this early.
+  if (Deps.size() == 1 && Deps[0].second.isClobber())
     return false;
-  
-  BasicBlock *EntryBlock = &LI->getParent()->getParent()->getEntryBlock();
   
   // Filter out useless results (non-locals, etc).  Keep track of the blocks
   // where we have a value available in repl, also keep track of whether we see
@@ -966,26 +958,25 @@ bool GVN::processNonLocalLoad(LoadInst *LI,
   SmallVector<std::pair<BasicBlock*, Value*>, 16> ValuesPerBlock;
   SmallVector<BasicBlock*, 16> UnavailableBlocks;
   
-  for (unsigned i = 0, e = deps.size(); i != e; ++i) {
-    BasicBlock *DepBB = deps[i].first;
-    MemDepResult DepInfo = deps[i].second;
+  for (unsigned i = 0, e = Deps.size(); i != e; ++i) {
+    BasicBlock *DepBB = Deps[i].first;
+    MemDepResult DepInfo = Deps[i].second;
     
-    if (DepInfo.isNonLocal()) {
-      // If this is a non-local dependency in the entry block, then we depend on
-      // the value live-in at the start of the function.  We could insert a load
-      // in the entry block to get this, but for now we'll just bail out.
-      if (DepBB == EntryBlock)
-        UnavailableBlocks.push_back(DepBB);
+    if (DepInfo.isClobber()) {
+      UnavailableBlocks.push_back(DepBB);
       continue;
     }
     
-    if (DepInfo.isNone()) {
+    Instruction *DepInst = DepInfo.getInst();
+    
+    // Loading the allocation -> undef.
+    if (isa<AllocationInst>(DepInst)) {
       ValuesPerBlock.push_back(std::make_pair(DepBB, 
                                               UndefValue::get(LI->getType())));
       continue;
     }
   
-    if (StoreInst* S = dyn_cast<StoreInst>(DepInfo.getInst())) {
+    if (StoreInst* S = dyn_cast<StoreInst>(DepInst)) {
       // Reject loads and stores that are to the same address but are of 
       // different types.
       // NOTE: 403.gcc does have this case (e.g. in readonly_fields_p) because
@@ -996,28 +987,13 @@ bool GVN::processNonLocalLoad(LoadInst *LI,
         continue;
       }
       
-      if (S->getPointerOperand() != LI->getPointerOperand() &&
-          VN.getAliasAnalysis()->alias(S->getPointerOperand(), 1,
-                                       LI->getPointerOperand(), 1)
-            != AliasAnalysis::MustAlias) {
-        UnavailableBlocks.push_back(DepBB);
-        continue;
-      }
       ValuesPerBlock.push_back(std::make_pair(DepBB, S->getOperand(0)));
       
-    } else if (LoadInst* LD = dyn_cast<LoadInst>(DepInfo.getInst())) {
+    } else if (LoadInst* LD = dyn_cast<LoadInst>(DepInst)) {
       if (LD->getType() != LI->getType()) {
         UnavailableBlocks.push_back(DepBB);
         continue;
       }
-      
-      if (LD->getPointerOperand() != LI->getPointerOperand() &&
-          VN.getAliasAnalysis()->alias(LD->getPointerOperand(), 1,
-                                       LI->getPointerOperand(), 1)
-          != AliasAnalysis::MustAlias) {
-        UnavailableBlocks.push_back(DepBB);
-        continue;
-      }
       ValuesPerBlock.push_back(std::make_pair(DepBB, LD));
     } else {
       UnavailableBlocks.push_back(DepBB);
@@ -1035,11 +1011,14 @@ bool GVN::processNonLocalLoad(LoadInst *LI,
   if (UnavailableBlocks.empty()) {
     // Use cached PHI construction information from previous runs
     SmallPtrSet<Instruction*, 4> &p = phiMap[LI->getPointerOperand()];
+    // FIXME: What does phiMap do? Are we positive it isn't getting invalidated?
     for (SmallPtrSet<Instruction*, 4>::iterator I = p.begin(), E = p.end();
          I != E; ++I) {
       if ((*I)->getParent() == LI->getParent()) {
         DEBUG(cerr << "GVN REMOVING NONLOCAL LOAD #1: " << *LI);
         LI->replaceAllUsesWith(*I);
+        if (isa<PointerType>((*I)->getType()))
+          MD->invalidateCachedPointerInfo(*I);
         toErase.push_back(LI);
         NumGVNLoad++;
         return true;
@@ -1055,6 +1034,11 @@ bool GVN::processNonLocalLoad(LoadInst *LI,
     // Perform PHI construction.
     Value* v = GetValueForBlock(LI->getParent(), LI, BlockReplValues, true);
     LI->replaceAllUsesWith(v);
+    
+    if (isa<PHINode>(v))
+      v->takeName(LI);
+    if (isa<PointerType>(v->getType()))
+      MD->invalidateCachedPointerInfo(v);
     toErase.push_back(LI);
     NumGVNLoad++;
     return true;
@@ -1148,7 +1132,10 @@ bool GVN::processNonLocalLoad(LoadInst *LI,
   // Perform PHI construction.
   Value* v = GetValueForBlock(LI->getParent(), LI, BlockReplValues, true);
   LI->replaceAllUsesWith(v);
-  v->takeName(LI);
+  if (isa<PHINode>(v))
+    v->takeName(LI);
+  if (isa<PointerType>(v->getType()))
+    MD->invalidateCachedPointerInfo(v);
   toErase.push_back(LI);
   NumPRELoad++;
   return true;
@@ -1156,84 +1143,65 @@ bool GVN::processNonLocalLoad(LoadInst *LI,
 
 /// processLoad - Attempt to eliminate a load, first by eliminating it
 /// locally, and then attempting non-local elimination if that fails.
-bool GVN::processLoad(LoadInst *L, DenseMap<Value*, LoadInst*> &lastLoad,
-                      SmallVectorImpl<Instruction*> &toErase) {
-  if (L->isVolatile()) {
-    lastLoad[L->getPointerOperand()] = L;
+bool GVN::processLoad(LoadInst *L, SmallVectorImpl<Instruction*> &toErase) {
+  if (L->isVolatile())
     return false;
-  }
   
   Value* pointer = L->getPointerOperand();
-  LoadInst*& last = lastLoad[pointer];
-  
+
   // ... to a pointer that has been loaded from before...
-  bool removedNonLocal = false;
   MemDepResult dep = MD->getDependency(L);
-  if (dep.isNonLocal() &&
-      L->getParent() != &L->getParent()->getParent()->getEntryBlock()) {
-    removedNonLocal = processNonLocalLoad(L, toErase);
+  
+  // If the value isn't available, don't do anything!
+  if (dep.isClobber())
+    return false;
+
+  // If it is defined in another block, try harder.
+  if (dep.isNonLocal())
+    return processNonLocalLoad(L, toErase);
+
+  Instruction *DepInst = dep.getInst();
+  if (StoreInst *DepSI = dyn_cast<StoreInst>(DepInst)) {
+    // Only forward substitute stores to loads of the same type.
+    // FIXME: Could do better!
+    if (DepSI->getPointerOperand()->getType() != pointer->getType())
+      return false;
     
-    if (!removedNonLocal)
-      last = L;
+    // Remove it!
+    L->replaceAllUsesWith(DepSI->getOperand(0));
+    if (isa<PointerType>(DepSI->getOperand(0)->getType()))
+      MD->invalidateCachedPointerInfo(DepSI->getOperand(0));
+    toErase.push_back(L);
+    NumGVNLoad++;
+    return true;
+  }
+
+  if (LoadInst *DepLI = dyn_cast<LoadInst>(DepInst)) {
+    // Only forward substitute stores to loads of the same type.
+    // FIXME: Could do better! load i32 -> load i8 -> truncate on little endian.
+    if (DepLI->getType() != L->getType())
+      return false;
     
-    return removedNonLocal;
+    // Remove it!
+    L->replaceAllUsesWith(DepLI);
+    if (isa<PointerType>(DepLI->getType()))
+      MD->invalidateCachedPointerInfo(DepLI);
+    toErase.push_back(L);
+    NumGVNLoad++;
+    return true;
   }
   
-  
-  bool deletedLoad = false;
-  
-  // Walk up the dependency chain until we either find
-  // a dependency we can use, or we can't walk any further
-  while (Instruction *DepInst = dep.getInst()) {
-    // ... that depends on a store ...
-    if (StoreInst* S = dyn_cast<StoreInst>(DepInst)) {
-      if (S->getPointerOperand() == pointer) {
-        // Remove it!
-        L->replaceAllUsesWith(S->getOperand(0));
-        toErase.push_back(L);
-        deletedLoad = true;
-        NumGVNLoad++;
-      }
-      
-      // Whether we removed it or not, we can't
-      // go any further
-      break;
-    } else if (!isa<LoadInst>(DepInst)) {
-      // Only want to handle loads below.
-      break;
-    } else if (!last) {
-      // If we don't depend on a store, and we haven't
-      // been loaded before, bail.
-      break;
-    } else if (DepInst == last) {
-      // Remove it!
-      L->replaceAllUsesWith(last);
-      toErase.push_back(L);
-      deletedLoad = true;
-      NumGVNLoad++;
-      break;
-    } else {
-      dep = MD->getDependencyFrom(L, DepInst, DepInst->getParent());
-    }
-  }
-
   // If this load really doesn't depend on anything, then we must be loading an
   // undef value.  This can happen when loading for a fresh allocation with no
   // intervening stores, for example.
-  if (dep.isNone()) {
-    // If this load depends directly on an allocation, there isn't
-    // anything stored there; therefore, we can optimize this load
-    // to undef.
+  if (isa<AllocationInst>(DepInst)) {
     L->replaceAllUsesWith(UndefValue::get(L->getType()));
     toErase.push_back(L);
-    deletedLoad = true;
     NumGVNLoad++;
+    return true;
   }
 
-  if (!deletedLoad)
-    last = L;
-  
-  return deletedLoad;
+  return false;
 }
 
 Value* GVN::lookupNumber(BasicBlock* BB, uint32_t num) {
@@ -1254,13 +1222,66 @@ Value* GVN::lookupNumber(BasicBlock* BB, uint32_t num) {
   return 0;
 }
 
+/// AttemptRedundancyElimination - If the "fast path" of redundancy elimination
+/// by inheritance from the dominator fails, see if we can perform phi 
+/// construction to eliminate the redundancy.
+Value* GVN::AttemptRedundancyElimination(Instruction* orig, unsigned valno) {
+  BasicBlock* BaseBlock = orig->getParent();
+  
+  SmallPtrSet<BasicBlock*, 4> Visited;
+  SmallVector<BasicBlock*, 8> Stack;
+  Stack.push_back(BaseBlock);
+  
+  DenseMap<BasicBlock*, Value*> Results;
+  
+  // Walk backwards through our predecessors, looking for instances of the
+  // value number we're looking for.  Instances are recorded in the Results
+  // map, which is then used to perform phi construction.
+  while (!Stack.empty()) {
+    BasicBlock* Current = Stack.back();
+    Stack.pop_back();
+    
+    // If we've walked all the way to a proper dominator, then give up. Cases
+    // where the instance is in the dominator will have been caught by the fast
+    // path, and any cases that require phi construction further than this are
+    // probably not worth it anyways.  Note that this is a SIGNIFICANT compile
+    // time improvement.
+    if (DT->properlyDominates(Current, orig->getParent())) return 0;
+    
+    DenseMap<BasicBlock*, ValueNumberScope*>::iterator LA =
+                                                       localAvail.find(Current);
+    if (LA == localAvail.end()) return 0;
+    DenseMap<uint32_t, Value*>::iterator V = LA->second->table.find(valno);
+    
+    if (V != LA->second->table.end()) {
+      // Found an instance, record it.
+      Results.insert(std::make_pair(Current, V->second));
+      continue;
+    }
+    
+    // If we reach the beginning of the function, then give up.
+    if (pred_begin(Current) == pred_end(Current))
+      return 0;
+    
+    for (pred_iterator PI = pred_begin(Current), PE = pred_end(Current);
+         PI != PE; ++PI)
+      if (Visited.insert(*PI))
+        Stack.push_back(*PI);
+  }
+  
+  // If we didn't find instances, give up.  Otherwise, perform phi construction.
+  if (Results.size() == 0)
+    return 0;
+  else
+    return GetValueForBlock(BaseBlock, orig, Results, true);
+}
+
 /// processInstruction - When calculating availability, handle an instruction
 /// by inserting it into the appropriate sets
 bool GVN::processInstruction(Instruction *I,
-                             DenseMap<Value*, LoadInst*> &lastSeenLoad,
                              SmallVectorImpl<Instruction*> &toErase) {
   if (LoadInst* L = dyn_cast<LoadInst>(I)) {
-    bool changed = processLoad(L, lastSeenLoad, toErase);
+    bool changed = processLoad(L, toErase);
     
     if (!changed) {
       unsigned num = VN.lookup_or_add(L);
@@ -1287,10 +1308,13 @@ bool GVN::processInstruction(Instruction *I,
     if (constVal) {
       for (PhiMapType::iterator PI = phiMap.begin(), PE = phiMap.end();
            PI != PE; ++PI)
-        if (PI->second.count(p))
-          PI->second.erase(p);
+        PI->second.erase(p);
         
       p->replaceAllUsesWith(constVal);
+      if (isa<PointerType>(constVal->getType()))
+        MD->invalidateCachedPointerInfo(constVal);
+      VN.erase(p);
+      
       toErase.push_back(p);
     } else {
       localAvail[I->getParent()]->table.insert(std::make_pair(num, I));
@@ -1302,13 +1326,28 @@ bool GVN::processInstruction(Instruction *I,
   } else if (num == nextNum) {
     localAvail[I->getParent()]->table.insert(std::make_pair(num, I));
     
-  // Perform value-number based elimination
+  // Perform fast-path value-number based elimination of values inherited from
+  // dominators.
   } else if (Value* repl = lookupNumber(I->getParent(), num)) {
     // Remove it!
     VN.erase(I);
     I->replaceAllUsesWith(repl);
+    if (isa<PointerType>(repl->getType()))
+      MD->invalidateCachedPointerInfo(repl);
     toErase.push_back(I);
     return true;
+
+#if 0
+  // Perform slow-pathvalue-number based elimination with phi construction.
+  } else if (Value* repl = AttemptRedundancyElimination(I, num)) {
+    // Remove it!
+    VN.erase(I);
+    I->replaceAllUsesWith(repl);
+    if (isa<PointerType>(repl->getType()))
+      MD->invalidateCachedPointerInfo(repl);
+    toErase.push_back(I);
+    return true;
+#endif
   } else {
     localAvail[I->getParent()]->table.insert(std::make_pair(num, I));
   }
@@ -1316,9 +1355,7 @@ bool GVN::processInstruction(Instruction *I,
   return false;
 }
 
-// GVN::runOnFunction - This is the main transformation entry point for a
-// function.
-//
+/// runOnFunction - This is the main transformation entry point for a function.
 bool GVN::runOnFunction(Function& F) {
   MD = &getAnalysis<MemoryDependenceAnalysis>();
   DT = &getAnalysis<DominatorTree>();
@@ -1340,9 +1377,13 @@ bool GVN::runOnFunction(Function& F) {
     changed |= removedBlock;
   }
   
+  unsigned Iteration = 0;
+  
   while (shouldContinue) {
+    DEBUG(cerr << "GVN iteration: " << Iteration << "\n");
     shouldContinue = iterateOnFunction(F);
     changed |= shouldContinue;
+    ++Iteration;
   }
   
   if (EnablePRE) {
@@ -1352,6 +1393,10 @@ bool GVN::runOnFunction(Function& F) {
       changed |= PREChanged;
     }
   }
+  // FIXME: Should perform GVN again after PRE does something.  PRE can move
+  // computations into blocks where they become fully redundant.  Note that
+  // we can't do this until PRE's critical edge splitting updates memdep.
+  // Actually, when this happens, we should just fully integrate PRE into GVN.
 
   cleanupGlobalSets();
 
@@ -1359,10 +1404,11 @@ bool GVN::runOnFunction(Function& F) {
 }
 
 
-bool GVN::processBlock(DomTreeNode* DTN) {
-  BasicBlock* BB = DTN->getBlock();
+bool GVN::processBlock(BasicBlock* BB) {
+  DomTreeNode* DTN = DT->getNode(BB);
+  // FIXME: Kill off toErase by doing erasing eagerly in a helper function (and
+  // incrementing BI before processing an instruction).
   SmallVector<Instruction*, 8> toErase;
-  DenseMap<Value*, LoadInst*> lastSeenLoad;
   bool changed_function = false;
   
   if (DTN->getIDom())
@@ -1373,7 +1419,7 @@ bool GVN::processBlock(DomTreeNode* DTN) {
   
   for (BasicBlock::iterator BI = BB->begin(), BE = BB->end();
        BI != BE;) {
-    changed_function |= processInstruction(BI, lastSeenLoad, toErase);
+    changed_function |= processInstruction(BI, toErase);
     if (toErase.empty()) {
       ++BI;
       continue;
@@ -1392,14 +1438,14 @@ bool GVN::processBlock(DomTreeNode* DTN) {
       DEBUG(cerr << "GVN removed: " << **I);
       MD->removeInstruction(*I);
       (*I)->eraseFromParent();
+      DEBUG(verifyRemoved(*I));
     }
+    toErase.clear();
 
     if (AtStart)
       BI = BB->begin();
     else
       ++BI;
-    
-    toErase.clear();
   }
   
   return changed_function;
@@ -1512,6 +1558,7 @@ bool GVN::performPRE(Function& F) {
       // are not value numbered precisely.
       if (!success) {
         delete PREInstr;
+        DEBUG(verifyRemoved(PREInstr));
         continue;
       }
       
@@ -1536,11 +1583,14 @@ bool GVN::performPRE(Function& F) {
       localAvail[CurrentBlock]->table[valno] = Phi;
       
       CurInst->replaceAllUsesWith(Phi);
+      if (isa<PointerType>(Phi->getType()))
+        MD->invalidateCachedPointerInfo(Phi);
       VN.erase(CurInst);
       
       DEBUG(cerr << "GVN PRE removed: " << *CurInst);
       MD->removeInstruction(CurInst);
       CurInst->eraseFromParent();
+      DEBUG(verifyRemoved(CurInst));
       Changed = true;
     }
   }
@@ -1552,16 +1602,24 @@ bool GVN::performPRE(Function& F) {
   return Changed || toSplit.size();
 }
 
-// iterateOnFunction - Executes one iteration of GVN
+/// iterateOnFunction - Executes one iteration of GVN
 bool GVN::iterateOnFunction(Function &F) {
   cleanupGlobalSets();
 
   // Top-down walk of the dominator tree
   bool changed = false;
+#if 0
+  // Needed for value numbering with phi construction to work.
+  ReversePostOrderTraversal<Function*> RPOT(&F);
+  for (ReversePostOrderTraversal<Function*>::rpo_iterator RI = RPOT.begin(),
+       RE = RPOT.end(); RI != RE; ++RI)
+    changed |= processBlock(*RI);
+#else
   for (df_iterator<DomTreeNode*> DI = df_begin(DT->getRootNode()),
        DE = df_end(DT->getRootNode()); DI != DE; ++DI)
-    changed |= processBlock(*DI);
-  
+    changed |= processBlock(DI->getBlock());
+#endif
+
   return changed;
 }
 
@@ -1574,3 +1632,37 @@ void GVN::cleanupGlobalSets() {
     delete I->second;
   localAvail.clear();
 }
+
+/// verifyRemoved - Verify that the specified instruction does not occur in our
+/// internal data structures.
+void GVN::verifyRemoved(const Instruction *Inst) const {
+  VN.verifyRemoved(Inst);
+
+  // Walk through the PHI map to make sure the instruction isn't hiding in there
+  // somewhere.
+  for (PhiMapType::iterator
+         I = phiMap.begin(), E = phiMap.end(); I != E; ++I) {
+    assert(I->first != Inst && "Inst is still a key in PHI map!");
+
+    for (SmallPtrSet<Instruction*, 4>::iterator
+           II = I->second.begin(), IE = I->second.end(); II != IE; ++II) {
+      assert(*II != Inst && "Inst is still a value in PHI map!");
+    }
+  }
+
+  // Walk through the value number scope to make sure the instruction isn't
+  // ferreted away in it.
+  for (DenseMap<BasicBlock*, ValueNumberScope*>::iterator
+         I = localAvail.begin(), E = localAvail.end(); I != E; ++I) {
+    const ValueNumberScope *VNS = I->second;
+
+    while (VNS) {
+      for (DenseMap<uint32_t, Value*>::iterator
+             II = VNS->table.begin(), IE = VNS->table.end(); II != IE; ++II) {
+        assert(II->second != Inst && "Inst still in value numbering scope!");
+      }
+
+      VNS = VNS->parent;
+    }
+  }
+}