[PATCH] D33056: [NewGVN] When a store becomes the leader, update the memory leader for the class

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

In https://reviews.llvm.org/D33056#751369, @davide wrote:

> I think we might just be missing a check for store count (as it's done in the other place we call `getNextMemoryLeader()`).
>  Please let me know what you think, and thanks for catching the mistake!
>
>   diff --git a/lib/Transforms/Scalar/NewGVN.cpp b/lib/Transforms/Scalar/NewGVN.cpp
>   index 3c9850b..f7182f2 100644
>   --- a/lib/Transforms/Scalar/NewGVN.cpp
>   +++ b/lib/Transforms/Scalar/NewGVN.cpp
>   @@ -1381,7 +1381,7 @@ bool NewGVN::setMemoryClass(const MemoryAccess *From,
>            NewClass->memory_insert(MP);
>            // This may have killed the class if it had no non-memory members
>            if (OldClass->getMemoryLeader() == From) {
>   -          if (OldClass->memory_empty()) {
>   +          if (OldClass->memory_empty() && OldClass->getStoreCount() == 0) {
>                OldClass->setMemoryLeader(nullptr);
>              } else {
>                OldClass->setMemoryLeader(getNextMemoryLeader(OldClass));
>  
>   fluttershy at SONY-PC C:\Users\fluttershy\work\llvm
>


Oh, yes, I believe you have discovered the root cause.
Sigh.
I really wish it was easy to unify all this handling into one place.

Can you add a definesNoMemory or something to CongruenceClass, and use it here?
It should be memory_empty && storecount == 0

Then we can also replace this check:

  if (OldClass->getStoreCount() != 0 || !OldClass->memory_empty()) {

with it

(This will also force us to make sure the class is in a consistent state. It would also let us add an assert that  that if (CC->definesNoMemory()), then <nothing in ->members() is a store> && and memory_empty()) in a few places.
(though this is an expensive assert)


https://reviews.llvm.org/D33056