[PATCH] D141712: [GVN] Improve PRE on load instructions

[Guozhi Wei via Phabricator via llvm-commits]

Carrot added a comment.

The compiler crash didn't occur in my last commit, but occurred in this commit. It is caused by the interaction between the new instruction deletion method and the implementation of replaceValuesPerBlockEntry.

Consider the following case

  ; case1.
  
  bb1:
     ...
     ; NewLoad is added here.
     br %cond1, label %bb2, label %bb3
  
  bb2:
     OldLoad
     ...
     br label %bb3
  
  bb3:
     Load
     ...

Once we decide to do the transformation, we take the following steps

- Insert NewLoad into bb1.
- Replace ValuesPerBlock(OldLoad->getParent(), OldLoad) with ValuesPerBlock(OldLoad->getParent(), NewLoad), so we'll get ValuesPerBlock(%bb2, NewLoad).
- Delete instruction OldLoad, and replace all its uses with NewLoad.
- Replace instruction Load with a new PHI instruction, required information is from ValuesPerBlock.

If we change the case to

  ; case2.
  
  bb1:
     ...
     ; NewLoad is added here.
     br %cond, label %bb2, label %bb3
  
  bb2:
     OldLoad
     ...
     br %cond2, label %bb5, label %bb4
  
  bb4:
     ...
     br label %bb3
  
  bb3:
     Load
     ...

We take the following steps to do the transformation

- Insert NewLoad into bb1.
- Try to replace ValuesPerBlock(OldLoad->getParent(), OldLoad) with ValuesPerBlock(OldLoad->getParent(), NewLoad). But we don't have an entry for ValuesPerBlock(%bb2, OldLoad), instead we have an entry ValuesPerBlock(%bb4, OldLoad),  and it is not changed.
- Delete instruction OldLoad, and replace all its uses with NewLoad.
- Replace instruction Load with a new PHI instruction, required information is from ValuesPerBlock. Then we get the value OldLoad from %bb4 again, because OldLoad is already deleted, it's an invalid value, and causes crash.

In my last version, OldLoad isn't deleted immediately, the reference to it in the PHI instruction is valid and correct. In the next iteration of GVN, OldLoad can be found as fully redundant, then it is  deleted and the use in PHI instruction is updated to use NewLoad.

It should be fixed in replaceValuesPerBlockEntry, every entry with the value OldLoad should be changed to NewLoad. Because OldLoad is dominated by NewLoad, so this change is safe.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141712/new/

https://reviews.llvm.org/D141712