[PATCH] D12004: [RewriteStatepointsForGC] Reduce the number of new instructions for base pointers

[Sanjoy Das via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

lgtm with comments addressed.


================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1032
@@ +1031,3 @@
+    if (auto *BaseI = dyn_cast<Instruction>(Base)) {
+      NewInsts.insert(BaseI);
+      Worklist.insert(BaseI);
----------------
reames wrote:
> sanjoy wrote:
> > Can't this set be populated as we create the instructions above?
> It absolutely could.  I was trying to keep the code separate to make it standalone, but I can merge if you'd prefer.  
I'd prefer that -- it makes it more obvious what happened.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1048
@@ +1047,3 @@
+        DEBUG(dbgs() << "Identical Base: " << *BaseI << "\n");
+        PushNewUsers(BaseI);
+        BaseI->replaceAllUsesWith(Bdv);
----------------
reames wrote:
> sanjoy wrote:
> > Can `PushNewUsers(BaseI)` be hoisted out of this `if` block with a `if (Visited.insert(BaseI).second)` check?  Or does this need to remain conditional for correctness?
> It's required for correctness.  Consider a self referential phi node.  You only want to add it to the worklist if you modify it, otherwise the loop would run forever.  
> 
> Oh, wait, you were proposing a separate Visited structure.  Yes, that would work.  Do you have a preference?
> Do you have a preference?

No, I was just trying to make sure I understood what was going on. :)


http://reviews.llvm.org/D12004