[PATCH] D98422: [Alias] Add a ah-hoc pattern with two PHI for isKnownNonEqual

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2548
+      // to the PHI's block.
+      if (Q.DT->dominates(PN1->getParent(), Pred)) {
+        // loop:
----------------
nikic wrote:
> jaykang10 wrote:
> > nikic wrote:
> > > I don't understand why this code is needed. For your example you are comparing `%cmp.0` and `%mul = mul nsw i32 %cmp.0, 2`, which should already be recognized as non-equal. What is this backedge check for?
> > There is a slightly different case as below.
> > 
> > ```
> >         // loop_header:
> >         //   %cmp.0 = phi i32 [ 3, %entry ], [ %inc, %loop_body ]
> >         //   %pos.0 = phi i32 [ 1, %entry ], [ %cmp.0, %loop_body ]
> >         //   ...
> >         // loop_body:
> >         //   ...
> >         //   %inc = add i32 %cmp.0, 1
> > ```
> > If the `%cmp.0` is ahead of `%pos.0`, it is not easy to say that `%cmp.0` is not same with `%pos.0`. Following PHI instruction's definition which @dmgreen mentioned, the incoming value comes from incoming block. In this case, if there is backedge from incoming block, I think we can say `%pos.0` has previous iteration's `%inc` value and `%com.0` is not same with `%pos.0`. That was the idea for backedge. I am sorry for making you confused. If you feel something wrong from it, please let me know... I could be wrong...
> I don't think this reasoning is generally valid, at least not without additional checks. Consider this counter-example:
> 
> ```
> %p1 = phi(1, %p1.next)
> %p2 = phi(3, %p1)
> %p1.next = or %p1, 2
> ```
> 
> On the first iteration `%p1 = 1, %p2 = 3`, on the second `%p1 = 3, %p2 = 1`, on the third `%p1 = 3, %p2 = 3`, so the phis are not known non-equal.
Yep, that's the reason why I added below FIXME comments. I have wanted to get the idea to fix it and that is the main thing for this patch.

```
        if (PN1 == IV2 || PN2 == IV1) {
          // FIXME: Do we have to check the IdxVal is not constant in detail?
          continue;
        }
```
I think I need to explain the whole situation. If you compile summary's c code, the two PHI's order is first `%pos.0` and second `%cmp.0`. However, the on loop rotation pass, SSAUpdater adds PHIs to newHeader using `BB->front()` and the order of PHIs is changed. First time, I thought I need to fix the SSAUpdater but @dmgreen pointed out the order of PHIs is not invalid from PHI's definition on LLVM IR ref. I have checked the PHIElimination pass removes the PHI correctly following the LLVM IR ref. It means that we can meet any order of PHIs on LLVM IR code. If we can not detect the PHIs are not same, we can return `NoAlias` from something like above c example and it blocks optimizations... I thought something like SCEV to resolve it but it could be too expensive for compile time.... The main purpose of this patch was to get the idea to resolve this issue... I am sorry for unclear patch and explanation. If you have idea about this issue, please let me know.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98422/new/

https://reviews.llvm.org/D98422