[PATCH] D26224: NewGVN

[Davide Italiano via Phabricator via llvm-commits]

davide added inline comments.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:647
+      new (ExpressionAllocator) CallExpression(CI->getNumOperands(), CI, HV);
+  setBasicExpressionInfo(CI, E, B);
+  return E;
----------------
hfinkel wrote:
> We also need to add operand bundles too for calls.
Addressed all your other comments, Hal. I put a `FIXME` here and I'll review it later (sorry I'm not super familiar with operator bundles and I want to add a test as well).


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:842
+
+/// performSymbolicEvaluation - Substitute and symbolize the value
+/// before value numbering.
----------------
silvas wrote:
> "Don’t duplicate function or class name at the beginning of the comment."
> 
> http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
Done, did a pass over `NewGVN.cpp`


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1455
+
+    if (DFSIn < other.DFSIn)
+      return true;
----------------
silvas wrote:
> This is just implementing a lexicographical comparison, right?
> 
> If so, I would do:
> 
> ```
> return std::tie(DFSIn, DFSOut, ...) < std::tie(other.DFSIn, other.DFSOut, ...);
> ```
Done, it's now much easier to understand, thanks!


https://reviews.llvm.org/D26224