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

[Daniel Berlin via Phabricator via llvm-commits]

dberlin marked 2 inline comments as done.
dberlin added inline comments.


================
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.
----------------
davide wrote:
> *sigh*.
Yeah, i think i'm going to try to push that in, because i think this invariant may discover more bugs.

Additionally, it *should* us turn back on a much simpler invariant for the memory accesses:
back in the day, we wanted to turn on the invariant that two stores with equivalent memory states must be in the same congruence class, and that is now true again (AFAIK)

Except for the above case :(



================
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
----------------
davide wrote:
> `#if 1` can go away.
Yup


https://reviews.llvm.org/D29195