[PATCH] D32151: Last of the major pieces to NewGVN - yay!

[Piotr Padlewski via Phabricator via llvm-commits]

Prazek added a comment.

Good job! How many of old gvn tests are still failing with newgvn?



================
Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:487
   PHIExpression(unsigned NumOperands, BasicBlock *B)
-      : BasicExpression(NumOperands, ET_Phi), BB(B) {}
+      : BasicExpression(NumOperands, ET_Phi), BB(B), AllRealOps(false) {}
   PHIExpression() = delete;
----------------
member initialization instead? (bool AllRealOps = false;) 


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1745
+// because they are often worse to put in place.
+// TODO: Separate cost from availability
+static bool alwaysAvailable(Value *V) {
----------------
does this comment still hold?


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1829-1830
+  // completeness, we should be able to always convert one to the other.
+  if (auto *RTV = RealToTemp.lookup(V)) {
+    if (E && !isa<ConstantExpression>(E) && !isa<VariableExpression>(E)) {
+      auto *LOL = lookupOperandLeader(RTV);
----------------
The second if expression seems to be cheaper, and they seems to be independent. What about switching them?


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:2042
       ++NumGVNNotMostDominatingLeader;
+#if 0
       assert(
----------------
debug?


https://reviews.llvm.org/D32151