[ADT] Add implementations for avgFloor and avgCeil to APInt #84431

Atousa · 2024-03-08T05:31:52Z

Supports both signed and unsigned expansions.
SelectionDAG now calls the APInt implementation of these functions.

Fixes #84211.

llvmbot · 2024-03-08T05:33:37Z

@llvm/pr-subscribers-llvm-selectiondag
@llvm/pr-subscribers-llvm-adt

@llvm/pr-subscribers-llvm-support

Author: Atousa Duprat (Atousa)

Changes

Supports both signed and unsigned expansions.
SelectionDAG now calls the APInt implementation of these functions.

Issue #84211.

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

4 Files Affected:

(modified) llvm/include/llvm/ADT/APInt.h (+12)
(modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (+8-24)
(modified) llvm/lib/Support/APInt.cpp (+36)
(modified) llvm/unittests/ADT/APIntTest.cpp (+50)

diff --git a/llvm/include/llvm/ADT/APInt.h b/llvm/include/llvm/ADT/APInt.h
index 1fc3c7b2236a17..72f88fd956f2f3 100644
--- a/llvm/include/llvm/ADT/APInt.h
+++ b/llvm/include/llvm/ADT/APInt.h
@@ -2193,6 +2193,18 @@ inline const APInt absdiff(const APInt &A, const APInt &B) {
   return A.uge(B) ? (A - B) : (B - A);
 }
 
+///.Compute the signed floor of the average of C1 and C2
+APInt avgFloorS(const APInt &C1, const APInt &C2);
+
+///.Compute the floor of the average of C1 and C2
+APInt avgFloorU(const APInt &C1, const APInt &C2);
+
+///.Compute the ceil of the average of C1 and C2
+APInt avgCeilS(const APInt &C1, const APInt &C2);
+
+///.Compute the ceil of the average of C1 and C2
+APInt avgCeilU(const APInt &C1, const APInt &C2);
+
 /// Compute GCD of two unsigned APInt values.
 ///
 /// This function returns the greatest common divisor of the two APInt values
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index f7ace79e8c51d4..a844a00be16291 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -6021,30 +6021,14 @@ static std::optional<APInt> FoldValue(unsigned Opcode, const APInt &C1,
     APInt C2Ext = C2.zext(FullWidth);
     return (C1Ext * C2Ext).extractBits(C1.getBitWidth(), C1.getBitWidth());
   }
-  case ISD::AVGFLOORS: {
-    unsigned FullWidth = C1.getBitWidth() + 1;
-    APInt C1Ext = C1.sext(FullWidth);
-    APInt C2Ext = C2.sext(FullWidth);
-    return (C1Ext + C2Ext).extractBits(C1.getBitWidth(), 1);
-  }
-  case ISD::AVGFLOORU: {
-    unsigned FullWidth = C1.getBitWidth() + 1;
-    APInt C1Ext = C1.zext(FullWidth);
-    APInt C2Ext = C2.zext(FullWidth);
-    return (C1Ext + C2Ext).extractBits(C1.getBitWidth(), 1);
-  }
-  case ISD::AVGCEILS: {
-    unsigned FullWidth = C1.getBitWidth() + 1;
-    APInt C1Ext = C1.sext(FullWidth);
-    APInt C2Ext = C2.sext(FullWidth);
-    return (C1Ext + C2Ext + 1).extractBits(C1.getBitWidth(), 1);
-  }
-  case ISD::AVGCEILU: {
-    unsigned FullWidth = C1.getBitWidth() + 1;
-    APInt C1Ext = C1.zext(FullWidth);
-    APInt C2Ext = C2.zext(FullWidth);
-    return (C1Ext + C2Ext + 1).extractBits(C1.getBitWidth(), 1);
-  }
+  case ISD::AVGFLOORS:
+    return APIntOps::avgFloorS(C1, C2);
+  case ISD::AVGFLOORU:
+    return APIntOps::avgFloorU(C1, C2);
+  case ISD::AVGCEILS:
+    return APIntOps::avgCeilS(C1, C2);
+  case ISD::AVGCEILU:
+    return APIntOps::avgCeilU(C1, C2);
   case ISD::ABDS:
     return APIntOps::smax(C1, C2) - APIntOps::smin(C1, C2);
   case ISD::ABDU:
diff --git a/llvm/lib/Support/APInt.cpp b/llvm/lib/Support/APInt.cpp
index e686b976523302..4c2188dbbbfb7f 100644
--- a/llvm/lib/Support/APInt.cpp
+++ b/llvm/lib/Support/APInt.cpp
@@ -3094,3 +3094,39 @@ void llvm::LoadIntFromMemory(APInt &IntVal, const uint8_t *Src,
     memcpy(Dst + sizeof(uint64_t) - LoadBytes, Src, LoadBytes);
   }
 }
