[llvm] r246718 - [RewriteStatepointsForGC] Bugfix for change 246133

[Philip Reames via llvm-commits]

Author: reames
Date: Wed Sep  2 17:25:07 2015
New Revision: 246718

URL: http://llvm.org/viewvc/llvm-project?rev=246718&view=rev
Log:
[RewriteStatepointsForGC] Bugfix for change 246133

Fix a bug in change 246133. I didn't handle the case where we had a cycle in the use graph and could add an instruction we were about to erase back on to the worklist. Oddly, I have not been able to write a small test case for this, even with the AssertingVH added. I have confirmed the basic theory for the fix on a large failing example, but all attempts to reduce that to something appropriate for a test case have failed.

Differential Revision: http://reviews.llvm.org/D12575


Modified:
    llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

Modified: llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp?rev=246718&r1=246717&r2=246718&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp Wed Sep  2 17:25:07 2015
@@ -1024,7 +1024,7 @@ static Value *findBasePointer(Value *I,
   // doing as a post process step is easier to reason about for the moment.
   DenseMap<Value *, Value *> ReverseMap;
   SmallPtrSet<Instruction *, 16> NewInsts;
-  SmallSetVector<Instruction *, 16> Worklist;
+  SmallSetVector<AssertingVH<Instruction>, 16> Worklist;
   for (auto Item : states) {
     Value *V = Item.first;
     Value *Base = Item.second.getBase();
@@ -1041,11 +1041,21 @@ static Value *findBasePointer(Value *I,
       Worklist.insert(BaseI);
     }
   }
-  auto PushNewUsers = [&](Instruction *I) {
-    for (User *U : I->users())
+  auto ReplaceBaseInstWith = [&](Value *BDV, Instruction *BaseI,
+                                 Value *Replacement) {
+    // Add users which are new instructions (excluding self references)
+    for (User *U : BaseI->users())
       if (auto *UI = dyn_cast<Instruction>(U))
-        if (NewInsts.count(UI))
+        if (NewInsts.count(UI) && UI != BaseI)
           Worklist.insert(UI);
+    // Then do the actual replacement
+    NewInsts.erase(BaseI);
+    ReverseMap.erase(BaseI);
+    BaseI->replaceAllUsesWith(Replacement);
+    BaseI->eraseFromParent();
+    assert(states.count(BDV));
+    assert(states[BDV].isConflict() && states[BDV].getBase() == BaseI);
+    states[BDV] = BDVState(BDVState::Conflict, Replacement);
   };
   const DataLayout &DL = cast<Instruction>(def)->getModule()->getDataLayout();
   while (!Worklist.empty()) {
@@ -1055,22 +1065,12 @@ static Value *findBasePointer(Value *I,
     if (auto *BdvI = dyn_cast<Instruction>(Bdv))
       if (BaseI->isIdenticalTo(BdvI)) {
         DEBUG(dbgs() << "Identical Base: " << *BaseI << "\n");
-        PushNewUsers(BaseI);
-        BaseI->replaceAllUsesWith(Bdv);
-        BaseI->eraseFromParent();
-        states[Bdv] = BDVState(BDVState::Conflict, Bdv);
-        NewInsts.erase(BaseI);
-        ReverseMap.erase(BaseI);
+        ReplaceBaseInstWith(Bdv, BaseI, Bdv);
         continue;
       }
     if (Value *V = SimplifyInstruction(BaseI, DL)) {
       DEBUG(dbgs() << "Base " << *BaseI << " simplified to " << *V << "\n");
-      PushNewUsers(BaseI);
-      BaseI->replaceAllUsesWith(V);
-      BaseI->eraseFromParent();
-      states[Bdv] = BDVState(BDVState::Conflict, V);
-      NewInsts.erase(BaseI);
-      ReverseMap.erase(BaseI);
+      ReplaceBaseInstWith(Bdv, BaseI, V);
       continue;
     }
   }