[PATCH] D28111: Misc cleanups and simplifications for NewGVN.Mostly use a bit more idiomatic C++ where we can, so we can combine some things later.

[Davide Italiano via Phabricator via llvm-commits]

davide added inline comments.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:232-233
+  op_inserter &operator*() { return *this; }
+  op_inserter &operator++() { return *this; }
+  op_inserter &operator++(int) { return *this; }
+};
----------------
hmm, this is a quite peculiar implementation of operator preincrement/postincrement as they actually don't increment anything.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:438-453
+class int_op_inserter
+    : public std::iterator<std::output_iterator_tag, void, void, void, void> {
+private:
+  typedef AggregateValueExpression Container;
+  Container *AVE;
+
+public:
----------------
`op_inserter` and `int_op_inserter` are nearly identical. Is there any chance you can templatize to reduce the duplication?


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:418-425
+  std::transform(Filtered.begin(), Filtered.end(),
+                 op_inserter(E), [&](const Use &U) -> Value *{
+                   // Don't try to transform self-defined phis
+                   if (U == PN)
+                     return PN;
+                   const BasicBlockEdge BBE(PN->getIncomingBlock(U), PhiBlock);
+                   return lookupOperandLeader(U, I, BBE);
----------------
Very nice use of `std::transform`.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1475-1482
+  auto UnreachableBlockPred =
+      [&](const BasicBlock &B) { return !ReachableBlocks.count(&B); };
+
+  for (auto &B : make_filter_range(F, UnreachableBlockPred)) {
+    DEBUG(dbgs() << "We believe block " << getBlockName(&B)
+          << " is unreachable\n");
+    deleteInstructionsInBlock(&B);
----------------
This is a very minor nit, but (almost) everywhere else in llvm we use `BB` for a `BasicBlock` variable. Here we had `B` to distinguish between it and `BB`, but I think it makes sense to use `BB` instead.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1753-1770
+  // instructions part of most singleton congruence classes, we know we
+  // will never eliminate them.
 
   // Instead, this eliminator looks at the congruence classes directly, sorts
   // them into a DFS ordering of the dominator tree, and then we just
-  // perform eliminate straight on the sets by walking the congruence
+  // perform elimination straight on the sets by walking the congruence
   // class member uses in order, and eliminate the ones dominated by the
----------------
Thanks for expanding the comment. You can consider committing this separately to reduce the diff (as it's fairly independent change).


https://reviews.llvm.org/D28111