[PATCH] D26224: NewGVN

[Michael Spencer via Phabricator via llvm-commits]

Bigcheese requested changes to this revision.
Bigcheese added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:49
+
+private:
+  void operator=(const Expression &) = delete;
----------------
These don't need to be private.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:54
+protected:
+  ExpressionType EType;
+  unsigned Opcode;
----------------
This can be private.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:55
+  ExpressionType EType;
+  unsigned Opcode;
+
----------------
Since you've got `getOptcode()` and `setOpcode()` this doesn't need to be protected. Either make this public and get rid of the accessors, or make it private.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:64
+  // Methods for support type inquiry through isa, cast, and dyn_cast.
+  static bool classof(const Expression *) { return true; }
+
----------------
I think this can be removed.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:69
+
+  virtual ~Expression() = default;
+
----------------
This needs to be an anchor as per http://llvm.org/docs/CodingStandards.html#provide-a-virtual-method-anchor-for-classes-in-headers


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:71-83
+  bool operator==(const Expression &Other) const {
+    if (Opcode != Other.Opcode)
+      return false;
+    if (Opcode == ~0U || Opcode == ~1U)
+      return true;
+    // Compare etype for anything but load and store.
+    if (getExpressionType() != ExpressionTypeLoad &&
----------------
The behavior of this operator is quite nonintuitive. It's actually `isTrivallyCongruent()`, not is equal. However if this is the natural equality definition for this type, then it would be fine with a comment explaining that.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:87-89
+  virtual hash_code getHashValue() const {
+    return hash_combine(EType, Opcode);
+  }
----------------
This can return a different `hash_code` for Expressions which compare equal.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:111-113
+  void operator=(const BasicExpression &) = delete;
+  BasicExpression(const BasicExpression &) = delete;
+  BasicExpression() = delete;
----------------
Don't need to be private.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:165
+  // Methods for support type inquiry through isa, cast, and dyn_cast.
+  static bool classof(const BasicExpression *) { return true; }
+  static bool classof(const Expression *EB) {
----------------
Not needed.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:189
+
+  virtual ~BasicExpression() = default;
+
----------------
Anchor.


================
Comment at: include/llvm/Transforms/Scalar/NewGVN.h:10-12
+/// This file provides the interface for LLVM's Global Value Numbering pass
+/// which eliminates fully redundant instructions. It also does somewhat Ad-Hoc
+/// PRE and dead load elimination.
----------------
This looks like the comment from the old GVN pass.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1
+//===- GVN.cpp - Eliminate redundant values and loads ---------------------===//
+//
----------------
NewGVN.cpp


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:10-14
+// This pass performs global value numbering to eliminate fully redundant
+// instructions.  It also performs simple dead load elimination.
+//
+// Note that this pass does the value numbering itself; it does not use the
+// ValueNumbering analysis passes.
----------------
Old comment. Missing \file


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:127
+    static const Expression *getEmptyKey() {
+      uintptr_t Val = static_cast<uintptr_t>(-1);
+      Val <<= PointerLikeTypeTraits<const Expression *>::NumLowBitsAvailable;
----------------
Prefer `~0u`


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:132
+    static const Expression *getTombstoneKey() {
+      uintptr_t Val = static_cast<uintptr_t>(-2);
+      Val <<= PointerLikeTypeTraits<const Expression *>::NumLowBitsAvailable;
----------------
Prefer `~1u`


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:145
+        return false;
+      return *LHS == *RHS;
+    }
----------------
This seems like the wrong equality operator for the above hash. It will return true for things that don't hash to the same value.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:210
+  static char ID; // Pass identification, replacement for typeid.
+  explicit NewGVN() : FunctionPass(ID) {
+    initializeNewGVNPass(*PassRegistry::getPassRegistry());
----------------
This doesn't need to be explicit.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1226
+
+std::pair<unsigned, unsigned> NewGVN::assignDFSNumbers(BasicBlock *B,
+                                                       unsigned Start) {
----------------
Do we have a proper type anywhere to use for a range instead of pair? first and last.


https://reviews.llvm.org/D26224