Skip to content

Conversation

@AZero13
Copy link
Contributor

@AZero13 AZero13 commented Jun 15, 2025

We have to choose between unsigned and signed if there are two overlaps, aka the range wraps around, meaning [254,255] vs. signed [-128, 125], but [254,255] correspond to [-2 -1] which is in the range [-128, 125].

However, a range that would not work would be one where one has to pick between [0, 129] vs [-127, 127] because 129 is -2 signed.

@AZero13 AZero13 requested a review from nikic as a code owner June 15, 2025 15:24
@llvmbot llvmbot added the llvm:analysis Includes value tracking, cost tables and constant folding label Jun 15, 2025
@llvmbot
Copy link
Member

llvmbot commented Jun 15, 2025

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: AZero13 (AZero13)

Changes

We have to choose between unsigned and signed if there are two overlaps, aka the range wraps around, meaning [254,255] vs. signed [-128, 125], but [254,255] correspond to [-2 -1] which is in the range [-128, 125].

However, a range that would not work would be one where one has to pick between [0, 129] vs [-127, 127] because 129 is -2 signed.


Full diff: https://github.com/llvm/llvm-project/pull/144267.diff

1 Files Affected:

  • (modified) llvm/lib/Analysis/ValueTracking.cpp (+49)
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index e7a1f07c0270d..11f20ce9671ba 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -9585,6 +9585,30 @@ static void setLimitsForBinOp(const BinaryOperator &BO, APInt &Lower,
       bool HasNSW = IIQ.hasNoSignedWrap(&BO);
       bool HasNUW = IIQ.hasNoUnsignedWrap(&BO);
 
+      if (HasNUW && HasNSW) {
+        APInt uLo = APInt::getZero(Width);
+        APInt uHi = *C + 1;
+
+        APInt sLo, sHi;
+        if (C->isNegative()) {
+          sLo = APInt::getSignedMinValue(Width);
+          sHi = APInt::getSignedMaxValue(Width) + *C + 2;
+        } else {
+          sLo = APInt::getSignedMinValue(Width) + *C + 1;
+          sHi = APInt::getSignedMaxValue(Width) + 1;
+        }
+
+        APInt iLo = APIntOps::smax(uLo, sLo);
+        APInt iHi = APIntOps::smin(uHi, sHi);
+
+        if (iLo.ult(iHi) && iLo.sge(APInt::getSignedMinValue(Width)) &&
+            (iHi - 1).sle(APInt::getSignedMaxValue(Width))) {
+          Lower = iLo;
+          Upper = iHi;
+          break;
+        }
+      }
+
       // If the caller expects a signed compare, then try to use a signed range.
       // Otherwise if both no-wraps are set, use the unsigned range because it
       // is never larger than the signed range. Example:
@@ -9615,6 +9639,31 @@ static void setLimitsForBinOp(const BinaryOperator &BO, APInt &Lower,
       bool HasNSW = IIQ.hasNoSignedWrap(&BO);
       bool HasNUW = IIQ.hasNoUnsignedWrap(&BO);
 
+      if (HasNUW && HasNSW) {
+        APInt uLo = *C;
+        APInt uHi = APInt::getZero(Width);
+
+        APInt sLo, sHi;
+        if (C->isNegative()) {
+          sLo = APInt::getSignedMinValue(Width);
+          sHi = APInt::getSignedMaxValue(Width) + *C + 1;
+        } else {
+          sLo = APInt::getSignedMinValue(Width) + *C;
+          sHi = APInt::getSignedMaxValue(Width) + 1;
+        }
+
+        APInt iLo = APIntOps::smax(uLo, sLo);
+        APInt iHi = APIntOps::smin(uHi, sHi);
+
+        if (iLo.ult(iHi) &&
+            iLo.sge(APInt::getSignedMinValue(Width)) &&
+            (iHi - 1).sle(APInt::getSignedMaxValue(Width))) {
+          Lower = iLo;
+          Upper = iHi;
+          break;
+        }
+      }
+
       // If the caller expects a signed compare, then try to use a signed
       // range. Otherwise if both no-wraps are set, use the unsigned range
       // because it is never larger than the signed range. Example: "add nuw

@AZero13 AZero13 marked this pull request as draft June 15, 2025 16:38
@github-actions
Copy link

github-actions bot commented Jun 15, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@AZero13 AZero13 marked this pull request as ready for review June 15, 2025 22:37
@AZero13 AZero13 force-pushed the unsigned branch 2 times, most recently from 813a4e9 to 4d57702 Compare June 15, 2025 22:37
…, return it

We have to choose between unsigned and signed if there are two overlaps, aka the range wraps around, meaning [254,255] vs. signed [-128, 125], but [254,255] correspond to [-2 -1] which is in the range [-128, 125].

However, a range that would not work would be one where one has to pick between [0, 129] vs [-127, 127] because 129 is -2 signed.

Update ValueTracking.cpp

Revert "[ValueTracking] If overlap in unsigned and signed range is contiguous, return it"

This reverts commit 22e997c489aad3173db78f6fee17212bd16be96d.

ok
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

computeConstantRange() produces a crude and cheap approximation. We are not interested in making it more accurate at the cost of significant complexity.

; CHECK: middle.block:
; CHECK-NEXT: [[CMP_N:%.*]] = icmp eq i64 [[N_MOD_VF]], 0
; CHECK-NEXT: br i1 [[CMP_N]], label [[FOR_END:%.*]], label [[SCALAR_PH]]
; CHECK-NEXT: br i1 false, label [[FOR_END:%.*]], label [[SCALAR_PH]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these changes look like miscompiles.

@AZero13 AZero13 closed this Jun 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

llvm:analysis Includes value tracking, cost tables and constant folding llvm:transforms

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants