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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/GVNSink.cpp:884
     if (I != I0)
       I->replaceAllUsesWith(I0);
   foldPointlessPHINodes(BBEnd);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96106