[ADT] Add APIntOps::abds signed absolute difference and rename absdiff -> abdu #84791

RKSimon · 2024-03-11T16:40:39Z

When I created APIntOps::absdiff, I totally missed that we already have ISD::ABDS/ABDU nodes, and we use this term in other places/targets as well.

I've added the APIntOps::abds implementation and renamed APIntOps::absdiff to APIntOps::abdu.

Given that APIntOps::absdiff is so young I don't think we need to create a deprecation wrapper, but I can if anyone thinks it important.

I'll do a KnownBits rename patch after this.

llvmbot · 2024-03-11T16:41:09Z

@llvm/pr-subscribers-llvm-selectiondag
@llvm/pr-subscribers-llvm-support

@llvm/pr-subscribers-llvm-adt

Author: Simon Pilgrim (RKSimon)

Changes

When I created APIntOps::absdiff, I totally missed that we already have ISD::ABDS/ABDU nodes, and we use this term in other places/targets as well.

I've added the APIntOps::abds implementation and renamed APIntOps::absdiff to APIntOps::abdu.

Given that APIntOps::absdiff is so young I don't think we need to create a deprecation wrapper, but I can if anyone thinks it important.

I'll do a KnownBits rename patch after this.

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

4 Files Affected:

(modified) llvm/include/llvm/ADT/APInt.h (+6-1)
(modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (+2-2)
(modified) llvm/unittests/ADT/APIntTest.cpp (+50-16)
(modified) llvm/unittests/Support/KnownBitsTest.cpp (+1-1)

diff --git a/llvm/include/llvm/ADT/APInt.h b/llvm/include/llvm/ADT/APInt.h
index 1fc3c7b2236a17..bea3e28adf308f 100644
--- a/llvm/include/llvm/ADT/APInt.h
+++ b/llvm/include/llvm/ADT/APInt.h
@@ -2188,8 +2188,13 @@ inline const APInt &umax(const APInt &A, const APInt &B) {
   return A.ugt(B) ? A : B;
 }
 
+/// Determine the absolute difference of two APInts considered to be signed.
+inline const APInt abds(const APInt &A, const APInt &B) {
+  return A.sge(B) ? (A - B) : (B - A);
+}
+
 /// Determine the absolute difference of two APInts considered to be unsigned.
-inline const APInt absdiff(const APInt &A, const APInt &B) {
+inline const APInt abdu(const APInt &A, const APInt &B) {
   return A.uge(B) ? (A - B) : (B - A);
 }
 
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index 06fe716a22db0f..ccd4025fafe796 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -6066,9 +6066,9 @@ static std::optional<APInt> FoldValue(unsigned Opcode, const APInt &C1,
     return (C1Ext + C2Ext + 1).extractBits(C1.getBitWidth(), 1);
   }
   case ISD::ABDS:
-    return APIntOps::smax(C1, C2) - APIntOps::smin(C1, C2);
+    return APIntOps::abds(C1, C2);
   case ISD::ABDU:
-    return APIntOps::umax(C1, C2) - APIntOps::umin(C1, C2);
+    return APIntOps::abdu(C1, C2);
   }
   return std::nullopt;
 }
diff --git a/llvm/unittests/ADT/APIntTest.cpp b/llvm/unittests/ADT/APIntTest.cpp
index 24324822356bf6..11237d2e1602dc 100644
--- a/llvm/unittests/ADT/APIntTest.cpp
+++ b/llvm/unittests/ADT/APIntTest.cpp
@@ -2532,38 +2532,72 @@ TEST(APIntTest, clearLowBits) {
   EXPECT_EQ(16u, i32hi16.popcount());
 }
 
