[PATCH] D42179: [NewGVN] Re-evaluate phi of ops after moving an instr to new class

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In https://reviews.llvm.org/D42179#978618, @dberlin wrote:

> Note also that we mark all operands that are part of the expression as Deps. If the leader of those operands changed, they would already be marked as changed as Users through the addAdditionalUsers part we do.


I had a look at how `performSymbolicEvaluation` adds additional users in case it managed to find existing values that could be used to simplify the value we evaluate.

In `findLeaderForInst`, we evaluate a temporary instruction (based on an original instruction OI). If we find an existing value VE to simplify this temporary instruction, we currently do not track that OI (which the temporary is based on) depends on VE, as `checkSimplificationResults` only adds additional dependencies for non temporary instructions. But if VE moves classes, the symbolic evaluation could change and thus we might find a suitable leader, but we do not detect that currently. I am probably missing more again, but from your responses I *think*  that in that case we should have recorded an additional dependency for OI, to cover that case. It would be great if you could let me know if that makes sense.

(The invariant checks I have still need some cleanup...)


Repository:
  rL LLVM

https://reviews.llvm.org/D42179