[llvm-commits] [llvm] r60263 - /llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp

[Chris Lattner]

Author: lattner
Date: Sat Nov 29 19:09:30 2008
New Revision: 60263

URL: http://llvm.org/viewvc/llvm-project?rev=60263&view=rev
Log:
remove a bit of incorrect code that tried to be tricky about speeding up 
dependencies.  The basic situation was this: consider if we had:

  store1
  ...
  store2
  ...
  store3

Where memdep thinks that store3 depends on store2 and store2 depends 
on store1.  The problem happens when we delete store2: The code in 
question was updating dep info for store3 to be store1.  This is a
spiffy optimization, but is not safe at all, because aliasing isn't
transitive.  This bug isn't exposed today with DSE because DSE will only
zap store2 if it is identifical to store 3, and in this case, it is 
safe to update it to depend on store1.  However, memcpyopt is not so
fortunate, which is presumably why the "dropInstruction" code used to
exist.

Since this doesn't actually provide a speedup in practice, just rip the
code out.

Modified:
    llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp

Modified: llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp?rev=60263&r1=60262&r2=60263&view=diff

==============================================================================
--- llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp (original)
+++ llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp Sat Nov 29 19:09:30 2008
@@ -336,47 +336,21 @@
     if (Instruction *Inst = DI->second.getPointer())
       ReverseNonLocalDeps[Inst].erase(RemInst);
 
-  // Shortly after this, we will look for things that depend on RemInst.  In
-  // order to update these, we'll need a new dependency to base them on.  We
-  // could completely delete any entries that depend on this, but it is better
-  // to make a more accurate approximation where possible.  Compute that better
-  // approximation if we can.
-  DepResultTy NewDependency;
-  
   // If we have a cached local dependence query for this instruction, remove it.
   //
   LocalDepMapType::iterator LocalDepEntry = LocalDeps.find(RemInst);
   if (LocalDepEntry != LocalDeps.end()) {
-    DepResultTy LocalDep = LocalDepEntry->second;
-    
-    // Remove this local dependency info.
-    LocalDeps.erase(LocalDepEntry);
-    
     // Remove us from DepInst's reverse set now that the local dep info is gone.
-    if (Instruction *Inst = LocalDep.getPointer())
-      ReverseLocalDeps[Inst].erase(RemInst);
-
-    // If we have unconfirmed info, don't trust it.
-    if (LocalDep.getInt() != Dirty) {
-      // If we have a confirmed non-local flag, use it.
-      if (LocalDep.getInt() == NonLocal || LocalDep.getInt() == None) {
-        // The only time this dependency is confirmed is if it is non-local.
-        NewDependency = LocalDep;
-      } else {
-        // If we have dep info for RemInst, set them to it.
-        Instruction *NDI = next(BasicBlock::iterator(LocalDep.getPointer()));
-        if (NDI != RemInst) // Don't use RemInst for the new dependency!
-          NewDependency = DepResultTy(NDI, Dirty);
-      }
+    if (Instruction *Inst = LocalDepEntry->second.getPointer()) {
+      SmallPtrSet<Instruction*, 4> &RLD = ReverseLocalDeps[Inst];
+      RLD.erase(RemInst);
+      if (RLD.empty())
+        ReverseLocalDeps.erase(Inst);
     }
-  }
-  
-  // If we don't already have a local dependency answer for this instruction,
-  // use the immediate successor of RemInst.  We use the successor because
-  // getDependence starts by checking the immediate predecessor of what is in
-  // the cache.
-  if (NewDependency == DepResultTy(0, Dirty))
-    NewDependency = DepResultTy(next(BasicBlock::iterator(RemInst)), Dirty);
+
+    // Remove this local dependency info.
+    LocalDeps.erase(LocalDepEntry);
+  }    
   
   // Loop over all of the things that depend on the instruction we're removing.
   // 
@@ -385,6 +359,16 @@
   ReverseDepMapType::iterator ReverseDepIt = ReverseLocalDeps.find(RemInst);
   if (ReverseDepIt != ReverseLocalDeps.end()) {
     SmallPtrSet<Instruction*, 4> &ReverseDeps = ReverseDepIt->second;
+    // RemInst can't be the terminator if it has stuff depending on it.
+    assert(!ReverseDeps.empty() && !isa<TerminatorInst>(RemInst) &&
+           "Nothing can locally depend on a terminator");
+    
+    // Anything that was locally dependent on RemInst is now going to be
+    // dependent on the instruction after RemInst.  It will have the dirty flag
+    // set so it will rescan.  This saves having to scan the entire block to get
+    // to this point.
+    Instruction *NewDepInst = next(BasicBlock::iterator(RemInst));
+                        
     for (SmallPtrSet<Instruction*, 4>::iterator I = ReverseDeps.begin(),
          E = ReverseDeps.end(); I != E; ++I) {
       Instruction *InstDependingOnRemInst = *I;
@@ -392,21 +376,12 @@
       // If we thought the instruction depended on itself (possible for
       // unconfirmed dependencies) ignore the update.
       if (InstDependingOnRemInst == RemInst) continue;
+                        
+      LocalDeps[InstDependingOnRemInst] = DepResultTy(NewDepInst, Dirty);
       
-      // Insert the new dependencies.
-      // FIXME: DEPENDENCIES ARE NOT TRANSITIVE!
-      //cerr << "FOO:\n";
-      //RemInst->dump();
-      //InstDependingOnRemInst->dump();
-      LocalDeps[InstDependingOnRemInst] = NewDependency;
-      
-      // If our NewDependency is an instruction, make sure to remember that new
-      // things depend on it.
-      if (Instruction *Inst = NewDependency.getPointer()) {
-        assert(Inst != RemInst);
-        ReverseDepsToAdd.push_back(std::make_pair(Inst, 
-                                                  InstDependingOnRemInst));
-      }
+      // Make sure to remember that new things depend on NewDepInst.
+      ReverseDepsToAdd.push_back(std::make_pair(NewDepInst, 
+                                                InstDependingOnRemInst));
     }
     
     ReverseLocalDeps.erase(ReverseDepIt);