+
+APInt APIntOps::avgFloorS(const APInt &C1, const APInt &C2) {
+
+  // Return floor((C1 + C2) /2))
+  unsigned FullWidth = C1.getBitWidth() + 1;
+  APInt C1Ext = C1.sext(FullWidth);
+  APInt C2Ext = C2.sext(FullWidth);
+  return (C1Ext + C2Ext).extractBits(C1.getBitWidth(), 1);
+}
+
+APInt APIntOps::avgFloorU(const APInt &C1, const APInt &C2) {
+
+  // Return floor((C1 + C2) /2))
+  unsigned FullWidth = C1.getBitWidth() + 1;
+  APInt C1Ext = C1.zext(FullWidth);
+  APInt C2Ext = C2.zext(FullWidth);
+  return (C1Ext + C2Ext).extractBits(C1.getBitWidth(), 1);
+}
+
+APInt APIntOps::avgCeilS(const APInt &C1, const APInt &C2) {
+
+  // Return ceil((C1 + C2) /2))
+  unsigned FullWidth = C1.getBitWidth() + 1;
+  APInt C1Ext = C1.sext(FullWidth);
+  APInt C2Ext = C2.sext(FullWidth);
+  return (C1Ext + C2Ext + 1).extractBits(C1.getBitWidth(), 1);
+}
+
+APInt APIntOps::avgCeilU(const APInt &C1, const APInt &C2) {
+
+  // Return ceil((C1 + C2) /2))
+  unsigned FullWidth = C1.getBitWidth() + 1;
+  APInt C1Ext = C1.zext(FullWidth);
+  APInt C2Ext = C2.zext(FullWidth);
+  return (C1Ext + C2Ext + 1).extractBits(C1.getBitWidth(), 1);
+}
\ No newline at end of file
diff --git a/llvm/unittests/ADT/APIntTest.cpp b/llvm/unittests/ADT/APIntTest.cpp
index 24324822356bf6..48beccb1f5167a 100644
--- a/llvm/unittests/ADT/APIntTest.cpp
+++ b/llvm/unittests/ADT/APIntTest.cpp
@@ -2877,6 +2877,56 @@ TEST(APIntTest, RoundingSDiv) {
   }
 }
 
