[PATCH] D99069: [ValueTracking] Handle increasing mul recurrence in isKnownNonZero()

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2455
+    Value *Start = nullptr, *Step = nullptr;
+    if (Q.IIQ.UseInstrInfo && matchSimpleRecurrence(PN, BO, Start, Step)) {
+      if (ConstantInt *StartCst = dyn_cast<ConstantInt>(Start)) {
----------------
nikic wrote:
> Style suggestion: I would write this using m_APInt matchers, which allows you to avoid some of the nesting:
> ```
> Value *Start, *Step;
> const APInt *StartC, *StepC;
> if (Q.IIQ.UseInstrInfo && matchSimpleRecurrence(PN, BO, Start, Step) &&
>     match(Start, m_APInt(StartC)) && match(Step, m_APInt(StepC)) &&
>     StartC->isStrictlyPositive()) {
>   ...
> }
> ```
Yep, it looks better. I will update it.


================
Comment at: llvm/test/Analysis/ValueTracking/monotonic-phi.ll:79
+  %cmp = icmp eq i8 %add, 0
+  ; CHECK-NOT: ret i1 false
+  ret i1 %cmp
----------------
nikic wrote:
> jaykang10 wrote:
> > nikic wrote:
> > > Why does this return false? Isn't this supposed to be a negative test case, due to lack of `nuw`/`nsw`?
> > Yep, it is negative test case due to lack of `nuw`/`nsw`. It could be better to check `CHECK: ret i1 %cmp`. I will update it.
> Ah, I guess I misread a `CHECK-NOT` as `CHECK` here? I've regenerated the check lines for this test in https://github.com/llvm/llvm-project/commit/b7aae9fab14540ad3b4ccda8a5f3a7284f404e63, so you can generate the expected output using llvm/utils/update_test_checks.py.
Yep, I will use the script after adding tests.


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

https://reviews.llvm.org/D99069