[llvm-commits] CVS: llvm/lib/Transforms/Scalar/GVNPRE.cpp

[Owen Anderson]

Changes in directory llvm/lib/Transforms/Scalar:

GVNPRE.cpp updated: 1.24 -> 1.25
---
Log message:

Fix a bug that was causing the elimination phase not to replace values when it should be.
With this patch, GVN-PRE now correctly optimizes the example from the thesis.

Many thanks to Daniel Berlin for helping me find errors in this.


---
Diffs of the changes:  (+56 -13)

 GVNPRE.cpp |   69 +++++++++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 56 insertions(+), 13 deletions(-)


Index: llvm/lib/Transforms/Scalar/GVNPRE.cpp
diff -u llvm/lib/Transforms/Scalar/GVNPRE.cpp:1.24 llvm/lib/Transforms/Scalar/GVNPRE.cpp:1.25
--- llvm/lib/Transforms/Scalar/GVNPRE.cpp:1.24	Thu Jun  7 20:52:45 2007
+++ llvm/lib/Transforms/Scalar/GVNPRE.cpp	Fri Jun  8 15:44:02 2007
@@ -45,7 +45,9 @@
     BinaryOperator* BO1 = cast<BinaryOperator>(left);
     BinaryOperator* BO2 = cast<BinaryOperator>(right);
     
-    if ((*this)(BO1->getOperand(0), BO2->getOperand(0)))
+    if (BO1->getOpcode() != BO2->getOpcode())
+      return BO1->getOpcode() < BO2->getOpcode();
+    else if ((*this)(BO1->getOperand(0), BO2->getOperand(0)))
       return true;
     else if ((*this)(BO2->getOperand(0), BO1->getOperand(0)))
       return false;
@@ -158,6 +160,7 @@
       Instruction* newVal = BinaryOperator::create(BO->getOpcode(),
                                              newOp1, newOp2,
                                              BO->getName()+".gvnpre");
+      
       if (add(newVal, nextValueNumber))
         nextValueNumber++;
       if (!find_leader(set, newVal)) {
@@ -165,6 +168,14 @@
         createdExpressions.insert(newVal);
         return newVal;
       } else {
+        ValueTable::iterator I = VN.find(newVal);
+        if (I->first == newVal)
+          VN.erase(newVal);
+        
+        std::set<Value*, ExprLT>::iterator F = MS.find(newVal);
+        if (*F == newVal)
+          MS.erase(newVal);
+        
         delete newVal;
         return 0;
       }
@@ -486,7 +497,6 @@
   
   
   // Phase 2: Insert
-  
   DOUT<< "\nPhase 2: Insertion\n";
   
   std::map<BasicBlock*, std::set<Value*, ExprLT> > new_sets;
@@ -503,6 +513,8 @@
       std::set<Value*, ExprLT>& availOut = availableOut[BB];
       std::set<Value*, ExprLT>& anticIn = anticipatedIn[BB];
       
+      new_set.clear();
+      
       // Replace leaders with leaders inherited from dominator
       if (DI->getIDom() != 0) {
         std::set<Value*, ExprLT>& dom_set = new_sets[DI->getIDom()->getBlock()];
@@ -510,9 +522,11 @@
              E = dom_set.end(); I != E; ++I) {
           new_set.insert(*I);
           
-          std::set<Value*, ExprLT>::iterator val = availOut.find(*I);
-          if (val != availOut.end())
+          Value* val = find_leader(availOut, *I);
+          while (val != 0) {
             availOut.erase(val);
+            val = find_leader(availOut, *I);
+          }
           availOut.insert(*I);
         }
       }
@@ -589,7 +603,14 @@
                                              BO->getName()+".gvnpre",
                                              (*PI)->getTerminator());
                   add(newVal, VN[BO]);
-                  availableOut[*PI].insert(newVal);
+                  
+                  std::set<Value*, ExprLT>& predAvail = availableOut[*PI];
+                  Value* val = find_leader(predAvail, newVal);
+                  while (val != 0) {
+                    predAvail.erase(val);
+                    val = find_leader(predAvail, newVal);
+                  }
+                  predAvail.insert(newVal);
                   
                   DOUT << "Creating value: " << std::hex << newVal << std::dec << "\n";
                   
@@ -614,7 +635,13 @@
               add(p, VN[e]);
               DOUT << "Creating value: " << std::hex << p << std::dec << "\n";
               
+              Value* val = find_leader(availOut, p);
+              while (val != 0) {
+                availOut.erase(val);
+                val = find_leader(availOut, p);
+              }
               availOut.insert(p);
+              
               new_stuff = true;
               
               DOUT << "Preds After Processing: ";
@@ -637,30 +664,46 @@
   }
   
   // Phase 3: Eliminate
+  DOUT << "\n\nPhase 3: Elimination\n\n";
+  
+  std::vector<std::pair<Instruction*, Value*> > replace;
+  std::vector<Instruction*> erase;
+  
   for (df_iterator<DomTreeNode*> DI = df_begin(DT.getRootNode()),
          E = df_end(DT.getRootNode()); DI != E; ++DI) {
     BasicBlock* BB = DI->getBlock();
     
-    std::vector<Instruction*> erase;
+    DOUT << "Block: " << BB->getName() << "\n";
+    dump_unique(availableOut[BB]);
+    DOUT << "\n\n";
     
     for (BasicBlock::iterator BI = BB->begin(), BE = BB->end();
          BI != BE; ++BI) {
-      if (!BI->isTerminator()) {
-        Value* leader = find_leader(availableOut[BB], BI);
+
+      if (isa<BinaryOperator>(BI)) {
+         Value *leader = find_leader(availableOut[BB], BI);
+  
         if (leader != 0)
           if (Instruction* Instr = dyn_cast<Instruction>(leader))
             if (Instr->getParent() != 0 && Instr != BI) {
-              BI->replaceAllUsesWith(leader);
+              replace.push_back(std::make_pair(BI, leader));
               erase.push_back(BI);
             }
       }
     }
-    
-    for (std::vector<Instruction*>::iterator I = erase.begin(), E = erase.end();
-         I != E; ++I)
-      (*I)->eraseFromParent();
   }
   
+  while (!replace.empty()) {
+    std::pair<Instruction*, Value*> rep = replace.back();
+    replace.pop_back();
+      
+    rep.first->replaceAllUsesWith(rep.second);
+  }
+    
+  for (std::vector<Instruction*>::iterator I = erase.begin(), E = erase.end();
+       I != E; ++I)
+     (*I)->eraseFromParent();
+  
   // Phase 4: Cleanup
   for (std::set<Instruction*>::iterator I = createdExpressions.begin(),
        E = createdExpressions.end(); I != E; ++I) {