[PATCH] D26224: NewGVN

[David Majnemer via llvm-commits]

majnemer added inline comments.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:14-15
+///
+//===----------------------------------------------------------------------===//
+#ifndef LLVM_TRANSFORMS_SCALAR_GVNEXPRESSION_H
+#define LLVM_TRANSFORMS_SCALAR_GVNEXPRESSION_H
----------------
I'd insert a newline here.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:16-17
+#ifndef LLVM_TRANSFORMS_SCALAR_GVNEXPRESSION_H
+#define LLVM_TRANSFORMS_SCALAR_GVNEXPRESSION_H
+#include "llvm/ADT/Hashing.h"
+#include "llvm/IR/Constant.h"
----------------
I'd insert a newline here.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:76
+      return true;
+    // Compare etype for anything but load and store
+    if (getExpressionType() != ExpressionTypeLoad &&
----------------
Comments should end with a period.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:85
+
+  virtual bool equals(const Expression &other) const { return true; }
+
----------------
`other` should follow the naming convention.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:118-119
+  Value **Operands;
+  unsigned int MaxOperands;
+  unsigned int NumOperands;
+  Type *ValueType;
----------------
`unsigned int` -> `unsigned`, there are other places where this not followed, please correct them.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:128-161
+  inline void swapOperands(unsigned First, unsigned Second) {
+    std::swap(Operands[First], Operands[Second]);
+  }
+  inline Value *getOperand(unsigned N) const {
+    assert(Operands && "Operands not allocated");
+    assert(N < NumOperands && "Operand out of range");
+    return Operands[N];
----------------
Inline keyword is superfluous, we are in the class definition. Please correct this elsewhere as well.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:556
+      return false;
+  } else if (const StoreExpression *OtherS =
+                 dyn_cast<StoreExpression>(&Other)) {
----------------
Please use `const auto *` when assigning w/ a `dyn_cast`, the type should be clear.


https://reviews.llvm.org/D26224