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

[Guozhi Wei via Phabricator via llvm-commits]

Carrot added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2703
     for (auto *I : InstrsToErase) {
-      assert(I->getParent() == BB && "Removing instruction from wrong block?");
       LLVM_DEBUG(dbgs() << "GVN removed: " << *I << '\n');
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > mkazantsev wrote:
> > > Carrot wrote:
> > > > mkazantsev wrote:
> > > > > I think this assert should not fail, and if it fails, you have a bug. `ICF` may keep cached information for it bound to its old parent, and will try to remove it from its new parent. You may get inconsistent state of `ICF` because of it. At least I don't see how you update it.
> > > > > 
> > > > > You should not move instructions. The right approach is to create a new one.
> > > > Add a call to ICF->insertInstructionTo for the new created load instruction. For the deleted instruction ICF->removeInstruction is called in line 2711.
> > > > 
> > > > Maybe a naive question, does Load instruction impact implicit control flow?
> > > Sorry, I didn't formulate the problem I'm seeing correctly. It's not that loads create implicit control flow. It's that by removing this assert, you allow instruction motion here. There is no check that you could only have moved a load, right? So potentially it creates a room for this kind of bugs. 
> > Let's not scatter cache updates across the code. There can be new caches added in the future, not only `ICF` or whatever it is now. We don't want multiple places where we need to update them. This opens doors for bugs.
> > 
> > Any serious reasons to move instruction rather than create a new one?
> Just imagine that someone will accidentally move an ICF instruction in later change. Currently, the assert is protecting us from it. By giving it up, we make mistakes like this harder to find.
Sounds reasonable. 

So now I just create a new load, replace all uses of old load with the new load. The dead old load instruction can be deleted in the next iteration of GVN.


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

https://reviews.llvm.org/D141712