[PATCH] D26224: NewGVN

[Sean Silva via Phabricator via llvm-commits]

silvas added a comment.

Simon has done a pretty good job with the style review. Once his comments are cleaned up it will be easier to provide further comments.

A couple other stylistic comments and a doc request. I still need to do a walk-through of the code and stuff to provide more in-depth comments.



================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:241
+
+  virtual void printInternal(raw_ostream &OS, bool printEType) const {
+    if (printEType)
----------------
`PrintEType` here and elsewhere.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:10
+/// \file
+/// This file implements the new LLVM's Global Value Numbering pass.
+///
----------------
This file comment needs to be signficantly expanded. Remember, lots of people looking at this class might be e.g. people taking a compiler class that want to look at a "real" GVN implementation. Let's make sure come away impressed so that they will want to join LLVM!

At the very least, some citations for the relevant paper and stuff, along with summary of which exact variant of the algorithm are implemented would be good. Also, I think the high-level idea of GVN is simple enough that a high-level from-scratch description would be appropriate. I can help with writing this if you want.

In theory, we can expand this later, but when have you seen a commit improving a file-level comment? The only one I remember was in response to post-commit review asking for an improved file-level comment. So getting it right the first time is actually pretty important.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:474
+  } else if (isa<Argument>(V) || isa<GlobalVariable>(V)) {
+#ifndef NDEBUG
+    if (I)
----------------
Are the ifdef's necessary when all that is inside is a DEBUG?


https://reviews.llvm.org/D26224