[PATCH] D26224: NewGVN

Hal Finkel via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 13 16:01:00 PST 2016

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.


More information about the llvm-commits mailing list