[PATCH] D29195: NewGVN: Fix bug exposed by PR31761

[Davide Italiano via Phabricator via llvm-commits]

davide accepted this revision.
davide added a comment.
This revision is now accepted and ready to land.

I reviewed this as part of https://reviews.llvm.org/D29149 (duplicated the comments here)



================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:707-711
+  // FIXME: We need to audit all the places that current set a nullptr To, and
+  // fix them.  There should always be *some* congruence class, even if it is
+  // singular.  Right now, we don't bother setting congruence classes for
+  // anything but stores, which means we have to return the original access
+  // here.  Otherwise, this should be unreachable.
----------------
*sigh*.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:768
   MemoryAccess *StoreAccess = MSSA->getMemoryAccess(SI);
+#if 1
   // See if we are defined by a previous store expression, it already has a
----------------
`#if 1` can go away.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1168-1182
+
+    // If we destroy the old access leader, we have to effectively destroy the
+    // congruence class.  When it comes to scalars, anything with the same value
+    // is as good as any other.  That means that one leader is as good as
+    // another, and as long as you have some leader for the value, you are
+    // good.. When it comes to *memory states*, only one particular thing really
+    // represents the definition of a given memory state.  Once it goes away, we
----------------
Thanks for this very explicative comment, I think it's very useful. Nit, dot at the end of the last sentence.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1247-1252
+// This gets filled in by the moveValue
+#if 0
+        // First store is the leading memory access
+        NewClass->RepMemoryAccess = MSSA->getMemoryAccess(SI);
+#endif
       } else {
----------------
This can be removed.


https://reviews.llvm.org/D29195