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

[Philip Reames via llvm-commits]

reames added a comment.

Anything I didn't explicitly comment on, I accept and will address in a revised patch once the comments are settled.


================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1018
@@ +1017,3 @@
+  DenseMap<Value*, Value*> ReverseMap;
+  SmallPtrSet<Instruction*, 16> NewInsts;
+  SmallSetVector<Instruction *, 16> Worklist;
----------------
sanjoy wrote:
> There are some whitespace issues here -- do you mind running this patch through `clang-format` before checkin?
Sure.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1032
@@ +1031,3 @@
+    if (auto *BaseI = dyn_cast<Instruction>(Base)) {
+      NewInsts.insert(BaseI);
+      Worklist.insert(BaseI);
----------------
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.  

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1048
@@ +1047,3 @@
+        DEBUG(dbgs() << "Identical Base: " << *BaseI << "\n");
+        PushNewUsers(BaseI);
+        BaseI->replaceAllUsesWith(Bdv);
----------------
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?

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1057
@@ +1056,3 @@
+    if (Value *V = SimplifyInstruction(BaseI, DL)) {
+      DEBUG(dbgs() << "Base " << *BaseI << " simplified to " << *V << "\n");
+      PushNewUsers(BaseI);
----------------
sanjoy wrote:
> Will re-pushing `V` onto `Worklist` help?  I'm thinking of cases where `SimplifyInstruction(BaseI)->isIdenticalTo(BdvI)`.
I don't think this would ever happen in practice, but it's theoretically sound to do.  


http://reviews.llvm.org/D12004