-TEST(APIntTest, AbsDiff) {
-  using APIntOps::absdiff;
+TEST(APIntTest, abds) {
+  using APIntOps::abds;
 
   APInt MaxU1(1, 1, false);
   APInt MinU1(1, 0, false);
-  EXPECT_EQ(1u, absdiff(MaxU1, MinU1).getZExtValue());
-  EXPECT_EQ(1u, absdiff(MinU1, MaxU1).getZExtValue());
+  EXPECT_EQ(1u, abds(MaxU1, MinU1).getZExtValue());
+  EXPECT_EQ(1u, abds(MinU1, MaxU1).getZExtValue());
 
   APInt MaxU4(4, 15, false);
   APInt MinU4(4, 0, false);
-  EXPECT_EQ(15u, absdiff(MaxU4, MinU4).getZExtValue());
-  EXPECT_EQ(15u, absdiff(MinU4, MaxU4).getZExtValue());
+  EXPECT_EQ(1, abds(MaxU4, MinU4).getSExtValue());
+  EXPECT_EQ(1, abds(MinU4, MaxU4).getSExtValue());
 
   APInt MaxS8(8, 127, true);
   APInt MinS8(8, -128, true);
-  EXPECT_EQ(1u, absdiff(MaxS8, MinS8).getZExtValue());
-  EXPECT_EQ(1u, absdiff(MinS8, MaxS8).getZExtValue());
+  EXPECT_EQ(-1, abds(MaxS8, MinS8).getSExtValue());
+  EXPECT_EQ(-1, abds(MinS8, MaxS8).getSExtValue());
 
   APInt MaxU16(16, 65535, false);
   APInt MinU16(16, 0, false);
-  EXPECT_EQ(65535u, absdiff(MaxU16, MinU16).getZExtValue());
-  EXPECT_EQ(65535u, absdiff(MinU16, MaxU16).getZExtValue());
+  EXPECT_EQ(1, abds(MaxU16, MinU16).getSExtValue());
+  EXPECT_EQ(1, abds(MinU16, MaxU16).getSExtValue());
 
   APInt MaxS16(16, 32767, true);
   APInt MinS16(16, -32768, true);
   APInt ZeroS16(16, 0, true);
-  EXPECT_EQ(1u, absdiff(MaxS16, MinS16).getZExtValue());
-  EXPECT_EQ(1u, absdiff(MinS16, MaxS16).getZExtValue());
-  EXPECT_EQ(32768u, absdiff(ZeroS16, MinS16));
-  EXPECT_EQ(32768u, absdiff(MinS16, ZeroS16));
-  EXPECT_EQ(32767u, absdiff(ZeroS16, MaxS16));
-  EXPECT_EQ(32767u, absdiff(MaxS16, ZeroS16));
+  EXPECT_EQ(-1, abds(MaxS16, MinS16).getSExtValue());
+  EXPECT_EQ(-1, abds(MinS16, MaxS16).getSExtValue());
+  EXPECT_EQ(32768u, abds(ZeroS16, MinS16));
+  EXPECT_EQ(32768u, abds(MinS16, ZeroS16));
+  EXPECT_EQ(32767u, abds(ZeroS16, MaxS16));
+  EXPECT_EQ(32767u, abds(MaxS16, ZeroS16));
+}
+
+TEST(APIntTest, abdu) {
+  using APIntOps::abdu;
+
+  APInt MaxU1(1, 1, false);
+  APInt MinU1(1, 0, false);
+  EXPECT_EQ(1u, abdu(MaxU1, MinU1).getZExtValue());
+  EXPECT_EQ(1u, abdu(MinU1, MaxU1).getZExtValue());
+
+  APInt MaxU4(4, 15, false);
+  APInt MinU4(4, 0, false);
+  EXPECT_EQ(15u, abdu(MaxU4, MinU4).getZExtValue());
+  EXPECT_EQ(15u, abdu(MinU4, MaxU4).getZExtValue());
+
+  APInt MaxS8(8, 127, true);
+  APInt MinS8(8, -128, true);
+  EXPECT_EQ(1u, abdu(MaxS8, MinS8).getZExtValue());
+  EXPECT_EQ(1u, abdu(MinS8, MaxS8).getZExtValue());
+
+  APInt MaxU16(16, 65535, false);
+  APInt MinU16(16, 0, false);
+  EXPECT_EQ(65535u, abdu(MaxU16, MinU16).getZExtValue());
+  EXPECT_EQ(65535u, abdu(MinU16, MaxU16).getZExtValue());
+
+  APInt MaxS16(16, 32767, true);
+  APInt MinS16(16, -32768, true);
+  APInt ZeroS16(16, 0, true);
+  EXPECT_EQ(1u, abdu(MaxS16, MinS16).getZExtValue());
+  EXPECT_EQ(1u, abdu(MinS16, MaxS16).getZExtValue());
+  EXPECT_EQ(32768u, abdu(ZeroS16, MinS16));
+  EXPECT_EQ(32768u, abdu(MinS16, ZeroS16));
+  EXPECT_EQ(32767u, abdu(ZeroS16, MaxS16));
+  EXPECT_EQ(32767u, abdu(MaxS16, ZeroS16));
 }
 
 TEST(APIntTest, GCD) {
diff --git a/llvm/unittests/Support/KnownBitsTest.cpp b/llvm/unittests/Support/KnownBitsTest.cpp
index 658f3796721c4e..701876c7e418cd 100644
--- a/llvm/unittests/Support/KnownBitsTest.cpp
+++ b/llvm/unittests/Support/KnownBitsTest.cpp
@@ -362,7 +362,7 @@ TEST(KnownBitsTest, BinaryExhaustive) {
         return KnownBits::absdiff(Known1, Known2);
       },
       [](const APInt &N1, const APInt &N2) {
-        return APIntOps::absdiff(N1, N2);
+        return APIntOps::abdu(N1, N2);
       },
       checkCorrectnessOnlyBinary);
   testBinaryOpExhaustive(

github-actions · 2024-03-11T16:43:24Z

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

jayfoad

LGTM.

jayfoad · 2024-03-11T16:45:22Z

llvm/include/llvm/ADT/APInt.h

@@ -2188,8 +2188,13 @@ inline const APInt &umax(const APInt &A, const APInt &B) {
  return A.ugt(B) ? A : B;
 }

+/// Determine the absolute difference of two APInts considered to be signed.
+inline const APInt abds(const APInt &A, const APInt &B) {
+  return A.sge(B) ? (A - B) : (B - A);


Don't need the parens (but I'm not insisting you remove them if you prefer them).

Thanks, I always tend towards being overly cautious within ternary

jayfoad · 2024-03-11T16:51:26Z

llvm/unittests/Support/KnownBitsTest.cpp

      [](const APInt &N1, const APInt &N2) {
-        return APIntOps::absdiff(N1, N2);
+        return APIntOps::abdu(N1, N2);
      },


Do you need the lambda at all? Can the whole entry be just APIntOps::abdu?

Yes, we can do that - the KnownBits function as well in many cases - although we've tended to keep the lambdas for all cases so far (smin/smax etc. above) - I have no strong preference.

Keeping the lambda seems silly if we don't need it, but we can always clean them all up later.

…f -> abdu When I created APIntOps::absdiff, I totally missed that we already have ISD::ABDS/ABDU nodes, and we use this term in other places/targets as well. I've added the APIntOps::abds implementation and renamed APIntOps::absdiff to APIntOps::abdu. Given that APIntOps::absdiff is so young I don't think we need to create a deprecation wrapper, but I can if anyone thinks it important. I'll do a KnownBits rename patch after this.

…bsdiff -> abdu When I created KnownBits::absdiff, I totally missed that we already have ISD::ABDS/ABDU nodes, and we use this term in other places/targets as well. I've added the KnownBits::abds implementation and renamed KnownBits::absdiff to KnownBits::abdu. Followup to llvm#84791

…bsdiff -> abdu (#84897) When I created KnownBits::absdiff, I totally missed that we already have ISD::ABDS/ABDU nodes, and we use this term in other places/targets as well. I've added the KnownBits::abds implementation and renamed KnownBits::absdiff to KnownBits::abdu. Followup to #84791

RKSimon requested review from jayfoad, nikic and goldsteinn March 11, 2024 16:40

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

jayfoad approved these changes Mar 11, 2024

View reviewed changes

jayfoad reviewed Mar 11, 2024

View reviewed changes

RKSimon force-pushed the apint-abd branch from a3f7c8e to 4303e34 Compare March 12, 2024 10:12

RKSimon merged commit 3358838 into llvm:main Mar 12, 2024
3 of 4 checks passed

RKSimon deleted the apint-abd branch March 12, 2024 10:42

RKSimon mentioned this pull request Mar 12, 2024

[Support] Add KnownBits::abds signed absolute difference and rename absdiff -> abdu #84897

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ADT] Add APIntOps::abds signed absolute difference and rename absdiff -> abdu #84791

[ADT] Add APIntOps::abds signed absolute difference and rename absdiff -> abdu #84791

RKSimon commented Mar 11, 2024

llvmbot commented Mar 11, 2024 •

edited

Loading

github-actions bot commented Mar 11, 2024 •

edited

Loading

jayfoad left a comment

jayfoad Mar 11, 2024

RKSimon Mar 11, 2024

jayfoad Mar 11, 2024

RKSimon Mar 11, 2024

jayfoad Mar 11, 2024

[ADT] Add APIntOps::abds signed absolute difference and rename absdiff -> abdu #84791

[ADT] Add APIntOps::abds signed absolute difference and rename absdiff -> abdu #84791

Conversation

RKSimon commented Mar 11, 2024

llvmbot commented Mar 11, 2024 • edited Loading

github-actions bot commented Mar 11, 2024 • edited Loading

jayfoad left a comment

Choose a reason for hiding this comment

jayfoad Mar 11, 2024

Choose a reason for hiding this comment

RKSimon Mar 11, 2024

Choose a reason for hiding this comment

jayfoad Mar 11, 2024

Choose a reason for hiding this comment

RKSimon Mar 11, 2024

Choose a reason for hiding this comment

jayfoad Mar 11, 2024

Choose a reason for hiding this comment

llvmbot commented Mar 11, 2024 •

edited

Loading

github-actions bot commented Mar 11, 2024 •

edited

Loading