[PATCH] D28192: NewGVN: Fix PR31480, PR31483, PR31499, by rewriting how memory congruence handling works.

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

Thanks for working on this! Some initial comments inline.



================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:776-781
+  DEBUG(dbgs() << "Setting " << *From << " equivalent to ");
+  if (!To)
+    DEBUG(dbgs() << "itself");
+  else
+    DEBUG(dbgs() << *To);
+  DEBUG(dbgs() << "\n");
----------------
This can maybe be expressed using a one-liner with the ternary operator, I find it slightly better, if you think this version is more readable you can keep it as is (I don't have a strong opinion).


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1056-1064
+                   << " and leader " << *(NewClass->RepLeader) << "\n");
       DEBUG(dbgs() << "Hash value was " << E->getHashValue() << "\n");
     } else {
       EClass = lookupResult.first->second;
+      if (isa<ConstantExpression>(E))
+        assert(isa<Constant>(EClass->RepLeader) &&
+               "Any class with a constant expression should have a "
----------------
These two changes look like they can be committed in isolation (the improved debug message && the assertion), unless the latter fires without this change.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1104-1110
+        // If this is a MemoryDef, we need to update the equivalence table. If
+        // we
+        // determined the expression is congruent to a different memory state,
+        // use that different memory state.  If we determined it didn't, we
+        // update
+        // that as well. Note that currently, we do not guarantee the
+        // "different" memory state dominates us.  The goal is to make things
----------------
This comment looks formatted in a strange way -- it could just be phab, in that case please ignore.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1114
+        // Right now, the only way they can be equivalent is for store
+        // expresions.
+        if (!isa<MemoryUse>(MA)) {
----------------
s/expresion/expressions/


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1591-1594
+// FIXME: Move this to expensive checks when we are satisfied with NewGVN
+#ifndef NDEBUG
+  verifyMemoryCongruency();
+#endif
----------------
Yay for the verifier. We should keep this under `NDEBUG` anyway as there's no bot handling LLVM_EXPENSIVE_CHECKS (because, such a shame, there are still tests failing in that configuration).


https://reviews.llvm.org/D28192