[PATCH] D32252: [GVN] Add phi-translate for scalarpre as a temporary solution

[Wei Mi via llvm-commits]

On Wed, Apr 19, 2017 at 11:26 PM, Daniel Berlin via Phabricator
<reviews at reviews.llvm.org> wrote:
> dberlin added a comment.
>
>
>
>>> I suspect you will have issues with backedges with this approach. I'm pretty sure it will fail in some cases to fixpoint (but maybe gvn is not strong enough to make that happen)
>>>  At the very least, you don't want this to transform:
>>
>> There is code in performScalarPRE checking whether the predecessor is the same as current bb and giving up if it is true. I tried small testcase and it seemed fine. I will pay attention to it when exercise it with more tests.
>
> This code will only work for the trivialest of cases :)
>
> GVN computes the RPO ordering.
> The easiest way to detect backedges is to densemap that.
> An edge x->y is a backedge if RPO[x] >= RPO[y]
>
> (This is because in a postorder traversal, you are guaranteed it will find the edges that lead to a block, before that block, since it will process all children to completion first
> So in a PO, PO[x] <= PO[y], since it will process all such children of y before processing y.
> In RPO, it's just reversed)
>

You are right, that part of code only works for trivialest case. I do
some tracing and find the following part of code handling the general
case with cycle:

2091     uint32_t TValNo = VN.phiTranslate(P, ValNo);
2092     Value *predV = findLeader(P, TValNo);
2093     if (!predV) {
2094       predMap.push_back(std::make_pair(static_cast<Value *>(nullptr), P));
2095       PREPred = P;
2096       ++NumWithout;
2097     } else if (predV == CurInst) {
===> handle the general case with cycle.
2098       /* CurInst dominates this predecessor. */
2099       NumWithout = 2;
2100       break;
2101     } else {
2102       predMap.push_back(std::make_pair(predV, P));
2103       ++NumWith;
2104     }

An example:
BB0:
  ...
BB1:
  i = phi(0, BB0, inc, BB2)
  inc = i + 1;
   ...
BB2:
  condbr BB1, BB3.
BB3:
  ...

VN.phiTranslate(inc, BB2) first convert "i + 1" to "inc + 1", then it
finds no existing value corresponding to "inc + 1" so it simply
returns the original value of inc.
Because TValNo is the same as ValNo, findLeader will return predV
which is the same as CurInst. Then as the comment says, it knows
CurInst dominates the predecessor, i.e., a cycles is found, so it
gives up PRE.

When findLeader finds the leader value in the predecessor is CurInst
itself, there must be a loop. If findLeader find another value that is
different from but still dominated by CurInst, the value would have
been eliminated in GVN.

>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D32252
>
>
>