+TEST(APIntTest, Average) {
+  APInt A2(32, 2);
+  APInt A100(32, 100);
+  APInt A101(32, 101);
+  APInt A200(32, 200, false);
+  APInt avg = APIntOps::avgFloorU(A100, A200);
+
+  EXPECT_EQ(APInt(32, 150), APIntOps::avgFloorU(A100, A200));
+  EXPECT_EQ(APIntOps::RoundingUDiv(A100 + A200, A2, APInt::Rounding::DOWN),
+            APIntOps::avgFloorU(A100, A200));
+  EXPECT_EQ(APIntOps::RoundingUDiv(A100 + A200, A2, APInt::Rounding::UP),
+            APIntOps::avgCeilU(A100, A200));
+  EXPECT_EQ(APIntOps::RoundingUDiv(A100 + A101, A2, APInt::Rounding::DOWN),
+            APIntOps::avgFloorU(A100, A101));
+  EXPECT_EQ(APIntOps::RoundingUDiv(A100 + A101, A2, APInt::Rounding::UP),
+            APIntOps::avgCeilU(A100, A101));
+
+  APInt Ap100(32, +100);
+  APInt Ap101(32, +101);
+  APInt Ap200(32, +200);
+  APInt Am100(32, -100);
+  APInt Am101(32, -101);
+  APInt Am200(32, -200);
+  EXPECT_EQ(APInt(32, +150), APIntOps::avgFloorS(Ap100, Ap200));
+  EXPECT_EQ(APIntOps::RoundingSDiv(Ap100 + Ap200, A2, APInt::Rounding::DOWN),
+            APIntOps::avgFloorS(Ap100, Ap200));
+  EXPECT_EQ(APIntOps::RoundingSDiv(Ap100 + Ap200, A2, APInt::Rounding::UP),
+            APIntOps::avgCeilS(Ap100, Ap200));
+
+  EXPECT_EQ(APInt(32, -150), APIntOps::avgFloorS(Am100, Am200));
+  EXPECT_EQ(APIntOps::RoundingSDiv(Am100 + Am200, A2, APInt::Rounding::DOWN),
+            APIntOps::avgFloorS(Am100, Am200));
+  EXPECT_EQ(APIntOps::RoundingSDiv(Am100 + Am200, A2, APInt::Rounding::UP),
+            APIntOps::avgCeilS(Am100, Am200));
+
+  EXPECT_EQ(APInt(32, +100), APIntOps::avgFloorS(Ap100, Ap101));
+  EXPECT_EQ(APIntOps::RoundingSDiv(Ap100 + Ap101, A2, APInt::Rounding::DOWN),
+            APIntOps::avgFloorS(Ap100, Ap101));
+  EXPECT_EQ(APInt(32, +101), APIntOps::avgCeilS(Ap100, Ap101));
+  EXPECT_EQ(APIntOps::RoundingSDiv(Ap100 + Ap101, A2, APInt::Rounding::UP),
+            APIntOps::avgCeilS(Ap100, Ap101));
+
+  EXPECT_EQ(APInt(32, -101), APIntOps::avgFloorS(Am100, Am101));
+  EXPECT_EQ(APIntOps::RoundingSDiv(Am100 + Am101, A2, APInt::Rounding::DOWN),
+            APIntOps::avgFloorS(Am100, Am101));
+  EXPECT_EQ(APInt(32, -100), APIntOps::avgCeilS(Am100, Am101));
+  EXPECT_EQ(APIntOps::RoundingSDiv(Am100 + Am101, A2, APInt::Rounding::UP),
+            APIntOps::avgCeilS(Am100, Am101));
+}
+
 TEST(APIntTest, umul_ov) {
   const std::pair<uint64_t, uint64_t> Overflows[] = {
       {0x8000000000000000, 2},

RKSimon · 2024-03-08T09:51:52Z

llvm/lib/Support/APInt.cpp

@@ -3094,3 +3094,39 @@ void llvm::LoadIntFromMemory(APInt &IntVal, const uint8_t *Src,
    memcpy(Dst + sizeof(uint64_t) - LoadBytes, Src, LoadBytes);
  }
 }
+
+APInt APIntOps::avgFloorS(const APInt &C1, const APInt &C2) {
+


remove extra newlines

jayfoad · 2024-03-08T10:02:20Z

[ADT] Add implementations for avgFloor and ceilFloor to APInt

Typo "ceilFloor"

jayfoad · 2024-03-08T10:03:03Z

llvm/include/llvm/ADT/APInt.h

@@ -2193,6 +2193,18 @@ inline const APInt absdiff(const APInt &A, const APInt &B) {
  return A.uge(B) ? (A - B) : (B - A);
 }

+///.Compute the signed floor of the average of C1 and C2


Space instead of dot after ///

It is the average that is signed or unsigned, not the floor. So the comment should be "Compute the floor of the signed average of C1 and C2". Similarly for the others.

jayfoad · 2024-03-08T16:25:41Z

llvm/include/llvm/ADT/APInt.h

+/// Compute the floor of the signed average of C1 and C2
+APInt avgFloorS(const APInt &C1, const APInt &C2);
+
+/// Compute the floor of the average of C1 and C2


Should say "unsigned average" here for consistency. Same for avgCeilU below.

jayfoad · 2024-03-08T16:48:18Z

Incidentally Hacker's Delight has a section "Average of Two Integers" which has some neat tricks for implementing these operations without extending to N+1-bit integers.

RKSimon · 2024-03-08T16:58:58Z

Incidentally Hacker's Delight has a section "Average of Two Integers" which has some neat tricks for implementing these operations without extending to N+1-bit integers.

Thanks, we use min/max/sat/cmpsel instructions on TargetLowering::expandABD - using those as fallbacks might be a nice followup good-first-issue for someone :)

Atousa · 2024-03-08T18:23:51Z

@RKSimon @jayfoad Any further comments?

Atousa · 2024-03-08T18:28:23Z

Incidentally Hacker's Delight has a section "Average of Two Integers" which has some neat tricks for implementing these operations without extending to N+1-bit integers.

Thanks, we use min/max/sat/cmpsel instructions on TargetLowering::expandABD - using those as fallbacks might be a nice followup good-first-issue for someone :)

