[RewriteStatepointsForGC] Bugfix for change 246133

[reames]

Summary

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.

[reames]

Fix minor whitespace issue.

[sanjoy]

lgtm with minor nits inline.

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

616 ↗

(On Diff #33863)

Not directly related to this change, but does this invariant actually hold? We're constructing a BDVState below with status == Conflict and a non-nullptr base.

1048 ↗

(On Diff #33863)

Can there be non Instruction uses of BaseI. If not, maybe this should be a cast<>?

1056 ↗

(On Diff #33863)

Can we assert states.count(BDV) here? Or maybe even states[BDV].base == BaseI?

[reames]

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

616 ↗

(On Diff #33863)

The comment does need updated. Actually, I think I should just extend the Status enum to include a new state because that's really what this is.

1048 ↗

(On Diff #33863)

I'm not sure about metadata and other weird values. I'd prefer to leave this as is.

1056 ↗

(On Diff #33863)

Yep, will do.

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

#endif
   }
 
   // Now that we're done with the algorithm, see if we can optimize the 
   // results slightly by reducing the number of new instructions needed. 
   // 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;
+  SmallSetVector<AssertingVH<Instruction>, 16> Worklist;
   for (auto Item : states) {
     Value *V = Item.first;
     Value *Base = Item.second.getBase();
     assert(V && Base);
     assert(!isKnownBaseResult(V) && "why did it get added?");
     assert(isKnownBaseResult(Base) &&
            "must be something we 'know' is a base pointer");
     if (!Item.second.isConflict())
       continue;
 
     ReverseMap[Base] = V;
     if (auto *BaseI = dyn_cast<Instruction>(Base)) {
       NewInsts.insert(BaseI);
       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()) {
     Instruction *BaseI = Worklist.pop_back_val();
     assert(NewInsts.count(BaseI));
     Value *Bdv = ReverseMap[BaseI];
     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;
     }
   }
 
   // Cache all of our results so we can cheaply reuse them
   // NOTE: This is actually two caches: one of the base defining value
   // relation and one of the base pointer relation!  FIXME
   for (auto item : states) {