[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#978616, @dberlin wrote:

> At a glance, i don't believe this is correct.
>  The ClassChanged part above should have already done this.
>  If not, can you detail me how it is happening?


What follows is my initial understanding after starting having a look at NewGVN and I am probably missing a couple of things. I would greatly appreciate any thoughts and pointers :)

I think the problem is the following: ExpressionToPhiOfOps is used to re-process the phi-of-ops, in case the leader of the symbolic (gvn) expression changed. In the `ClassChanged` case, we currently re-process any phi-of-ops that where using expression `E`, as its leader has changed. But by moving `I` to a new class, the leader of all variable expressions of `I` might has changed too, e.g. because `I` was not the leader of the old class.

It seems like we are missing this case. So if during phi-of-ops processing, we failed with symbolic (gvn) expression E1(I), and E != E1(I), we do not re-process the phi-of-ops instruction, whereas I think we should re-process all phi-of-ops instructions, which depended on any symbolic expression involving I. "depend" here might be not the best term, I mean the symbolic expression we evaluated in `findLeaderForInst`.

> 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 think this is a symbolic dependency, rather than an additional use , as it is no operand of the new expressions.

Also, it seems like the debug output "New class .... for expression ..." could be misleading in some cases. For example, the code path also gets hit if `E` is a `VariableExpression` with an existing class and the value `I` is just moved to the existing `EClass`, without the class of `E` being changed.


Repository:
  rL LLVM

https://reviews.llvm.org/D42179