[PATCH] D26224: NewGVN

[Piotr Padlewski via Phabricator via llvm-commits]

Prazek added inline comments.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:111-114
+  Value **Operands;
+  unsigned MaxOperands;
+  unsigned NumOperands;
+  Type *ValueType;
----------------
It would be better to provide defautl values here


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:125-126
+  BasicExpression(unsigned NumOperands, ExpressionType ET)
+      : Expression(ET), Operands(nullptr), MaxOperands(NumOperands),
+        NumOperands(0), ValueType(nullptr) {}
+  virtual ~BasicExpression() override;
----------------
and remove it from here.
Also, it seems suspicious that NumOperands argument is not NumOperands, it is actually MaxOperands.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:187
+
+    const BasicExpression &OE = cast<BasicExpression>(Other);
+    if (getType() != OE.getType())
----------------
const auto &OE


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:241
+      return false;
+    const CallExpression &OE = cast<CallExpression>(Other);
+    return DefiningAccess == OE.DefiningAccess;
----------------
const auto &OE


https://reviews.llvm.org/D26224