[PATCH] D26224: NewGVN

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:65
+      return true;
+    // Compare etype for anything but load and store.
+    if (getExpressionType() != ET_Load &&
----------------
Please explain in this comment why you're not comparing the expression types for loads and stores. This is also somewhat confusing because we've already compared the opcodes.


================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:183
+  virtual bool equals(const Expression &Other) const override {
+    const BasicExpression &OE = cast<BasicExpression>(Other);
+    if (getOpcode() != OE.getOpcode())
----------------
This looks a bit odd. Shouldn't you check the opcode equality first and then cast to BasicExpression?


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:215
+  // Debugging for how many times each block and instruction got processed.
+  DenseMap<const Value *, unsigned> ProcessedCount;
+
----------------
Do we want to guard this with `#ifdef NDEBUG`?


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:460
+// I, and see if it resulted in a simpler expression. If so, return
+// that expression
+// TODO: Once finished, this should not take an Instruction, we only
----------------
Add period after expression.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:647
+      new (ExpressionAllocator) CallExpression(CI->getNumOperands(), CI, HV);
+  setBasicExpressionInfo(CI, E, B);
+  return E;
----------------
We also need to add operand bundles too for calls.


https://reviews.llvm.org/D26224