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

[Daniel Berlin via llvm-commits]

On Wed, May 10, 2017 at 12:50 PM, Daniel Berlin via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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
>

In particular, this check is really !definesNoMemory

We could also instead add definesMemory() and use it directly here, and
then the place you are fixing is !definesMemory().

Not sure which we'd rather have.


> (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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170510/073fcfe3/attachment.html>