[PATCH] D26224: NewGVN

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

Sorry for the delay. I'm on the road this week with limited access to internet.
I encourage other people to comment so that we can get the first revision in tree and iterate from there.



================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:87-89
+  virtual hash_code getHashValue() const {
+    return hash_combine(EType, Opcode);
+  }
----------------
Bigcheese wrote:
> This can return a different `hash_code` for Expressions which compare equal.
See Danny's reply (in the next mail).


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1226
+
+std::pair<unsigned, unsigned> NewGVN::assignDFSNumbers(BasicBlock *B,
+                                                       unsigned Start) {
----------------
Bigcheese wrote:
> Do we have a proper type anywhere to use for a range instead of pair? first and last.
No, I personally don't mind std::pair, but I can change it if you feel strong.


https://reviews.llvm.org/D26224