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

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

In https://reviews.llvm.org/D42179#978874, @fhahn wrote:

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


Yes

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

Yes, but this should be taken care of by the code mentioned, by adding those ops as dependencies on the instruction.

> It seems like we are missing this case.

We should not be :)

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

The phi of ops can only depend on the expressions that exist for the operands, and those expressions change only if the evaluation of the instruction changes.  In such a case, the instruction will move classes, and hit the class changed case.  It will then mark the operands in the phi of ops as changed, as they are users of that instruction (either additional users, or regular users).    If they are symbolically marked, they should get caught by the other mark call in classchanged.
what is the case where the instructions that make up those operands don't move classes, but the expression still changes anyway? Such a thing should be impossible.

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

VariableExpressions do not actually have classes on their own, and we'll never store them :)
But it still should fall into the classchanged case if it matters.


Repository:
  rL LLVM

https://reviews.llvm.org/D42179