[PATCH] D98422: [ValueTracking] Handle two PHIs in isKnownNonEqual()

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2566
+
+      IsNonEqual = false;
+      break;
----------------
nikic wrote:
> 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.
Yep, I will update it.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2554
+  if (PN1->getParent()->hasNPredecessors(0))
+    return false;
+
----------------
nikic wrote:
> 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).
There was a regression. Below loop uses  `predecessors(PN1->getParent())` as range of loop. On optimization pass, there was a unreachable PN's block. In this case, the block did not have predecessors and it caused wrong return value. Let me update it.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2569
+    // to the PHI's block.
+    if (Q.DT != nullptr && Q.DT->dominates(PN1->getParent(), Pred)) {
+      // loop:
----------------
nikic wrote:
> 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.
You are right!!! I will delete it.


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


================
Comment at: llvm/test/Analysis/ValueTracking/non-equal-two-phis.ll:1
+; RUN: opt < %s -basic-aa -instcombine -S 2>&1 | FileCheck %s
+
----------------
nikic wrote:
> 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.
Yep, I will add the tests and delete this test.


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

https://reviews.llvm.org/D98422