MathExtras: rewrite some methods to never overflow

[artagnon]

Rewrite divideCeil, divideNearest, divideFloorSigned, and divideCeilSigned to never overflow.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Ramkumar Ramachandra (artagnon)

Changes

Currently, divideCeil may overflow due to its dependency on alignTo, and there is no way to avoid overflowing in alignTo. Remove this dependency, and rewrite divideCeil to never overflow. Also rewrite divideNearest to never overflow. Clarify divideCeilSigned and divideFloorSigned to indicate that they can never overflow.

---

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

2 Files Affected:

• (modified) llvm/include/llvm/Support/MathExtras.h (+18-10)
• (modified) llvm/unittests/Support/MathExtrasTest.cpp (+25-2)

diff --git a/llvm/include/llvm/Support/MathExtras.h b/llvm/include/llvm/Support/MathExtras.h
index 05d87e176dec1..74815b7377ca5 100644
--- a/llvm/include/llvm/Support/MathExtras.h
+++ b/llvm/include/llvm/Support/MathExtras.h
@@ -385,6 +385,8 @@ inline uint64_t PowerOf2Ceil(uint64_t A) {
 ///   alignTo(~0LL, 8) = 0
 ///   alignTo(321, 255) = 510
 /// \endcode
+///
+/// May overflow.
 inline uint64_t alignTo(uint64_t Value, uint64_t Align) {
   assert(Align != 0u && "Align can't be 0.");
   return (Value + Align - 1) / Align * Align;
@@ -424,33 +426,37 @@ template <uint64_t Align> constexpr inline uint64_t alignTo(uint64_t Value) {
   return (Value + Align - 1) / Align * Align;
 }
 
-/// Returns the integer ceil(Numerator / Denominator). Unsigned integer version.
+/// Returns the integer ceil(Numerator / Denominator). Unsigned version.
+/// Guaranteed to never overflow.
 inline uint64_t divideCeil(uint64_t Numerator, uint64_t Denominator) {
-  return alignTo(Numerator, Denominator) / Denominator;
+  uint64_t Bias = (Numerator != 0);
+  return (Numerator - Bias) / Denominator + Bias;
 }
 
-/// Returns the integer ceil(Numerator / Denominator). Signed integer version.
+/// Returns the integer ceil(Numerator / Denominator). Signed version.
+/// Guaranteed to never overflow.
 inline int64_t divideCeilSigned(int64_t Numerator, int64_t Denominator) {
   assert(Denominator && "Division by zero");
   if (!Numerator)
     return 0;
   // C's integer division rounds towards 0.
-  int64_t X = (Denominator > 0) ? -1 : 1;
+  int64_t Bias = (Denominator > 0 ? 1 : -1);
   bool SameSign = (Numerator > 0) == (Denominator > 0);
-  return SameSign ? ((Numerator + X) / Denominator) + 1
+  return SameSign ? (Numerator - Bias) / Denominator + 1
                   : Numerator / Denominator;
 }
 
-/// Returns the integer floor(Numerator / Denominator). Signed integer version.
+/// Returns the integer floor(Numerator / Denominator). Signed version.
+/// Guaranteed to never overflow.
 inline int64_t divideFloorSigned(int64_t Numerator, int64_t Denominator) {
   assert(Denominator && "Division by zero");
   if (!Numerator)
     return 0;
   // C's integer division rounds towards 0.
-  int64_t X = (Denominator > 0) ? -1 : 1;
+  int64_t Bias = Denominator > 0 ? 1 : -1;
   bool SameSign = (Numerator > 0) == (Denominator > 0);
   return SameSign ? Numerator / Denominator
-                  : -((-Numerator + X) / Denominator) - 1;
+                  : -((-Numerator - Bias) / Denominator) - 1;
 }
 
 /// Returns the remainder of the Euclidean division of LHS by RHS. Result is
@@ -461,9 +467,11 @@ inline int64_t mod(int64_t Numerator, int64_t Denominator) {
   return Mod < 0 ? Mod + Denominator : Mod;
 }
 
-/// Returns the integer nearest(Numerator / Denominator).
+/// Returns (Numerator / Denominator) rounded by round-half-up. Guranteed to
+/// never overflow.
 inline uint64_t divideNearest(uint64_t Numerator, uint64_t Denominator) {
-  return (Numerator + (Denominator / 2)) / Denominator;
+  return (Numerator / Denominator) +
+         (Denominator == 1 ? 0 : Numerator % Denominator >= Denominator / 2);
 }
 
 /// Returns the largest uint64_t less than or equal to \p Value and is
diff --git a/llvm/unittests/Support/MathExtrasTest.cpp b/llvm/unittests/Support/MathExtrasTest.cpp
index bcccb963c96ae..ce620a443756d 100644
--- a/llvm/unittests/Support/MathExtrasTest.cpp
+++ b/llvm/unittests/Support/MathExtrasTest.cpp
@@ -459,15 +459,30 @@ TEST(MathExtras, DivideNearest) {
   EXPECT_EQ(divideNearest(14, 3), 5u);
   EXPECT_EQ(divideNearest(15, 3), 5u);
   EXPECT_EQ(divideNearest(0, 3), 0u);
+  EXPECT_EQ(divideNearest(5, 4), 1u);
+  EXPECT_EQ(divideNearest(6, 4), 2u);
+  EXPECT_EQ(divideNearest(3, 1), 3u);
   EXPECT_EQ(divideNearest(std::numeric_limits<uint32_t>::max(), 2),
-            2147483648u);
+            std::numeric_limits<uint32_t>::max() / 2 + 1);
+  EXPECT_EQ(divideNearest(std::numeric_limits<uint64_t>::max(), 2),
+            std::numeric_limits<uint64_t>::max() / 2 + 1);
+  EXPECT_EQ(divideNearest(std::numeric_limits<uint64_t>::max(), 1),
+            std::numeric_limits<uint64_t>::max());
 }
 
 TEST(MathExtras, DivideCeil) {
   EXPECT_EQ(divideCeil(14, 3), 5u);
   EXPECT_EQ(divideCeil(15, 3), 5u);
   EXPECT_EQ(divideCeil(0, 3), 0u);
-  EXPECT_EQ(divideCeil(std::numeric_limits<uint32_t>::max(), 2), 2147483648u);
+  EXPECT_EQ(divideCeil(5, 4), 2u);
+  EXPECT_EQ(divideCeil(6, 4), 2u);
+  EXPECT_EQ(divideCeil(3, 1), 3u);
+  EXPECT_EQ(divideCeil(std::numeric_limits<uint32_t>::max(), 2),
+            std::numeric_limits<uint32_t>::max() / 2 + 1);
+  EXPECT_EQ(divideCeil(std::numeric_limits<uint64_t>::max(), 2),
+            std::numeric_limits<uint64_t>::max() / 2 + 1);
+  EXPECT_EQ(divideCeil(std::numeric_limits<uint64_t>::max(), 1),
+            std::numeric_limits<uint64_t>::max());
 
   EXPECT_EQ(divideCeilSigned(14, 3), 5);
   EXPECT_EQ(divideCeilSigned(15, 3), 5);
@@ -479,8 +494,12 @@ TEST(MathExtras, DivideCeil) {
   EXPECT_EQ(divideCeilSigned(0, -3), 0);
   EXPECT_EQ(divideCeilSigned(std::numeric_limits<int32_t>::max(), 2),
             std::numeric_limits<int32_t>::max() / 2 + 1);
+  EXPECT_EQ(divideCeilSigned(std::numeric_limits<int64_t>::max(), 2),
+            std::numeric_limits<int64_t>::max() / 2 + 1);
   EXPECT_EQ(divideCeilSigned(std::numeric_limits<int32_t>::max(), -2),
             std::numeric_limits<int32_t>::min() / 2 + 1);
+  EXPECT_EQ(divideCeilSigned(std::numeric_limits<int64_t>::max(), -2),
+            std::numeric_limits<int64_t>::min() / 2 + 1);
 }
 
 TEST(MathExtras, DivideFloorSigned) {
@@ -494,8 +513,12 @@ TEST(MathExtras, DivideFloorSigned) {
   EXPECT_EQ(divideFloorSigned(0, -3), 0);
   EXPECT_EQ(divideFloorSigned(std::numeric_limits<int32_t>::max(), 2),
             std::numeric_limits<int32_t>::max() / 2);
+  EXPECT_EQ(divideFloorSigned(std::numeric_limits<int64_t>::max(), 2),
+            std::numeric_limits<int64_t>::max() / 2);
   EXPECT_EQ(divideFloorSigned(std::numeric_limits<int32_t>::max(), -2),
             std::numeric_limits<int32_t>::min() / 2);
+  EXPECT_EQ(divideFloorSigned(std::numeric_limits<int64_t>::max(), -2),
+            std::numeric_limits<int64_t>::min() / 2);
 }
 
 TEST(MathExtras, Mod) {

[jayfoad]

Rewrite divideCeil, divideNearest, and divideFloorSigned to never overflow.

Also mention divideCeilSigned.

[jayfoad]

Looks good as far as I can see.

[nikic]

It looks like this does have some overhead in practice: https://llvm-compile-time-tracker.com/compare.php?from=23c1b488fee99ac598203b6c3972be3b404dfbbe&to=bfd95a003139a8f930874b2234c3cab545d504a1&stat=instructions:u

It looks like LLVM currently can't optimize this even if overflow isn't possible: #95652

[artagnon]

Interesting. I suppose this needs to be fixed in InstCombine?

[jayfoad]

It looks like this does have some overhead in practice

It would be good to know which functions are hot, and with what kind of arguments.

Do we have a good way to specialize these functions when Y is known to be a power of 2, e.g. with something like if (__builtin_constant_p(Y) && isPowerOf2(Y))?