[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:2521
+  bool HasSameIncomingBB = true;
+  if (PN1 && PN2 && PN1->getParent() == PN2->getParent() && Q.DT != nullptr) {
+    // Check the incoming blocks of two PHIs are same.
----------------
nikic wrote:
> If the parents are the same, then you don't need to check the incoming blocks, they must be the same.
> 
> You also don't need to check PN1 and PN2 for null, they are expected to be non-null in this function.
You are right! I will update it.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2548
+      // to the PHI's block.
+      if (Q.DT->dominates(PN1->getParent(), Pred)) {
+        // loop:
----------------
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...


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2553
+        //   ...
+        //   %inc = add i32 %cmp.0, 1
+        //   br label %loop
----------------
nikic wrote:
> `%inc` is not used in the phi nodes, so it's not clear what the relation is. Maybe you meant to replace `%mul` with `%inc`?
Sorry for typo... `%inc` is correct.


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

https://reviews.llvm.org/D98422