[PATCH] D53786: [AliasSetTracker] Actually delete instructions from the AliasSetTracker.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

I agree with all your points! Thank you for following up.

The issue is, we have this "deleteValue" which, AFAICT, is only called from LICM, only when removing dead instructions.

But, the AST has pointers + unknown instructions. So calling deleteValue(I) never removes pointers, only the unknown instructions.
I was attempting to see what happens if I do make it remove the pointers, when given an Instruction. But that's not nearly enough or, in my mind, correct.

1. It's not enough, because when adding an instruction we may add more than a pointer (such as arguments added for a CallSite), just like you said.
2. It's not correct, because when removing an instruction with pointer Ptr, we're removing Ptr from the set, even if there may be other instructions modifying that Ptr.

Hence my surprise that this patch is not breaking anything :)

Now, when actually removing pointers in LICM we're now invalidating the iterator over alias sets, hence the change you noticed that's worrying. I cannot see other cases where this would happen.
Before, we were not removing anything, because that part is doing promotion, and we're never promoting unknown instructions, hence the calls to deleteValue in that part of LICM are pretty much no-ops.

Broader question is, as you very well put it: why are we removing things? Or better phrased: why are we attempting to remove things?
If we really care about removed instructions to reflect in the AST, this patch needs a rewrite.
If we don't care about removed instructions, then the "deleteValue()" API and its uses can *mostly* all go away.

(I can see one potential use case: call deleteValue(I) when sinking/hoisting if the instruction is dead, which may make an alias set free of unknown instructions and hence promotable; but then deleteValue could be renamed deleteUnknownInstruction and it would not be called anywhere in the promotion section, so we wouldn't worry about potential iterator invalidations.)


Repository:
  rL LLVM

https://reviews.llvm.org/D53786