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

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

I don't see any particular problem with the code, but I have a few questions (inline) to ensure that I've understood the code correctly.


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

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1020
@@ +1019,3 @@
+  SmallSetVector<Instruction *, 16> Worklist;
+  for (auto item : states) {
+    Value *V = item.first;
----------------
Should be `Item`.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1032
@@ +1031,3 @@
+    if (auto *BaseI = dyn_cast<Instruction>(Base)) {
+      NewInsts.insert(BaseI);
+      Worklist.insert(BaseI);
----------------
Can't this set be populated as we create the instructions above?

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

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1055
@@ +1054,3 @@
+      }
+    const DataLayout &DL = BaseI->getModule()->getDataLayout();
+    if (Value *V = SimplifyInstruction(BaseI, DL)) {
----------------
I'd hoist this bit outside the loop.

================
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);
----------------
Will re-pushing `V` onto `Worklist` help?  I'm thinking of cases where `SimplifyInstruction(BaseI)->isIdenticalTo(BdvI)`.


http://reviews.llvm.org/D12004