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

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

In https://reviews.llvm.org/D33056#751288, @dberlin wrote:

> I'm pretty sure this is wrong.
>  I'll look into it as soon as i get this machine back together.
>  We actually do set the leader.
>  Or at least, we should -
>
> As the code says:
>
>   // We rely on the code below handling the MemoryAccess change.
>   
>   
>
> We call moveMemoryToNewCongruenceClass below this which does:
>
>   // First, see what happens to the new class
>   if (!NewClass->getMemoryLeader()) {
>     // Should be a new class, or a store becoming a leader of a new class.
>     assert(NewClass->size() == 1 ||
>            (isa<StoreInst>(I) && NewClass->getStoreCount() == 1));
>     NewClass->setMemoryLeader(InstMA);
>   
>
> This should give NewClass a memory leader when the store becomes a leader.




In https://reviews.llvm.org/D33056#751288, @dberlin wrote:

> I'm pretty sure this is wrong.
>  I'll look into it as soon as i get this machine back together.
>  We actually do set the leader.
>  Or at least, we should -
>
> As the code says:
>
>   // We rely on the code below handling the MemoryAccess change.
>   
>   
>
> We call moveMemoryToNewCongruenceClass below this which does:
>
>   // First, see what happens to the new class
>   if (!NewClass->getMemoryLeader()) {
>     // Should be a new class, or a store becoming a leader of a new class.
>     assert(NewClass->size() == 1 ||
>            (isa<StoreInst>(I) && NewClass->getStoreCount() == 1));
>     NewClass->setMemoryLeader(InstMA);
>   
>
> This should give NewClass a memory leader when the store becomes a leader.


Oh, sure it was. I thought it was skipped because for some reason I messed up MemoryUse and MemoryDef.
Anyway.

This is how we lose the memoryleader:
When we change leader to a store, we set the new memory leader to be:

  7 = MemoryPhi({for.body,2},{if.then,3})

Then immediately after we value number that `MemoryPhi`:

  Processing MemoryPhi 12 = MemoryPhi({for.body,4},{if.then,5})
  Memory Phi value numbered to 5 = MemoryDef(12)

In `valueNumberPHI` we call `setMemoryClass` which resets the leader:

  // This may have killed the class if it had no non-memory members
  if (OldClass->getMemoryLeader() == From) {
    if (OldClass->memory_empty()) {
      OldClass->setMemoryLeader(nullptr);
    } else {

and the next instruction we process (the load), looks up for that leader and crashes.


https://reviews.llvm.org/D33056