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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2566
+
+      IsNonEqual = false;
+      break;
----------------
jaykang10 wrote:
> lebedev.ri wrote:
> > Now that it is in a separate function, early return can be used.
> Yep, I will update it.
You need to adjust the context instruction when recursing into phis. Grep for `RecQ` in this file.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2554
+  if (PN1->getParent()->hasNPredecessors(0))
+    return false;
+
----------------
Why is it necessary to explicitly handle this case? It should be fine to report that two empty phis are non-equal (they're poison, so they can be either equal or not equal, or both at the same time).


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2569
+    // to the PHI's block.
+    if (Q.DT != nullptr && Q.DT->dominates(PN1->getParent(), Pred)) {
+      // loop:
----------------
Why do we still need this code at all? With the improved handling for multiplies, we shouldn't need this special check anymore. Your tests still pass if I delete this code entirely.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2654
+      // a PHI and another one isn't.
+      if (isNonEqualPHIs(PN1, PN2, Depth + 1, Q))
+        return true;
----------------
You are incrementing the Depth twice, once here, and once inside isNonEqualPHIs. I think you want to drop the increment here.


================
Comment at: llvm/test/Analysis/ValueTracking/non-equal-two-phis.ll:1
+; RUN: opt < %s -basic-aa -instcombine -S 2>&1 | FileCheck %s
+
----------------
These tests are still testing the isKnownNonEqual() functionality via load forwarding and BasicAA. It would be better to make these `-instsimplify` tests that that fold an `icmp` to true or false.


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

https://reviews.llvm.org/D98422