Skip to content

Conversation

RKSimon
Copy link
Collaborator

@RKSimon RKSimon commented Sep 3, 2024

If we have the lower/upper bounds for a value, determine the known bits shared by all values within those bounds.

Unlike llvm::ConstantRange, these bounds values are INCLUSIVE (to stop overflow issues in some future use cases).

@llvmbot
Copy link
Member

llvmbot commented Sep 3, 2024

@llvm/pr-subscribers-llvm-support

Author: Simon Pilgrim (RKSimon)

Changes

If we have the lower/upper bounds for a value, determine the known bits shared by all values within those bounds.

Unlike llvm::ConstantRange, these bounds values are INCLUSIVE (to stop overflow issues in some future use cases).


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

3 Files Affected:

  • (modified) llvm/include/llvm/Support/KnownBits.h (+3)
  • (modified) llvm/lib/Support/KnownBits.cpp (+14)
  • (modified) llvm/unittests/Support/KnownBitsTest.cpp (+17)
diff --git a/llvm/include/llvm/Support/KnownBits.h b/llvm/include/llvm/Support/KnownBits.h
index e4ec202f36aae0..b169a0aba3edac 100644
--- a/llvm/include/llvm/Support/KnownBits.h
+++ b/llvm/include/llvm/Support/KnownBits.h
@@ -291,6 +291,9 @@ struct KnownBits {
     return KnownBits(~C, C);
   }
 
+  /// Create known bits from a known INCLUSIVE pair of unsigned constants.
+  static KnownBits makeConstantRange(const APInt &Lower, const APInt &Upper);
+
   /// Returns KnownBits information that is known to be true for both this and
   /// RHS.
   ///
diff --git a/llvm/lib/Support/KnownBits.cpp b/llvm/lib/Support/KnownBits.cpp
index 8e31e0ced2d731..5c37575d157358 100644
--- a/llvm/lib/Support/KnownBits.cpp
+++ b/llvm/lib/Support/KnownBits.cpp
@@ -750,6 +750,20 @@ static KnownBits computeForSatAddSub(bool Add, bool Signed,
   return Res;
 }
 
+KnownBits KnownBits::makeConstantRange(const APInt &Lower, const APInt &Upper) {
+  assert(Lower.getBitWidth() == Upper.getBitWidth() && "Bitwidth mismatch");
+  assert(Lower.ule(Upper) && "Constant range mismatch");
+
+  KnownBits Known = KnownBits::makeConstant(Lower);
+  if (std::optional<unsigned> DifferentBit =
+          APIntOps::GetMostSignificantDifferentBit(Lower, Upper)) {
+    Known.Zero.clearLowBits(*DifferentBit + 1);
+    Known.One.clearLowBits(*DifferentBit + 1);
+  }
+
+  return Known;
+}
+
 KnownBits KnownBits::sadd_sat(const KnownBits &LHS, const KnownBits &RHS) {
   return computeForSatAddSub(/*Add*/ true, /*Signed*/ true, LHS, RHS);
 }
diff --git a/llvm/unittests/Support/KnownBitsTest.cpp b/llvm/unittests/Support/KnownBitsTest.cpp
index b6e16f809ea779..f10a153225ab2c 100644
--- a/llvm/unittests/Support/KnownBitsTest.cpp
+++ b/llvm/unittests/Support/KnownBitsTest.cpp
@@ -668,6 +668,23 @@ TEST(KnownBitsTest, ICmpExhaustive) {
   });
 }
 
+TEST(KnownBitsTest, ConstantRange) {
+  unsigned Bits = 4;
+  unsigned MaxValue = (1U << Bits) - 1;
+  for (unsigned Lo = 0; Lo <= MaxValue; ++Lo) {
+    const APInt LoInt(Bits, Lo);
+    for (unsigned Hi = Lo; Hi <= MaxValue; ++Hi) {
+      KnownBits Ref = KnownBits::makeConstant(LoInt);
+      for (unsigned I = Lo + 1; I <= Hi; ++I)
+        Ref = Ref.intersectWith(KnownBits::makeConstant(APInt(Bits, I)));
+
+      KnownBits Known = KnownBits::makeConstantRange(LoInt, APInt(Bits, Hi));
+      EXPECT_EQ(Ref.Zero, Known.Zero);
+      EXPECT_EQ(Ref.One, Known.One);
+    }
+  }
+}
+
 TEST(KnownBitsTest, GetMinMaxVal) {
   unsigned Bits = 4;
   ForeachKnownBits(Bits, [&](const KnownBits &Known) {

@nikic
Copy link
Contributor

nikic commented Sep 3, 2024

Why do we need this if we have ConstantRange::toKnownBits()?

@RKSimon
Copy link
Collaborator Author

RKSimon commented Sep 3, 2024

Are people happy for me to use KnownBits -> ConstantRange -> KnownBits paths ? My WIP patches already do this but I'd convinced myself that I'd immediately get feedback asking me to remove the ConstantRange dependency :(

@nikic
Copy link
Contributor

nikic commented Sep 3, 2024

Can you share your wip patch for context?

@RKSimon
Copy link
Collaborator Author

RKSimon commented Sep 3, 2024

#107085

@nikic
Copy link
Contributor

nikic commented Sep 3, 2024

#107085

The usage of ConstantRange there looks ok to me. But I'm also not really opposed to adding this API.

…bits from a pair of unsigned lo/hi bound values

If we have the lower/upper bounds for a value, determine the known bits shared by all values within those bounds.

Unlike llvm::ConstantRange, these bounds values are INCLUSIVE (to stop overflow issues in some future use cases).
@RKSimon RKSimon force-pushed the knownbits-constantrange branch from 32c031b to 3712221 Compare September 3, 2024 12:35
APIntOps::GetMostSignificantDifferentBit(Lower, Upper)) {
Known.Zero.clearLowBits(*DifferentBit + 1);
Known.One.clearLowBits(*DifferentBit + 1);
}
Copy link
Contributor

@goldsteinn goldsteinn Sep 3, 2024

Choose a reason for hiding this comment

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

Can this be implemented as just: KnownBits::makeConstant(Lower).intersectWith(KnownBits::makeConstant(Upper))?

No it cannot

@RKSimon RKSimon changed the title [KnownBits] Add KnownBits::makeConstantRange to determine the known bits from a pair of unsigned lo/hi bound values [KnownBits] Add KnownBits::makeInclusiveRange to determine the known bits from a pair of unsigned lo/hi bound values Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants