[PATCH] D12575: [RewriteStatepointsForGC] Bugfix for 246133

Philip Reames via llvm-commits llvm-commits at lists.llvm.org
Wed Sep 2 14:57:56 PDT 2015


reames created this revision.
reames added a reviewer: sanjoy.
reames added a subscriber: llvm-commits.

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.  

http://reviews.llvm.org/D12575

Files:
  lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

Index: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
===================================================================
--- lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
+++ lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
@@ -1022,8 +1022,8 @@
   // Arguably, this should be integrated into the algorithm above, but 
   // 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;
+  SmallPtrSet<Instruction*, 16> NewInsts;
+  SmallSetVector<AssertingVH<Instruction>, 16> Worklist;
   for (auto Item : states) {
     Value *V = Item.first;
     Value *Base = Item.second.getBase();
@@ -1040,11 +1040,19 @@
       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();
+    states[BDV] = BDVState(BDVState::Conflict, Replacement);
   };
   const DataLayout &DL = cast<Instruction>(def)->getModule()->getDataLayout();
   while (!Worklist.empty()) {
@@ -1054,22 +1062,12 @@
     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;
     }
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D12575.33859.patch
Type: text/x-patch
Size: 2529 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150902/a411ba1a/attachment.bin>


More information about the llvm-commits mailing list