[PATCH] D43865: [NewGVN] Create new ValuePHI node, if the number of operands does not match.

Daniel Berlin via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Mar 2 13:54:50 PST 2018


dberlin added a comment.

So, we are definitely "doing it wrong".

This code is definitely broken in the case that multiple operands are defined by phi of ops, and they are not in the same block.
In fact, i think it's now broken with all multiple phis, unless the phis are in the same block. It is just getting lucky because it is getting blocked from using them.

It also badly needs refactoring because of how long it took to notice this.

Here is what is happening:

Let me start with context. The reason it differentiates the phi block from the instruction block is to catch the case the instruction operand is dependent on a dominating phi

ie
bb0
 a = phi
 goto bb1

bb1:
 b = add a, 1

These are  to transform valid phi of ops.
Here it will evaluate add a, 1 for each predecessor block of *a*

The limitation on the code the way it is currently written, however, is that it assumes *all* phis for the instruction are in the same block.
Thus, it only evaluates the phiblock once, and generates the operation in the phi block and sees what happens.
You can see this in the code if you look hard enough. the loop over the original instruction operands will return an expression or nullptr the second it hits an operand that is a phi.
The only reason it translates each phi in the expression when they are in the same block is because it does ```Op = Op->DoPHITranslation(PHIBlock, PredBB);``` for each pred.
Which will handle it regardless of whether it's the original phi or not.

If you give it something like:
bb:

  br label %bb1

bb1:

  %tmp = phi i32 [ 0, %bb ], [ %tmp10, %bb6 ]

bb6:

  %tmp7 = phi i32 [ 0, %bb1 ], [ 1, %bb4 ], [ 1, %bb5 ]
  %tmp9 = or i32 %tmp, %tmp7

It will try to evaluate tmp9 in the tmp1 block as or i32 <phi pred>, %tmp7, and either

1. fail to find anything
2. fail safety checks
3. get unlucky, find something, and crash

At least as far as i've been able to reproduce, the only case #3 occurs is when we've made phi of ops for the operands, as you see in this case.
There, it gets past the safety checking/etc, and all the way to the valuephi creation, but now the value phi is in the wrong block.

The correct way to handle all of this is unfortunately recursive.
We'd have to translate through each phi and phi block that occurs as an operand, and evaluate the operation in every possible predecessor of those, and merge those results.

This is unlikely to be worth it ATM, it seems incredibly uncommon.
It would be easy, however, if we refactored this code sanely.

For now, safety wise, we should verify all phis for an op are in the same block and give up if not.
I'll send along a patch to fhahn that works but is otherwise untested.


https://reviews.llvm.org/D43865





More information about the llvm-commits mailing list