Fix an issue that Chris noticed, where local PRE was not properly instantiating
[oota-llvm.git] / lib / Transforms / Scalar / GVN.cpp
index bf2253fbd086c48d91b04351b492d2d10fa31ce2..039d52029aac9d88da399ae802cbf48d6d2a00fd 100644 (file)
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Transforms/Utils/BasicBlockUtils.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(NumGVNBlocks, "Number of blocks merged");
 
 static cl::opt<bool> EnablePRE("enable-pre",
-                                cl::init(false), cl::Hidden);
+                               cl::init(true), cl::Hidden);
 
 //===----------------------------------------------------------------------===//
 //                         ValueTable Class
@@ -164,8 +166,10 @@ namespace {
       void erase(Value* v);
       unsigned size();
       void setAliasAnalysis(AliasAnalysis* A) { AA = A; }
+      AliasAnalysis *getAliasAnalysis() const { return AA; }
       void setMemDep(MemoryDependenceAnalysis* M) { MD = M; }
       void setDomTree(DominatorTree* D) { DT = D; }
+      uint32_t getNextUnusedValueNumber() { return nextValueNumber; }
   };
 }
 
@@ -453,66 +457,75 @@ uint32_t ValueTable::lookup_or_add(Value* V) {
         return nextValueNumber++;
       }
       
-      Instruction* local_dep = MD->getDependency(C);
+      MemDepResult local_dep = MD->getDependency(C);
       
-      if (local_dep == MemoryDependenceAnalysis::None) {
+      if (local_dep.isNone()) {
         valueNumbering.insert(std::make_pair(V, nextValueNumber));
         return nextValueNumber++;
-      } else if (local_dep != MemoryDependenceAnalysis::NonLocal) {
-        if (!isa<CallInst>(local_dep)) {
+      }
+      
+      if (Instruction *LocalDepInst = local_dep.getInst()) {
+        if (!isa<CallInst>(LocalDepInst)) {
           valueNumbering.insert(std::make_pair(V, nextValueNumber));
           return nextValueNumber++;
         }
         
-        CallInst* local_cdep = cast<CallInst>(local_dep);
+        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++;
-        } else if (!C->getCalledFunction()) { 
+        }
+        
+        if (!C->getCalledFunction()) { 
           valueNumbering.insert(std::make_pair(V, nextValueNumber));
           return nextValueNumber++;
-        } else {
-          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));
-            if (c_vn != cd_vn) {
-              valueNumbering.insert(std::make_pair(V, nextValueNumber));
-              return nextValueNumber++;
-            }
-          }
+        }
         
-          uint32_t v = lookup_or_add(local_cdep);
-          valueNumbering.insert(std::make_pair(V, v));
-          return v;
+        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));
+          if (c_vn != cd_vn) {
+            valueNumbering.insert(std::make_pair(V, nextValueNumber));
+            return nextValueNumber++;
+          }
         }
-      }
       
+        uint32_t v = lookup_or_add(local_cdep);
+        valueNumbering.insert(std::make_pair(V, v));
+        return v;
+      }
       
-      DenseMap<BasicBlock*, Value*> deps;
-      MD->getNonLocalDependency(C, deps);
+
+      const MemoryDependenceAnalysis::NonLocalDepInfo &deps = 
+        MD->getNonLocalDependency(C);
       CallInst* cdep = 0;
       
