[PATCH] D12575: [RewriteStatepointsForGC] Bugfix for change 246133

[Philip Reames via llvm-commits]

reames added inline comments.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:616
@@ -615,3 +615,3 @@
   Status status;
   Value *base; // non null only if status == base
 };
----------------
sanjoy wrote:
> 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`.
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.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1048
@@ -1046,1 +1047,3 @@
+    for (User *U : BaseI->users())
       if (auto *UI = dyn_cast<Instruction>(U))
+        if (NewInsts.count(UI) && UI != BaseI)
----------------
sanjoy wrote:
> Can there be non `Instruction` uses of `BaseI`.  If not, maybe this should be a `cast<>`?
I'm not sure about metadata and other weird values.  I'd prefer to leave this as is. 

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1056
@@ -1049,1 +1055,3 @@
+    BaseI->eraseFromParent();
+    states[BDV] = BDVState(BDVState::Conflict, Replacement);
   };
----------------
sanjoy wrote:
> Can we assert `states.count(BDV)` here?  Or maybe even `states[BDV].base == BaseI`?
Yep, will do.


http://reviews.llvm.org/D12575