[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.

Thank you very much for providing such a detailed response, that's really helpful and I really appreciate it :)

I'll try to add some additional checks for invariants to pin down where something went wrong. I think either of the following conditions should  hold just before the end of the `if (ClassChanged || LeaderChanged) {` block, for all pairs (E, I) in ExpressionToPhiOfOps:

- the leader of the class for expression E (getClassForExpression) is not changed by `moveValueToNewCongruenceClass`
- I is in TouchedInstructions or any instruction with I as one of its users is in TouchedInstructions

In other words, if the leader for expression E changes, I should get re-processed. Does that make sense?

> Your patch actually just marks the phi of ops expressions as changed *even when all that happened was a leader change, and the class did not change*.

It is sufficient to only have the additional `markPhiOfOpsChanged(createVariableOrConstant(I))` if ClassChange, but I agree it is quite heavy handed and not complete.


Repository:
  rL LLVM

https://reviews.llvm.org/D42179