[PATCH] D26224: NewGVN
Davide Italiano via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 13 18:14:21 PST 2016
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;
> 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.
> "Don’t duplicate function or class name at the beginning of the comment."
Done, did a pass over `NewGVN.cpp`
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1455
+ if (DFSIn < other.DFSIn)
+ return true;
> 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!
More information about the llvm-commits