[ValueTracking] Handle increasing mul recurrence in isKnownNonZero()

[jaykang10]

Summary

Detect a recurrence that monotonically increases with mul.

[nikic]

It might make sense to rephrase this code in terms of matchSimpleRecurrence().

[jaykang10]

In D99069#2641038, @nikic wrote:

It might make sense to rephrase this code in terms of matchSimpleRecurrence().

Ah! Thanks for letting me know. I did not know that. Let me try.

[jaykang10]

Use matchSimpleRecurrence

[jaykang10]

In D99069#2641050, @jaykang10 wrote:

In D99069#2641038, @nikic wrote:

It might make sense to rephrase this code in terms of matchSimpleRecurrence().

Ah! Thanks for letting me know. I did not know that. Let me try.

@nikic I have updated the patch with matchSimpleRecurrence. Please check it.

[nikic]

llvm/lib/Analysis/ValueTracking.cpp

2459

I think the matchSimpleRecurrence() check should be higher, e.g. PN->getNumIncomingValues() == 2 is already part of it.

2461

I'd rename R/L to Start and Step.

2468

Why does this exclude C = 1? {1,*,2}<nuw> is still non-zero. The one value is important for inequality, but here we're checking for non-zero.

2471

Again, this isOne() check looks unnecessary to me.

llvm/test/Analysis/ValueTracking/monotonic-phi.ll

61

Why is this or needed?

79

Why does this return false? Isn't this supposed to be a negative test case, due to lack of nuw/nsw?

[jaykang10]

llvm/lib/Analysis/ValueTracking.cpp

2459

Yep, you are right. I will move it up.

2461

Yep, I will rename them.

2468

You are right!!! We are checking non-zero. I will update it.

2471

Yep, I will update it.

llvm/test/Analysis/ValueTracking/monotonic-phi.ll

61

We do not need it. I will remove it.

79

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.

[jaykang10]

Following comments of @nikic, updated code.

[nikic]

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.

llvm/lib/Analysis/ValueTracking.cpp

2462

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

llvm/test/Analysis/ValueTracking/monotonic-phi.ll

79

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.

[jaykang10]

In D99069#2642605, @nikic wrote:

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.

Yep, let me add more tests.

[jaykang10]

llvm/lib/Analysis/ValueTracking.cpp

2462

Yep, it looks better. I will update it.

llvm/test/Analysis/ValueTracking/monotonic-phi.ll

79

Yep, I will use the script after adding tests.

[jaykang10]

Added more tests

[nikic]

LGTM

[jaykang10]

In D99069#2645965, @nikic wrote:

LGTM

Thanks @nikic If possible, can you also review https://reviews.llvm.org/D98422 please? I think it could also be ready to merge.

llvm/lib/Analysis/ValueTracking.cpp

else if (const PHINode *PN = dyn_cast<PHINode>(V)) {
     // Try and detect a recurrence that monotonically increases from a
     // starting value, as these are common as induction variables.
     if (PN->getNumIncomingValues() == 2) {
       Value *Start = PN->getIncomingValue(0);
       Value *Induction = PN->getIncomingValue(1);
       if (isa<ConstantInt>(Induction) && !isa<ConstantInt>(Start))
         std::swap(Start, Induction);
       if (ConstantInt *C = dyn_cast<ConstantInt>(Start)) {
         if (!C->isZero() && !C->isNegative()) {
-          ConstantInt *X;
-          if (Q.IIQ.UseInstrInfo &&
-              (match(Induction, m_NSWAdd(m_Specific(PN), m_ConstantInt(X))) ||
-               match(Induction, m_NUWAdd(m_Specific(PN), m_ConstantInt(X)))) &&
-              !X->isNegative())
+          BinaryOperator *BO = nullptr;
+          Value *R = nullptr, *L = nullptr;
+          if (Q.IIQ.UseInstrInfo && matchSimpleRecurrence(PN, BO, R, L)) {
+            if (auto StepCst = dyn_cast<ConstantInt>(L)) {
+              if (BO->getOpcode() == Instruction::Add &&
+                  (BO->hasNoUnsignedWrap() || BO->hasNoSignedWrap()) &&
+                  !StepCst->isNegative())
+                return true;
+              if (BO->getOpcode() == Instruction::Mul && !C->isOne() &&
+                  (BO->hasNoUnsignedWrap() || BO->hasNoSignedWrap()) &&
+                  !StepCst->isNegative() && !StepCst->isZero() &&
+                  !StepCst->isOne())
                 return true;
             }
           }
         }
+      }
+    }
     // Check if all incoming values are non-zero using recursion.
     Query RecQ = Q;
     unsigned NewDepth = std::max(Depth, MaxAnalysisRecursionDepth - 1);
     return llvm::all_of(PN->operands(), [&](const Use &U) {
       if (U.get() == PN)
         return true;
       RecQ.CxtI = PN->getIncomingBlock(U)->getTerminator();
       return isKnownNonZero(U.get(), DemandedElts, NewDepth, RecQ);

llvm/test/Analysis/ValueTracking/monotonic-phi.ll

loop:
   %cmp1 = icmp eq i8 %A, %n
   br i1 %cmp1, label %exit, label %loop
 exit:
   %add = or i8 %A, %r
   %cmp = icmp eq i8 %add, 0
   ; CHECK: ret i1 false
   ret i1 %cmp
 }
+
+; CHECK-LABEL: @test4
+define i1 @test4(i8 %p, i8* %pq, i8 %n, i8 %r) {
+entry:
+  br label %loop
+loop:
+  %A = phi i8 [ 2, %entry ], [ %next, %loop ]
+  %next = mul nsw i8 %A, 2
+  %cmp1 = icmp eq i8 %A, %n
+  br i1 %cmp1, label %exit, label %loop
+exit:
+  %add = or i8 %A, %r
+  %cmp = icmp eq i8 %add, 0
+  ; CHECK: ret i1 false
+  ret i1 %cmp
+}
+
+; CHECK-LABEL: @test5
+define i1 @test5(i8 %p, i8* %pq, i8 %n, i8 %r) {
+entry:
+  br label %loop
+loop:
+  %A = phi i8 [ 2, %entry ], [ %next, %loop ]
+  %next = mul i8 %A, 2
+  %cmp1 = icmp eq i8 %A, %n
+  br i1 %cmp1, label %exit, label %loop
+exit:
+  %add = or i8 %A, %r
+  %cmp = icmp eq i8 %add, 0
+  ; CHECK-NOT: ret i1 false
+  ret i1 %cmp
+}