[PATCH] D96106: [GVNSink][Fix] Unregister assumptions that are replaced (PR49043)

Sat Feb 6 09:17:53 PST 2021

jdoerfert added inline comments.

================
Comment at: llvm/lib/Transforms/Scalar/GVNSink.cpp:884
     if (I != I0)
       I->replaceAllUsesWith(I0);
   foldPointlessPHINodes(BBEnd);
----------------
nikic wrote:
> jdoerfert wrote:
> > @nikic This is where one assume is replaced with another, e.g., in the attached test.
> Okay, I think now I get it. The problem is that we use a WeakTrackingVH, which means that it will follow RAUW, and we do multiple replacements to the same assume.
> 
> This makes me wonder though: Can't we just replace the WeakTrackingVH with a WeakVH? Doing RAUW on assumes is not really meaningful because the instruction has no return value. I believe it's fine to just ignore RAUW by using WeakVH, and then in this case, the old assumes would get nulled out by the eraseFromParent below.
Trying that now. Avoiding dangling references is still kinda nice but I'm OK with a minimal fix as well. Either way.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96106