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

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

This is really nice =)
I'll do some performance testing myself tomorrow, some comments in the meanwhile.



================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:725-729
+  if (From == To)
+    return true;
+  auto *FromDTN = DT->getNode(From);
+  auto *ToDTN = DT->getNode(To);
+  return RPOOrdering.lookup(FromDTN) >= RPOOrdering.lookup(ToDTN);
----------------
I see how you're looking at the RPO ordering to determine if it's a backedge, but for people reading the code for the first time, maybe we can add a longer comment (maybe an example?)


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:741-743
+  if (Result)
+    return Result;
+  return TempToMemory.lookup(I);
----------------
Style'ish, ternary operator (up to you, can be a later cleanup).


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:2042
       ++NumGVNNotMostDominatingLeader;
+#if 0
       assert(
----------------
Prazek wrote:
> debug?
DEBUG or comment out.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:2087
   // If it's not a memory use, set the MemoryAccess equivalence
-  auto *InstMA = dyn_cast_or_null<MemoryDef>(MSSA->getMemoryAccess(I));
+  auto *InstMA = dyn_cast_or_null<MemoryDef>(getMemoryAccess(I));
   bool InstWasMemoryLeader = InstMA && OldClass->getMemoryLeader() == InstMA;
----------------
I think hiding getMemoryAccess in an helper is nice, but can be split.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:3345
   }
-
   return AnythingReplaced;
----------------
unrelated?


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:3361
   DenseMap<const Value *, unsigned int> UseCounts;
-  for (CongruenceClass *CC : reverse(CongruenceClasses)) {
+  for (auto *CC : reverse(CongruenceClasses)) {
     // Track the equivalent store info so we can decide whether to try
----------------
unrelated?


https://reviews.llvm.org/D32151