[PATCH] D24805: [GVNSink] Initial GVNSink prototype

[Eli Friedman via Phabricator via llvm-commits]

efriedma edited reviewers, added: efriedma; removed: eli.friedman.
efriedma added inline comments.


================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:508
+  GVNSink(DominatorTree *DT, PostDominatorTree *PDT) : VN(), DT(DT), PDT(PDT) {}
+  bool run(Function &F) {
+    DEBUG(dbgs() << "GVNSink: running on function @" << F.getName() << "\n");
----------------
Maybe move some of the implementations out of the class definition to reduce indentation?


================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:577
+    }
+  }
+
----------------
This function should probably be somewhere outside GVNHoist, so other passes can use it.


================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:683
+      if ((isa<CallInst>(I0) || isa<InvokeInst>(I0)) && OpNum == E - 1 &&
+	  PHI.areAnyIncomingValuesConstant())
+        return None;
----------------
Indentation.


================
Comment at: lib/Transforms/Scalar/GVNSink.cpp:871
+      return {0, 0};
+    for (auto &N : depth_first(PDT)) {
+      if (!N->getBlock())
----------------
Could you use `post_order(F)` instead of PDT here?  If not, could you add a comment explaining what properties of the post-dominator tree you're depending on?


Repository:
  rL LLVM

https://reviews.llvm.org/D24805