Can I open a new discussion regarding a better implementation of AvgFloor and AvgCeil in TargetLowering::expandABD?

llvm/lib/Support/APInt.cpp

RKSimon · 2024-03-09T16:47:48Z

Incidentally Hacker's Delight has a section "Average of Two Integers" which has some neat tricks for implementing these operations without extending to N+1-bit integers.

Thanks, we use min/max/sat/cmpsel instructions on TargetLowering::expandABD - using those as fallbacks might be a nice followup good-first-issue for someone :)

Can I open a new discussion regarding a better implementation of AvgFloor and AvgCeil in TargetLowering::expandABD?

I've raised a ticket here #84639

RKSimon · 2024-03-09T16:56:11Z

I've also raised #84640 for equivalent KnownBits handling

Atousa · 2024-03-11T04:00:37Z

humble ping @nikic @davemgreen @goldsteinn . Thanks!

llvm/unittests/ADT/APIntTest.cpp

topperc · 2024-03-11T05:57:29Z

llvm/lib/Support/APInt.cpp

@@ -3094,3 +3094,35 @@ void llvm::LoadIntFromMemory(APInt &IntVal, const uint8_t *Src,
    memcpy(Dst + sizeof(uint64_t) - LoadBytes, Src, LoadBytes);
  }
 }
+
+APInt APIntOps::avgFloorS(const APInt &C1, const APInt &C2) {
+  // Return floor((C1 + C2) /2))


/2 -> / 2

llvm/unittests/ADT/APIntTest.cpp

jayfoad · 2024-03-12T08:21:36Z

llvm/lib/Support/APInt.cpp

@@ -3094,3 +3094,35 @@ void llvm::LoadIntFromMemory(APInt &IntVal, const uint8_t *Src,
    memcpy(Dst + sizeof(uint64_t) - LoadBytes, Src, LoadBytes);
  }
 }