-      for (DenseMap<BasicBlock*, Value*>::iterator I = deps.begin(),
-           E = deps.end(); I != E; ++I) {
-        if (I->second == MemoryDependenceAnalysis::None) {
-          valueNumbering.insert(std::make_pair(V, nextValueNumber));
+      // Check to see if we have a single dominating call instruction that is
+      // identical to C.
+      for (unsigned i = 0, e = deps.size(); i != e; ++i) {
+        const MemoryDependenceAnalysis::NonLocalDepEntry *I = &deps[i];
+        // Ignore non-local dependencies.
+        if (I->second.isNonLocal())
+          continue;
 
-          return nextValueNumber++;
-        } else if (I->second != MemoryDependenceAnalysis::NonLocal) {
-          if (DT->properlyDominates(I->first, C->getParent())) {
-            if (CallInst* CD = dyn_cast<CallInst>(I->second))
-              cdep = CD;
-            else {
-              valueNumbering.insert(std::make_pair(V, nextValueNumber));
-              return nextValueNumber++;
-            }
-          } else {
-            valueNumbering.insert(std::make_pair(V, nextValueNumber));
-            return nextValueNumber++;
-          }
+        // We don't handle non-depedencies.  If we already have a call, reject
+        // instruction dependencies.
+        if (I->second.isNone() || cdep != 0) {
+          cdep = 0;
+          break;
+        }
+        
+        CallInst *NonLocalDepCall = dyn_cast<CallInst>(I->second.getInst());
+        // FIXME: All duplicated with non-local case.
+        if (NonLocalDepCall && DT->properlyDominates(I->first, C->getParent())){
+          cdep = NonLocalDepCall;
+          continue;
         }
+        
+        cdep = 0;
+        break;
       }
       
       if (!cdep) {
@@ -524,24 +537,24 @@ uint32_t ValueTable::lookup_or_add(Value* V) {
           cdep->getNumOperands() != C->getNumOperands()) {
         valueNumbering.insert(std::make_pair(V, nextValueNumber));
         return nextValueNumber++;
-      } else if (!C->getCalledFunction()) { 
+      }
+      if (!C->getCalledFunction()) { 
         valueNumbering.insert(std::make_pair(V, nextValueNumber));
         return nextValueNumber++;
-      } else {
-        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(cdep->getOperand(i));
-          if (c_vn != cd_vn) {
-            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(cdep->getOperand(i));
+        if (c_vn != cd_vn) {
+          valueNumbering.insert(std::make_pair(V, nextValueNumber));
+          return nextValueNumber++;
         }
-        
-        uint32_t v = lookup_or_add(cdep);
-        valueNumbering.insert(std::make_pair(V, v));
-        return v;
       }
       
+      uint32_t v = lookup_or_add(cdep);
+      valueNumbering.insert(std::make_pair(V, v));
+      return v;
+      
     } else {
       valueNumbering.insert(std::make_pair(V, nextValueNumber));
       return nextValueNumber++;
@@ -680,18 +693,6 @@ void ValueTable::erase(Value* V) {
 //                         GVN Pass
 //===----------------------------------------------------------------------===//
 
-namespace llvm {
-  template<> struct DenseMapInfo<uint32_t> {
-    static inline uint32_t getEmptyKey() { return ~0; }
-    static inline uint32_t getTombstoneKey() { return ~0 - 1; }
-    static unsigned getHashValue(const uint32_t& Val) { return Val * 37; }
-    static bool isPod() { return true; }
-    static bool isEqual(const uint32_t& LHS, const uint32_t& RHS) {
-      return LHS == RHS;
-    }
-  };
-}
-
 namespace {
   struct VISIBILITY_HIDDEN ValueNumberScope {
     ValueNumberScope* parent;
@@ -707,9 +708,12 @@ namespace {
     bool runOnFunction(Function &F);
   public:
     static char ID; // Pass identification, replacement for typeid
-    GVN() : FunctionPass((intptr_t)&ID) { }
+    GVN() : FunctionPass(&ID) { }
 
   private:
+    MemoryDependenceAnalysis *MD;
+    DominatorTree *DT;
+
     ValueTable VN;
     DenseMap<BasicBlock*, ValueNumberScope*> localAvail;
     
@@ -719,12 +723,12 @@ namespace {
     
     // This transformation requires dominator postdominator info
     virtual void getAnalysisUsage(AnalysisUsage &AU) const {
-      AU.setPreservesCFG();
       AU.addRequired<DominatorTree>();
       AU.addRequired<MemoryDependenceAnalysis>();
       AU.addRequired<AliasAnalysis>();
+      
+      AU.addPreserved<DominatorTree>();
       AU.addPreserved<AliasAnalysis>();
-      AU.addPreserved<MemoryDependenceAnalysis>();
     }
   
     // Helper fuctions
@@ -747,6 +751,8 @@ namespace {
     bool isSafeReplacement(PHINode* p, Instruction* inst);
     bool performPRE(Function& F);
     Value* lookupNumber(BasicBlock* BB, uint32_t num);
+    bool mergeBlockIntoPredecessor(BasicBlock* BB);
+    void cleanupGlobalSets();
   };
   
   char GVN::ID = 0;
@@ -769,16 +775,14 @@ void GVN::dump(DenseMap<uint32_t, Value*>& d) {
 }
 
 Value* GVN::CollapsePhi(PHINode* p) {
-  DominatorTree &DT = getAnalysis<DominatorTree>();
   Value* constVal = p->hasConstantValue();
-  
   if (!constVal) return 0;
   
   Instruction* inst = dyn_cast<Instruction>(constVal);
   if (!inst)
     return constVal;
     
-  if (DT.dominates(inst, p))
+  if (DT->dominates(inst, p))
     if (isSafeReplacement(p, inst))
       return inst;
   return 0;
@@ -807,6 +811,11 @@ Value *GVN::GetValueForBlock(BasicBlock *BB, LoadInst* orig,
   DenseMap<BasicBlock*, Value*>::iterator V = Phis.find(BB);
   if (V != Phis.end() && !top_level) return V->second;
   
+  // If the block is unreachable, just return undef, since this path
+  // can't actually occur at runtime.
+  if (!DT->isReachableFromEntry(BB))
+    return Phis[BB] = UndefValue::get(orig->getType());
+  
   BasicBlock* singlePred = BB->getSinglePredecessor();
   if (singlePred) {
     Value *ret = GetValueForBlock(singlePred, orig, Phis);
@@ -829,8 +838,7 @@ Value *GVN::GetValueForBlock(BasicBlock *BB, LoadInst* orig,
     PN->addIncoming(val, *PI);
   }
   
-  AliasAnalysis& AA = getAnalysis<AliasAnalysis>();
-  AA.copyValue(orig, PN);
+  VN.getAliasAnalysis()->copyValue(orig, PN);
   
   // Attempt to collapse PHI nodes that are trivially redundant
   Value* v = CollapsePhi(PN);
@@ -840,9 +848,6 @@ Value *GVN::GetValueForBlock(BasicBlock *BB, LoadInst* orig,
     return PN;
   }
     
-  MemoryDependenceAnalysis& MD = getAnalysis<MemoryDependenceAnalysis>();
-
-  MD.removeInstruction(PN);
   PN->replaceAllUsesWith(v);
 
   for (DenseMap<BasicBlock*, Value*>::iterator I = Phis.begin(),
@@ -850,6 +855,8 @@ Value *GVN::GetValueForBlock(BasicBlock *BB, LoadInst* orig,
     if (I->second == PN)
       I->second = v;
 
+  DEBUG(cerr << "GVN removed: " << *PN);
+  MD->removeInstruction(PN);
   PN->eraseFromParent();
 
   Phis[BB] = v;
@@ -860,31 +867,78 @@ Value *GVN::GetValueForBlock(BasicBlock *BB, LoadInst* orig,
 /// non-local by performing PHI construction.
 bool GVN::processNonLocalLoad(LoadInst* L,
                               SmallVectorImpl<Instruction*> &toErase) {
-  MemoryDependenceAnalysis& MD = getAnalysis<MemoryDependenceAnalysis>();
-  
   // Find the non-local dependencies of the load
-  DenseMap<BasicBlock*, Value*> deps;
-  MD.getNonLocalDependency(L, deps);
+  const MemoryDependenceAnalysis::NonLocalDepInfo &deps = 
+    MD->getNonLocalDependency(L);
+  DEBUG(cerr << "INVESTIGATING NONLOCAL LOAD: " << deps.size() << *L);
+#if 0
+  DEBUG(for (unsigned i = 0, e = deps.size(); i != e; ++i) {
+        cerr << "  " << deps[i].first->getName();
+          if (Instruction *I = deps[i].second.getInst())
+        cerr << *I;
+        else
+        cerr << "\n";
+        });
+#endif          
+  
+  // 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)
+    return false;
+  
+  BasicBlock *EntryBlock = &L->getParent()->getParent()->getEntryBlock();
   
   DenseMap<BasicBlock*, Value*> repl;
   
   // Filter out useless results (non-locals, etc)
-  for (DenseMap<BasicBlock*, Value*>::iterator I = deps.begin(), E = deps.end();
-       I != E; ++I) {
-    if (I->second == MemoryDependenceAnalysis::None)
-      return false;
-  
-    if (I->second == MemoryDependenceAnalysis::NonLocal)
+  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.
+      //
+      // FIXME: Consider emitting a load in the entry block to catch this case!
+      // Tricky part is to sink so that it doesn't execute in places where it
+      // isn't needed.
+      if (DepBB == EntryBlock)
+        return false;
       continue;
+    }
+    
+    if (DepInfo.isNone()) {
+      repl[DepBB] = UndefValue::get(L->getType());
+      continue;
+    }
   
-    if (StoreInst* S = dyn_cast<StoreInst>(I->second)) {
-      if (S->getPointerOperand() != L->getPointerOperand())
+    if (StoreInst* S = dyn_cast<StoreInst>(DepInfo.getInst())) {
+      // 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
+      // of bitfield access, it would be interesting to optimize for it at some
+      // point.
+      if (S->getOperand(0)->getType() != L->getType())
         return false;
-      repl[I->first] = S->getOperand(0);
-    } else if (LoadInst* LD = dyn_cast<LoadInst>(I->second)) {
-      if (LD->getPointerOperand() != L->getPointerOperand())
+      
+      if (S->getPointerOperand() != L->getPointerOperand() &&
+          VN.getAliasAnalysis()->alias(S->getPointerOperand(), 1,
+                                       L->getPointerOperand(), 1)
+            != AliasAnalysis::MustAlias)
+        return false;
+      repl[DepBB] = S->getOperand(0);
+    } else if (LoadInst* LD = dyn_cast<LoadInst>(DepInfo.getInst())) {
+      if (LD->getType() != L->getType())
         return false;
-      repl[I->first] = LD;
+      
+      if (LD->getPointerOperand() != L->getPointerOperand() &&
+          VN.getAliasAnalysis()->alias(LD->getPointerOperand(), 1,
+                                       L->getPointerOperand(), 1)
+            != AliasAnalysis::MustAlias)
+        return false;
+      repl[DepBB] = LD;
     } else {
       return false;
     }
@@ -895,7 +949,6 @@ bool GVN::processNonLocalLoad(LoadInst* L,
   for (SmallPtrSet<Instruction*, 4>::iterator I = p.begin(), E = p.end();
        I != E; ++I) {
     if ((*I)->getParent() == L->getParent()) {
-      MD.removeInstruction(L);
       L->replaceAllUsesWith(*I);
       toErase.push_back(L);
       NumGVNLoad++;
@@ -904,16 +957,15 @@ bool GVN::processNonLocalLoad(LoadInst* L,
     
     repl.insert(std::make_pair((*I)->getParent(), *I));
   }
-  
+
+  DEBUG(cerr << "GVN REMOVING NONLOCAL LOAD: " << *L);
+
   // Perform PHI construction
   SmallPtrSet<BasicBlock*, 4> visited;
   Value* v = GetValueForBlock(L->getParent(), L, repl, true);
-  
-  MD.removeInstruction(L);
   L->replaceAllUsesWith(v);
   toErase.push_back(L);
   NumGVNLoad++;
-
   return true;
 }
 
@@ -930,10 +982,9 @@ bool GVN::processLoad(LoadInst *L, DenseMap<Value*, LoadInst*> &lastLoad,
   LoadInst*& last = lastLoad[pointer];
   
   // ... to a pointer that has been loaded from before...
-  MemoryDependenceAnalysis& MD = getAnalysis<MemoryDependenceAnalysis>();
   bool removedNonLocal = false;
-  Instruction* dep = MD.getDependency(L);
-  if (dep == MemoryDependenceAnalysis::NonLocal &&
+  MemDepResult dep = MD->getDependency(L);
+  if (dep.isNonLocal() &&
       L->getParent() != &L->getParent()->getParent()->getEntryBlock()) {
     removedNonLocal = processNonLocalLoad(L, toErase);
     
@@ -948,15 +999,11 @@ bool GVN::processLoad(LoadInst *L, DenseMap<Value*, LoadInst*> &lastLoad,
   
   // Walk up the dependency chain until we either find
   // a dependency we can use, or we can't walk any further
-  while (dep != MemoryDependenceAnalysis::None &&
-         dep != MemoryDependenceAnalysis::NonLocal &&
-         (isa<LoadInst>(dep) || isa<StoreInst>(dep))) {
+  while (Instruction *DepInst = dep.getInst()) {
     // ... that depends on a store ...
-    if (StoreInst* S = dyn_cast<StoreInst>(dep)) {
+    if (StoreInst* S = dyn_cast<StoreInst>(DepInst)) {
       if (S->getPointerOperand() == pointer) {
         // Remove it!
-        MD.removeInstruction(L);
-        
         L->replaceAllUsesWith(S->getOperand(0));
         toErase.push_back(L);
         deletedLoad = true;
@@ -966,50 +1013,36 @@ bool GVN::processLoad(LoadInst *L, DenseMap<Value*, LoadInst*> &lastLoad,
       // 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 (dep == last) {
+    } else if (DepInst == last) {
       // Remove it!
-      MD.removeInstruction(L);
-      
       L->replaceAllUsesWith(last);
       toErase.push_back(L);
       deletedLoad = true;
       NumGVNLoad++;
-        
       break;
     } else {
-      dep = MD.getDependency(L, dep);
+      dep = MD->getDependencyFrom(L, DepInst, DepInst->getParent());
     }
   }
 
-  if (dep != MemoryDependenceAnalysis::None &&
-      dep != MemoryDependenceAnalysis::NonLocal &&
-      isa<AllocationInst>(dep)) {
-    // Check that this load is actually from the
-    // allocation we found
-    Value* v = L->getOperand(0);
-    while (true) {
-      if (BitCastInst *BC = dyn_cast<BitCastInst>(v))
-        v = BC->getOperand(0);
-      else if (GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(v))
-        v = GEP->getOperand(0);
-      else
-        break;
-    }
-    if (v == dep) {
-      // If this load depends directly on an allocation, there isn't
-      // anything stored there; therefore, we can optimize this load
-      // to undef.
-      MD.removeInstruction(L);
-
-      L->replaceAllUsesWith(UndefValue::get(L->getType()));
-      toErase.push_back(L);
-      deletedLoad = true;
-      NumGVNLoad++;
-    }
+  // 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.
+    L->replaceAllUsesWith(UndefValue::get(L->getType()));
+    toErase.push_back(L);
+    deletedLoad = true;
+    NumGVNLoad++;
   }
 
   if (!deletedLoad)
@@ -1019,7 +1052,11 @@ bool GVN::processLoad(LoadInst *L, DenseMap<Value*, LoadInst*> &lastLoad,
 }
 
 Value* GVN::lookupNumber(BasicBlock* BB, uint32_t num) {
-  ValueNumberScope* locals = localAvail[BB];
+  DenseMap<BasicBlock*, ValueNumberScope*>::iterator I = localAvail.find(BB);
+  if (I == localAvail.end())
+    return 0;
+  
+  ValueNumberScope* locals = I->second;
   
   while (locals) {
     DenseMap<uint32_t, Value*>::iterator I = locals->table.find(num);
@@ -1048,11 +1085,12 @@ bool GVN::processInstruction(Instruction *I,
     return changed;
   }
   
+  uint32_t nextNum = VN.getNextUnusedValueNumber();
   unsigned num = VN.lookup_or_add(I);
   
   // Allocations are always uniquely numbered, so we can save time and memory
   // by fast failing them.
-  if (isa<AllocationInst>(I)) {
+  if (isa<AllocationInst>(I) || isa<TerminatorInst>(I)) {
     localAvail[I->getParent()]->table.insert(std::make_pair(num, I));
     return false;
   }
@@ -1072,17 +1110,21 @@ bool GVN::processInstruction(Instruction *I,
     } else {
       localAvail[I->getParent()]->table.insert(std::make_pair(num, I));
     }
+  
+  // If the number we were assigned was a brand new VN, then we don't
+  // need to do a lookup to see if the number already exists
+  // somewhere in the domtree: it can't!
+  } else if (num == nextNum) {
+    localAvail[I->getParent()]->table.insert(std::make_pair(num, I));
+    
   // Perform value-number based elimination
   } else if (Value* repl = lookupNumber(I->getParent(), num)) {
     // Remove it!
-    MemoryDependenceAnalysis& MD = getAnalysis<MemoryDependenceAnalysis>();
-    MD.removeInstruction(I);
-    
     VN.erase(I);
     I->replaceAllUsesWith(repl);
     toErase.push_back(I);
     return true;
-  } else if (!I->isTerminator()) {
+  } else {
     localAvail[I->getParent()]->table.insert(std::make_pair(num, I));
   }
   
@@ -1093,25 +1135,47 @@ bool GVN::processInstruction(Instruction *I,
 // function.
 //
 bool GVN::runOnFunction(Function& F) {
+  MD = &getAnalysis<MemoryDependenceAnalysis>();
+  DT = &getAnalysis<DominatorTree>();
   VN.setAliasAnalysis(&getAnalysis<AliasAnalysis>());
-  VN.setMemDep(&getAnalysis<MemoryDependenceAnalysis>());
-  VN.setDomTree(&getAnalysis<DominatorTree>());
+  VN.setMemDep(MD);
+  VN.setDomTree(DT);
   
   bool changed = false;
   bool shouldContinue = true;
   
+  // Merge unconditional branches, allowing PRE to catch more
+  // optimization opportunities.
+  for (Function::iterator FI = F.begin(), FE = F.end(); FI != FE; ) {
+    BasicBlock* BB = FI;
+    ++FI;
+    bool removedBlock = MergeBlockIntoPredecessor(BB, this);
+    if (removedBlock) NumGVNBlocks++;
+    
+    changed |= removedBlock;
+  }
+  
   while (shouldContinue) {
     shouldContinue = iterateOnFunction(F);
     changed |= shouldContinue;
   }
   
+  if (EnablePRE) {
+    bool PREChanged = true;
+    while (PREChanged) {
+      PREChanged = performPRE(F);
+      changed |= PREChanged;
+    }
+  }
+
+  cleanupGlobalSets();
+
   return changed;
 }
 
 
 bool GVN::processBlock(DomTreeNode* DTN) {
   BasicBlock* BB = DTN->getBlock();
-
   SmallVector<Instruction*, 8> toErase;
   DenseMap<Value*, LoadInst*> lastSeenLoad;
   bool changed_function = false;
@@ -1139,8 +1203,11 @@ bool GVN::processBlock(DomTreeNode* DTN) {
       --BI;
 
     for (SmallVector<Instruction*, 4>::iterator I = toErase.begin(),
-         E = toErase.end(); I != E; ++I)
+         E = toErase.end(); I != E; ++I) {
+      DEBUG(cerr << "GVN removed: " << **I);
+      MD->removeInstruction(*I);
       (*I)->eraseFromParent();
+    }
 
     if (AtStart)
       BI = BB->begin();
@@ -1156,8 +1223,9 @@ bool GVN::processBlock(DomTreeNode* DTN) {
 /// performPRE - Perform a purely local form of PRE that looks for diamond
 /// control flow patterns and attempts to perform simple PRE at the join point.
 bool GVN::performPRE(Function& F) {
-  bool changed = false;
+  bool Changed = false;
   SmallVector<std::pair<TerminatorInst*, unsigned>, 4> toSplit;
+  DenseMap<BasicBlock*, Value*> predMap;
   for (df_iterator<BasicBlock*> DI = df_begin(&F.getEntryBlock()),
        DE = df_end(&F.getEntryBlock()); DI != DE; ++DI) {
     BasicBlock* CurrentBlock = *DI;
@@ -1167,14 +1235,14 @@ bool GVN::performPRE(Function& F) {
     
     for (BasicBlock::iterator BI = CurrentBlock->begin(),
          BE = CurrentBlock->end(); BI != BE; ) {
-      if (isa<AllocaInst>(BI) || isa<TerminatorInst>(BI) ||
-          isa<LoadInst>(BI) || isa<StoreInst>(BI) ||
-          isa<CallInst>(BI) || isa<PHINode>(BI)) {
-        BI++;
+      Instruction *CurInst = BI++;
+      
+      if (isa<AllocationInst>(CurInst) || isa<TerminatorInst>(CurInst) ||
+          isa<PHINode>(CurInst) || CurInst->mayReadFromMemory() ||
+          CurInst->mayWriteToMemory())
         continue;
-      }
       
-      uint32_t valno = VN.lookup(BI);
+      uint32_t valno = VN.lookup(CurInst);
       
       // Look for the predecessors for PRE opportunities.  We're
       // only trying to solve the basic diamond case, where
@@ -1185,7 +1253,8 @@ bool GVN::performPRE(Function& F) {
       unsigned numWith = 0;
       unsigned numWithout = 0;
       BasicBlock* PREPred = 0;
-      DenseMap<BasicBlock*, Value*> predMap;
+      predMap.clear();
+
       for (pred_iterator PI = pred_begin(CurrentBlock),
            PE = pred_end(CurrentBlock); PI != PE; ++PI) {
         // We're not interested in PRE where the block is its
@@ -1204,7 +1273,7 @@ bool GVN::performPRE(Function& F) {
         if (predV == localAvail[*PI]->table.end()) {
           PREPred = *PI;
           numWithout++;
-        } else if (predV->second == BI) {
+        } else if (predV->second == CurInst) {
           numWithout = 2;
         } else {
           predMap[*PI] = predV->second;
@@ -1214,10 +1283,8 @@ bool GVN::performPRE(Function& F) {
       
       // Don't do PRE when it might increase code size, i.e. when
       // we would need to insert instructions in more than one pred.
-      if (numWithout != 1 || numWith == 0) {
-        BI++;
+      if (numWithout != 1 || numWith == 0)
         continue;
-      }
       
       // We can't do PRE safely on a critical edge, so instead we schedule
       // the edge to be split and perform the PRE the next time we iterate
@@ -1225,15 +1292,14 @@ bool GVN::performPRE(Function& F) {
       unsigned succNum = 0;
       for (unsigned i = 0, e = PREPred->getTerminator()->getNumSuccessors();
            i != e; ++i)
-        if (PREPred->getTerminator()->getSuccessor(i) == PREPred) {
+        if (PREPred->getTerminator()->getSuccessor(i) == CurrentBlock) {
           succNum = i;
           break;
         }
         
       if (isCriticalEdge(PREPred->getTerminator(), succNum)) {
         toSplit.push_back(std::make_pair(PREPred->getTerminator(), succNum));
-        changed = true;
-        BI++;
+        Changed = true;
         continue;
       }
       
@@ -1242,17 +1308,19 @@ bool GVN::performPRE(Function& F) {
       // will be available in the predecessor by the time we need them.  Any
       // that weren't original present will have been instantiated earlier
       // in this loop.
-      Instruction* PREInstr = BI->clone();
+      Instruction* PREInstr = CurInst->clone();
       bool success = true;
-      for (unsigned i = 0; i < BI->getNumOperands(); ++i) {
-        Value* op = BI->getOperand(i);
-        if (isa<Argument>(op) || isa<Constant>(op) || isa<GlobalValue>(op))
-          PREInstr->setOperand(i, op);
-        else if (!lookupNumber(PREPred, VN.lookup(op))) {
+      for (unsigned i = 0, e = CurInst->getNumOperands(); i != e; ++i) {
+        Value *Op = PREInstr->getOperand(i);
+        if (isa<Argument>(Op) || isa<Constant>(Op) || isa<GlobalValue>(Op))
+          continue;
+        
+        if (Value *V = lookupNumber(PREPred, VN.lookup(Op))) {
+          PREInstr->setOperand(i, V);
+        } else {
           success = false;
           break;
-        } else
-          PREInstr->setOperand(i, lookupNumber(PREPred, VN.lookup(op)));
+        }
       }
       
       // Fail out if we encounter an operand that is not available in
@@ -1260,12 +1328,11 @@ bool GVN::performPRE(Function& F) {
       // are not value numbered precisely.
       if (!success) {
         delete PREInstr;
-        BI++;
         continue;
       }
       
       PREInstr->insertBefore(PREPred->getTerminator());
-      PREInstr->setName(BI->getName() + ".pre");
+      PREInstr->setName(CurInst->getName() + ".pre");
       predMap[PREPred] = PREInstr;
       VN.add(PREInstr, valno);
       NumGVNPRE++;
@@ -1274,62 +1341,56 @@ bool GVN::performPRE(Function& F) {
       localAvail[PREPred]->table.insert(std::make_pair(valno, PREInstr));
       
       // Create a PHI to make the value available in this block.
-      PHINode* Phi = PHINode::Create(BI->getType(),
-                                     BI->getName() + ".pre-phi",
+      PHINode* Phi = PHINode::Create(CurInst->getType(),
+                                     CurInst->getName() + ".pre-phi",
                                      CurrentBlock->begin());
       for (pred_iterator PI = pred_begin(CurrentBlock),
            PE = pred_end(CurrentBlock); PI != PE; ++PI)
         Phi->addIncoming(predMap[*PI], *PI);
       
       VN.add(Phi, valno);
-      
-      // The newly created PHI completely replaces the old instruction,
-      // so we need to update the maps to reflect this.
-      DomTreeNode* DTN = getAnalysis<DominatorTree>()[CurrentBlock];
-      for (DomTreeNode::iterator UI = DTN->begin(), UE = DTN->end();
-           UI != UE; ++UI)
-        localAvail[(*UI)->getBlock()]->table[valno] = Phi;
       localAvail[CurrentBlock]->table[valno] = Phi;
       
-      BI->replaceAllUsesWith(Phi);
-      VN.erase(BI);
-      
-      Instruction* erase = BI;
-      BI++;
-      erase->eraseFromParent();
+      CurInst->replaceAllUsesWith(Phi);
+      VN.erase(CurInst);
       
-      changed = true;
+      DEBUG(cerr << "GVN PRE removed: " << *CurInst);
+      MD->removeInstruction(CurInst);
+      CurInst->eraseFromParent();
+      Changed = true;
     }
   }
   
   for (SmallVector<std::pair<TerminatorInst*, unsigned>, 4>::iterator
-       I = toSplit.begin(), E = toSplit.end(); I != E; ++I)
+       I = toSplit.begin(), E = toSplit.end(); I != E; ++I) {
     SplitCriticalEdge(I->first, I->second, this);
+    BasicBlock* NewBlock = I->first->getSuccessor(I->second);
+    localAvail[NewBlock] =
+             new ValueNumberScope(localAvail[I->first->getParent()]);
+  }
   
-  return changed;
+  return Changed;
 }
 
-// GVN::iterateOnFunction - Executes one iteration of GVN
+// iterateOnFunction - Executes one iteration of GVN
 bool GVN::iterateOnFunction(Function &F) {
-  // Clean out global sets from any previous functions
-  VN.clear();
-  phiMap.clear();
-  
-  for (DenseMap<BasicBlock*, ValueNumberScope*>::iterator
-       I = localAvail.begin(), E = localAvail.end(); I != E; ++I)
-    delete I->second;
-  localAvail.clear();
-  
-  DominatorTree &DT = getAnalysis<DominatorTree>();   
+  cleanupGlobalSets();
 
   // Top-down walk of the dominator tree
   bool changed = false;
-  for (df_iterator<DomTreeNode*> DI = df_begin(DT.getRootNode()),
-       DE = df_end(DT.getRootNode()); DI != DE; ++DI)
+  for (df_iterator<DomTreeNode*> DI = df_begin(DT->getRootNode()),
+       DE = df_end(DT->getRootNode()); DI != DE; ++DI)
     changed |= processBlock(*DI);
   
-  if (!EnablePRE)
-    changed |= performPRE(F);
-  
   return changed;
 }
+
+void GVN::cleanupGlobalSets() {
+  VN.clear();
+  phiMap.clear();
+
+  for (DenseMap<BasicBlock*, ValueNumberScope*>::iterator
+       I = localAvail.begin(), E = localAvail.end(); I != E; ++I)
+    delete I->second;
+  localAvail.clear();
+}