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

[Davide Italiano via llvm-commits]

I'd go for `definesNoMemory` (we can easily change later if we don't like it).
I'll commit the refactor and update the patch soon.

On Wed, May 10, 2017 at 12:51 PM, Daniel Berlin <dberlin at dberlin.org> wrote:
>
>
> 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
>>
>>
>>
>