[PATCH] D28594: NewGVN: Move leaders around properly to ensure we have a canonical dominating leader. Fixes PR 31613.

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

Some comments inline, this looks correct but I'd like to run some tests tomorrow morning once I'm back to my workstation. Quick question, I can see how a set/vector can help here but I didn't think about the fib heap. Why did you mention such a variant and not let's say binomial/trinomial?



================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:148
+  // is not sorted and is expensive to keep sorted all the time.
+  std::pair<Value *, unsigned int> NextLeader = {nullptr, ~0};
+
----------------
`~0U` ?


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1074
+  if (I == OldClass->NextLeader.first)
+    OldClass->NextLeader = {nullptr, ~0};
+
----------------
`~0U` ?


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1080-1084
+  assert(
+      !isa<Instruction>(NewClass->RepLeader) || !NewClass->RepLeader ||
+      I == NewClass->RepLeader ||
+      !DT->properlyDominates(
+          I->getParent(), cast<Instruction>(NewClass->RepLeader)->getParent()));
----------------
Can you please add a messahge to this assertion?


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1131
+      std::pair<Value *, unsigned> MinDFS = {nullptr, ~0};
+      for (const auto X : OldClass->Members) {
+        auto DFSNum = InstrDFS.lookup(X);
----------------
X can probably be passed by reference?
I'm always in doubt with `auto` (I try to never rely on the implicit referenceness/pointerness of auto and always be explicit to avoid accidental copies)


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1203
+      moveValueToNewCongruenceClass(I, IClass, EClass);
+
+    markUsersTouched(I);
----------------
spurious whiteline


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1220
     }
-  } else if (StoreInst *SI = dyn_cast<StoreInst>(V)) {
+  } else if (StoreInst *SI = dyn_cast<StoreInst>(I)) {
     // There is, sadly, one complicating thing for stores.  Stores do not
----------------
auto *


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1562-1563
 // congruence classes.  Note that this checking is not perfect, and is currently
-// subject to very rare false negatives. It is only useful for testing/debugging.
+// subject to very rare false negatives. It is only useful for testing and
+// debugging.
 void NewGVN::verifyMemoryCongruency() const {
----------------
unrelated I assume?


================
Comment at: test/Transforms/NewGVN/pr31613.ll:68
+}
+; ModuleID = 'pr31613-3.ll'
+
----------------
this moduleId here crept in?


https://reviews.llvm.org/D28594