[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.

[Daniel Berlin via Phabricator via llvm-commits]

dberlin marked 2 inline comments as done.
dberlin 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; }
+};
----------------
davide wrote:
> hmm, this is a quite peculiar implementation of operator preincrement/postincrement as they actually don't increment anything.
This is standard for inserters.
See https://gcc.gnu.org/onlinedocs/gcc-4.6.2/libstdc++/api/a01052_source.html#l00408
and
http://en.cppreference.com/w/cpp/iterator/back_insert_iterator
" Incrementing the std::back_insert_iterator is a no-op."



================
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:
----------------
davide wrote:
> `op_inserter` and `int_op_inserter` are nearly identical. Is there any chance you can templatize to reduce the duplication?
yeah, i'll try to fix this.



================
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);
----------------
davide wrote:
> 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.
Fixed


https://reviews.llvm.org/D28111