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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

This looks mostly good. Only remaining issue is that we need to aggressively limit recursion when it comes to phi nodes. What most other ValueTracking code does is to use `MaxAnalysisRecursionDepth - 1` as the depth for recursion over phis, which means that only one more level of recursion is allowed. If that still covers your motivating case, then I'd suggest to follow that pattern. If that doesn't cover your motivating case, then the alternative here would be to only allow full recursion over one pair of phi operands, and only check that the others are trivially non-equal (i.e. distinct ConstantInts). That should definitely cover your case, and would also be in line with existing practice for isKnownNonEqual(). Either way is fine by me.



================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2558
+    Query RecQ = Q;
+    RecQ.CxtI = PN1;
+    if (!isKnownNonEqual(IV1, IV2, Depth + 1, RecQ))
----------------
This should be using `IncomBB->getTerminator()` as the context, not the phi node itself.


================
Comment at: llvm/test/Analysis/ValueTracking/monotonic-phi.ll:305
+  ret i1 %cmp
+}
----------------
We should also have a negative test for the case where the start value is the same (e.g. 2, 2 instead of 2. 3).

known-non-equal.ll might be a better fit for these tests.


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

https://reviews.llvm.org/D98422