+
+APInt APIntOps::avgFloorS(const APInt &C1, const APInt &C2) {
+  // Return floor((C1 + C2)/2))


Suggested change

// Return floor((C1 + C2)/2))

// Return floor((C1 + C2)/2)

Similarly for the other comments below

RKSimon · 2024-03-12T09:10:33Z

llvm/lib/Support/APInt.cpp

+}
+
+APInt APIntOps::avgCeilU(const APInt &C1, const APInt &C2) {
+  // Return ceil((C1 + C2)/2))


Add assertions to all 4 methods that C1.getBitWidth() == C2.getBitWidth()

RKSimon · 2024-03-12T14:04:23Z

llvm/unittests/ADT/APIntTest.cpp

@@ -14,6 +14,7 @@
 #include "llvm/Support/Alignment.h"
 #include "gtest/gtest.h"
 #include <array>
+#include <limits.h>


#include <climits>

RKSimon

A couple of minors and a naming question - but other than that this almost looks ready

RKSimon · 2024-03-13T10:06:49Z

llvm/include/llvm/ADT/APInt.h

+APInt avgCeilS(const APInt &C1, const APInt &C2);
+
+/// Compute the ceil of the unsigned average of C1 and C2
+APInt avgCeilU(const APInt &C1, const APInt &C2);


Does anyone have any preference for camelcase vs lowercase?

RKSimon · 2024-03-13T10:07:41Z

llvm/lib/Support/APInt.cpp

+
+APInt APIntOps::avgFloorS(const APInt &C1, const APInt &C2) {
+  // Return floor((C1 + C2)/2)
+  assert(C1.getBitWidth() == C2.getBitWidth());


(style) Assertions must have messages (same below):

assert(C1.getBitWidth() == C2.getBitWidth() && "Unequal bitwidths");

I know that's what the coding standards say, but it seems a bit silly when it encourages people to write a message that just says exactly the same as the condition, but in English.

Supports both signed and unsigned expansions. SelectionDAG now calls the APInt implementation of these functions.

Atousa · 2024-03-14T04:56:27Z

@jayfoad @RKSimon @kuhar @topperc @arsenm Thanks all for reviewing the PR!

RKSimon

LGTM - cheers

antmox · 2024-03-15T10:46:12Z

Hi, looks like this commit broke clang-arm64-windows-msvc-2stage bot:
https://lab.llvm.org/buildbot/#/builders/120/builds/6362

C:\Users\Tcwg\llvm-worker\clang-arm64-windows-msvc-2stage\llvm\llvm\unittests\ADT\APIntTest.cpp(2921,9): error: call to constructor of 'APInt' is ambiguous
 2921 |   APInt ApUMax(32, UINT_MAX, false);
      |         ^      ~~~~~~~~~~~~~~~~~~~
C:\Users\Tcwg\llvm-worker\clang-arm64-windows-msvc-2stage\llvm\llvm\include\llvm/ADT/APInt.h(109,3): note: candidate constructor
  109 |   APInt(unsigned numBits, uint64_t val, bool isSigned = false)
      |   ^
C:\Users\Tcwg\llvm-worker\clang-arm64-windows-msvc-2stage\llvm\llvm\include\llvm/ADT/APInt.h(135,3): note: candidate constructor
  135 |   APInt(unsigned numBits, unsigned numWords, const uint64_t bigVal[]);
      |   ^
1 error generated.

Could you please look at this ?

RKSimon · 2024-03-15T11:48:05Z

I've pushed a fix here 41bdcaa

Atousa marked this pull request as ready for review March 8, 2024 05:33

llvmbot added llvm:support llvm:SelectionDAG SelectionDAGISel as well llvm:adt labels Mar 8, 2024

RKSimon requested review from RKSimon, nikic and goldsteinn March 8, 2024 09:50

RKSimon reviewed Mar 8, 2024

View reviewed changes

RKSimon requested a review from davemgreen March 8, 2024 09:53

jayfoad reviewed Mar 8, 2024

View reviewed changes

Atousa changed the title ~~[ADT] Add implementations for avgFloor and ceilFloor to APInt~~ [ADT] Add implementations for avgFloor and avgCeil to APInt Mar 8, 2024

Atousa force-pushed the main branch from 0d7caaa to 7e98102 Compare March 8, 2024 16:17

jayfoad reviewed Mar 8, 2024

View reviewed changes

Atousa force-pushed the main branch from 7e98102 to 3c787b5 Compare March 8, 2024 16:34

kuhar reviewed Mar 8, 2024

View reviewed changes

llvm/lib/Support/APInt.cpp Outdated Show resolved Hide resolved

Atousa force-pushed the main branch from 3c787b5 to 1980165 Compare March 8, 2024 19:20

RKSimon mentioned this pull request Mar 9, 2024

[DAG] TargetLowering::expandABD - investigate alternative expansions #84639

Closed

arsenm reviewed Mar 11, 2024

View reviewed changes

llvm/unittests/ADT/APIntTest.cpp Show resolved Hide resolved

topperc reviewed Mar 11, 2024

View reviewed changes

Atousa force-pushed the main branch from 1980165 to 16a2205 Compare March 12, 2024 05:35

arsenm reviewed Mar 12, 2024

View reviewed changes

llvm/unittests/ADT/APIntTest.cpp Show resolved Hide resolved

Atousa force-pushed the main branch from 16a2205 to bc7a907 Compare March 12, 2024 07:49

jayfoad reviewed Mar 12, 2024

View reviewed changes

RKSimon reviewed Mar 12, 2024

View reviewed changes

Atousa force-pushed the main branch from bc7a907 to 6c42041 Compare March 13, 2024 05:20

RKSimon mentioned this pull request Mar 13, 2024

[Support] Add KnownBits implementations for avgFloor and avgCeil #84640

Open

3 tasks

RKSimon reviewed Mar 13, 2024

View reviewed changes

[ADT] Add implementations for avgFloor and avgCeil to APInt

362caa1

Supports both signed and unsigned expansions. SelectionDAG now calls the APInt implementation of these functions.

Atousa force-pushed the main branch from 6c42041 to 362caa1 Compare March 14, 2024 04:56

arsenm approved these changes Mar 14, 2024

View reviewed changes

RKSimon approved these changes Mar 14, 2024

View reviewed changes

RKSimon merged commit aff0570 into llvm:main Mar 14, 2024
4 checks passed

RKSimon mentioned this pull request Mar 15, 2024

[ADT] Add signed and unsigned mulh to APInt #84719

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ADT] Add implementations for avgFloor and avgCeil to APInt #84431

[ADT] Add implementations for avgFloor and avgCeil to APInt #84431

Atousa commented Mar 8, 2024 •

edited by RKSimon

llvmbot commented Mar 8, 2024 •

edited

RKSimon Mar 8, 2024

jayfoad commented Mar 8, 2024

jayfoad Mar 8, 2024

jayfoad Mar 8, 2024

jayfoad Mar 8, 2024

jayfoad commented Mar 8, 2024

RKSimon commented Mar 8, 2024

Atousa commented Mar 8, 2024

Atousa commented Mar 8, 2024

RKSimon commented Mar 9, 2024

RKSimon commented Mar 9, 2024

Atousa commented Mar 11, 2024

topperc Mar 11, 2024

jayfoad Mar 12, 2024

RKSimon Mar 12, 2024

RKSimon Mar 12, 2024

RKSimon left a comment

RKSimon Mar 13, 2024

jayfoad Mar 14, 2024

RKSimon Mar 13, 2024

jayfoad Mar 13, 2024

Atousa commented Mar 14, 2024 •

edited

RKSimon left a comment

antmox commented Mar 15, 2024

RKSimon commented Mar 15, 2024

[ADT] Add implementations for avgFloor and avgCeil to APInt #84431

[ADT] Add implementations for avgFloor and avgCeil to APInt #84431

Conversation

Atousa commented Mar 8, 2024 • edited by RKSimon

llvmbot commented Mar 8, 2024 • edited

Choose a reason for hiding this comment

jayfoad commented Mar 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayfoad commented Mar 8, 2024

RKSimon commented Mar 8, 2024

Atousa commented Mar 8, 2024

Atousa commented Mar 8, 2024

RKSimon commented Mar 9, 2024

RKSimon commented Mar 9, 2024

Atousa commented Mar 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RKSimon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Atousa commented Mar 14, 2024 • edited

RKSimon left a comment

Choose a reason for hiding this comment

antmox commented Mar 15, 2024

RKSimon commented Mar 15, 2024

Atousa commented Mar 8, 2024 •

edited by RKSimon

llvmbot commented Mar 8, 2024 •

edited

Atousa commented Mar 14, 2024 •

edited