[llvm] [ValueTracking] Clarify KnownBits recurrence code (PR #222266)
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 9 03:19:00 PDT 2026
https://github.com/nikic updated https://github.com/llvm/llvm-project/pull/222266
>From 1610533b068b0cadb816bbcfb1f524a0f96d1ed8 Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Wed, 9 Sep 2026 10:23:47 +0200
Subject: [PATCH 1/3] [ValueTracking] Clarify KnownBits recurrence code
While reviewing a related PR, I found the R/L variable naming here
very confusing. Use Start and Step instead, matching the parameter
names of matchSimpleRecurrence().
While doing that, I also found some context instruction adjustment
that does not make sense to me. We need to adjust the context for
the start value to match the start terminator, as it comes from
the phi node. But the step operand has no relation to the phi, so
I don't think there is any reason why this needs to adjust context
to the loop latch terminator.
---
llvm/lib/Analysis/ValueTracking.cpp | 34 ++++++++++++++---------------
1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 950b125028b99..422a7d60b615b 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -1819,8 +1819,8 @@ static void computeKnownBitsFromOperator(const Operator *I,
case Instruction::PHI: {
const PHINode *P = cast<PHINode>(I);
BinaryOperator *BO = nullptr;
- Value *R = nullptr, *L = nullptr;
- if (matchSimpleRecurrence(P, BO, R, L)) {
+ Value *Start = nullptr, *Step = nullptr;
+ if (matchSimpleRecurrence(P, BO, Start, Step)) {
// Handle the case of a simple two-predecessor recurrence PHI.
// There's a lot more that could theoretically be done here, but
// this is sufficient to catch some interesting cases.
@@ -1853,7 +1853,7 @@ static void computeKnownBitsFromOperator(const Operator *I,
// add sufficient tests to cover.
SimplifyQuery RecQ = Q.getWithoutCondContext();
RecQ.CxtI = P;
- computeKnownBits(R, DemandedElts, Known2, RecQ, Depth + 1);
+ computeKnownBits(Start, DemandedElts, Known2, RecQ, Depth + 1);
switch (Opcode) {
case Instruction::Shl:
// A shl recurrence will only increase the tailing zeros
@@ -1889,22 +1889,20 @@ static void computeKnownBitsFromOperator(const Operator *I,
// D69571).
SimplifyQuery RecQ = Q.getWithoutCondContext();
- unsigned OpNum = P->getOperand(0) == R ? 0 : 1;
- Instruction *RInst = P->getIncomingBlock(OpNum)->getTerminator();
- Instruction *LInst = P->getIncomingBlock(1 - OpNum)->getTerminator();
+ unsigned OpNum = P->getOperand(0) == Start ? 0 : 1;
+ Instruction *StartTerm = P->getIncomingBlock(OpNum)->getTerminator();
- // Ok, we have a PHI of the form L op= R. Check for low
+ // Ok, we have a recurrence of the form {Start,op,Step}. Check for low
// zero bits.
- RecQ.CxtI = RInst;
- computeKnownBits(R, DemandedElts, Known2, RecQ, Depth + 1);
+ RecQ.CxtI = StartTerm;
+ computeKnownBits(Start, DemandedElts, Known2, RecQ, Depth + 1);
// We need to take the minimum number of known bits
- KnownBits Known3(BitWidth);
- RecQ.CxtI = LInst;
- computeKnownBits(L, DemandedElts, Known3, RecQ, Depth + 1);
+ KnownBits KnownStep(BitWidth);
+ computeKnownBits(Step, DemandedElts, KnownStep, Q, Depth + 1);
Known.Zero.setLowBits(std::min(Known2.countMinTrailingZeros(),
- Known3.countMinTrailingZeros()));
+ KnownStep.countMinTrailingZeros()));
auto *OverflowOp = dyn_cast<OverflowingBinaryOperator>(BO);
if (!OverflowOp || !Q.IIQ.hasNoSignedWrap(OverflowOp))
@@ -1921,9 +1919,9 @@ static void computeKnownBitsFromOperator(const Operator *I,
// (add non-negative, non-negative) --> non-negative
// (add negative, negative) --> negative
case Instruction::Add: {
- if (Known2.isNonNegative() && Known3.isNonNegative())
+ if (Known2.isNonNegative() && KnownStep.isNonNegative())
Known.makeNonNegative();
- else if (Known2.isNegative() && Known3.isNegative())
+ else if (Known2.isNegative() && KnownStep.isNegative())
Known.makeNegative();
break;
}
@@ -1933,16 +1931,16 @@ static void computeKnownBitsFromOperator(const Operator *I,
case Instruction::Sub: {
if (BO->getOperand(0) != I)
break;
- if (Known2.isNonNegative() && Known3.isNegative())
+ if (Known2.isNonNegative() && KnownStep.isNegative())
Known.makeNonNegative();
- else if (Known2.isNegative() && Known3.isNonNegative())
+ else if (Known2.isNegative() && KnownStep.isNonNegative())
Known.makeNegative();
break;
}
// (mul nsw non-negative, non-negative) --> non-negative
case Instruction::Mul:
- if (Known2.isNonNegative() && Known3.isNonNegative())
+ if (Known2.isNonNegative() && KnownStep.isNonNegative())
Known.makeNonNegative();
break;
>From 1712c63612ead891e850a4011c6f9747b79e74bf Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Wed, 9 Sep 2026 12:15:47 +0200
Subject: [PATCH 2/3] Add test for miscompile
---
.../test/Transforms/InstCombine/recurrence.ll | 33 +++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/llvm/test/Transforms/InstCombine/recurrence.ll b/llvm/test/Transforms/InstCombine/recurrence.ll
index 6207009b531d5..bbaf4af5bb760 100644
--- a/llvm/test/Transforms/InstCombine/recurrence.ll
+++ b/llvm/test/Transforms/InstCombine/recurrence.ll
@@ -162,4 +162,37 @@ loop: ; preds = %loop, %entry
br label %loop
}
+declare i32 @get_step()
+
+define i1 @test_loop_variant_step_with_condition() {
+; CHECK-LABEL: @test_loop_variant_step_with_condition(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: br label [[LOOP:%.*]]
+; CHECK: loop:
+; CHECK-NEXT: [[STEP:%.*]] = call i32 @get_step()
+; CHECK-NEXT: [[C:%.*]] = icmp sgt i32 [[STEP]], -1
+; CHECK-NEXT: br i1 [[C]], label [[EXIT:%.*]], label [[LATCH:%.*]]
+; CHECK: latch:
+; CHECK-NEXT: br label [[LOOP]]
+; CHECK: exit:
+; CHECK-NEXT: ret i1 true
+;
+entry:
+ br label %loop
+
+loop:
+ %iv = phi i32 [ 0, %entry ], [ %iv.next, %latch ]
+ %step = call i32 @get_step()
+ %iv.next = add nsw i32 %iv, %step
+ %c = icmp sge i32 %step, 0
+ br i1 %c, label %exit, label %latch
+
+latch:
+ br label %loop
+
+exit:
+ %result = icmp sge i32 %iv, 0
+ ret i1 %result
+}
+
declare void @use(i64)
>From ffb5738445dd528c49d4e9c5eee45f34a2a2d989 Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Wed, 9 Sep 2026 12:17:57 +0200
Subject: [PATCH 3/3] Restore previous context
---
llvm/lib/Analysis/ValueTracking.cpp | 9 +++++++--
llvm/test/Transforms/InstCombine/recurrence.ll | 7 +++++--
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 422a7d60b615b..dc9bc3a88806f 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -1891,15 +1891,20 @@ static void computeKnownBitsFromOperator(const Operator *I,
unsigned OpNum = P->getOperand(0) == Start ? 0 : 1;
Instruction *StartTerm = P->getIncomingBlock(OpNum)->getTerminator();
+ Instruction *LatchTerm =
+ P->getIncomingBlock(1 - OpNum)->getTerminator();
// Ok, we have a recurrence of the form {Start,op,Step}. Check for low
// zero bits.
RecQ.CxtI = StartTerm;
computeKnownBits(Start, DemandedElts, Known2, RecQ, Depth + 1);
- // We need to take the minimum number of known bits
+ // We need to take the minimum number of known bits.
+ // The step may be loop-variant, so make sure we don't make use of
+ // any conditions that only hold on the last iteration.
KnownBits KnownStep(BitWidth);
- computeKnownBits(Step, DemandedElts, KnownStep, Q, Depth + 1);
+ RecQ.CxtI = LatchTerm;
+ computeKnownBits(Step, DemandedElts, KnownStep, RecQ, Depth + 1);
Known.Zero.setLowBits(std::min(Known2.countMinTrailingZeros(),
KnownStep.countMinTrailingZeros()));
diff --git a/llvm/test/Transforms/InstCombine/recurrence.ll b/llvm/test/Transforms/InstCombine/recurrence.ll
index bbaf4af5bb760..456b20a187519 100644
--- a/llvm/test/Transforms/InstCombine/recurrence.ll
+++ b/llvm/test/Transforms/InstCombine/recurrence.ll
@@ -169,13 +169,16 @@ define i1 @test_loop_variant_step_with_condition() {
; CHECK-NEXT: entry:
; CHECK-NEXT: br label [[LOOP:%.*]]
; CHECK: loop:
+; CHECK-NEXT: [[IV:%.*]] = phi i32 [ 0, [[ENTRY:%.*]] ], [ [[IV_NEXT:%.*]], [[LATCH:%.*]] ]
; CHECK-NEXT: [[STEP:%.*]] = call i32 @get_step()
; CHECK-NEXT: [[C:%.*]] = icmp sgt i32 [[STEP]], -1
-; CHECK-NEXT: br i1 [[C]], label [[EXIT:%.*]], label [[LATCH:%.*]]
+; CHECK-NEXT: br i1 [[C]], label [[EXIT:%.*]], label [[LATCH]]
; CHECK: latch:
+; CHECK-NEXT: [[IV_NEXT]] = add nsw i32 [[IV]], [[STEP]]
; CHECK-NEXT: br label [[LOOP]]
; CHECK: exit:
-; CHECK-NEXT: ret i1 true
+; CHECK-NEXT: [[RESULT:%.*]] = icmp sgt i32 [[IV]], -1
+; CHECK-NEXT: ret i1 [[RESULT]]
;
entry:
br label %loop
More information about the llvm-commits
mailing list