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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

This looks good to me, but I think needs a couple more tests:

- Positive test for nuw instead of nsw.
- Negative test for zero start value.
- Negative test for negative multiply. (We can't really test zero multiply, as it will get folded away.)
- Negative test for negative start value -- alternatively, you can slightly extend this patch and split the start value check between add and mul: Add requires strictly positive start, but for mul non-zero is sufficient, I believe.



================
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)) {
----------------
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()) {
  ...
}
```


================
Comment at: llvm/test/Analysis/ValueTracking/monotonic-phi.ll:79
+  %cmp = icmp eq i8 %add, 0
+  ; CHECK-NOT: ret i1 false
+  ret i1 %cmp
----------------
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.


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

https://reviews.llvm.org/D99069