[KnownBits] Add KnownBits::add and KnownBits::sub helper wrappers. #99468

RKSimon · 2024-07-18T10:44:49Z

No description provided.

llvmbot · 2024-07-18T10:45:22Z

@llvm/pr-subscribers-llvm-globalisel
@llvm/pr-subscribers-llvm-transforms
@llvm/pr-subscribers-backend-arm
@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-llvm-support

Author: Simon Pilgrim (RKSimon)

Changes

Assumes NSW = NUW = false

Simplifies uses in callbacks by avoiding the need for a custom KnownBits::computeForAddSub wrapper

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

4 Files Affected:

(modified) llvm/include/llvm/Support/KnownBits.h (+13)
(modified) llvm/lib/Analysis/ValueTracking.cpp (+4-14)
(modified) llvm/lib/Target/X86/X86ISelLowering.cpp (+7-8)
(modified) llvm/unittests/Support/KnownBitsTest.cpp (+8)

diff --git a/llvm/include/llvm/Support/KnownBits.h b/llvm/include/llvm/Support/KnownBits.h
index 7ed3d525bd8fb..c49b0395b268b 100644
--- a/llvm/include/llvm/Support/KnownBits.h
+++ b/llvm/include/llvm/Support/KnownBits.h
@@ -329,6 +329,19 @@ struct KnownBits {
   static KnownBits computeForSubBorrow(const KnownBits &LHS, KnownBits RHS,
                                        const KnownBits &Borrow);
 
+  /// Compute knownbits resulting from addition of LHS and RHS.
+  /// NSW and NUW flags are assumed to be false.
+  static KnownBits add(const KnownBits& LHS, const KnownBits& RHS) {
+    return computeForAddSub(/*Add=*/true, /*NSW=*/false, /*NUW=*/false, LHS,
+                            RHS);
+  }
+  /// Compute knownbits resulting from subtraction of LHS and RHS.
+  /// NSW and NUW flags are assumed to be false.
+  static KnownBits sub(const KnownBits& LHS, const KnownBits& RHS) {
+    return computeForAddSub(/*Add=*/false, /*NSW=*/false, /*NUW=*/false, LHS,
+                            RHS);
+  }
+
   /// Compute knownbits resulting from llvm.sadd.sat(LHS, RHS)
   static KnownBits sadd_sat(const KnownBits &LHS, const KnownBits &RHS);
 
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 535a248a5f1a2..64386f6c11c08 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -1800,13 +1800,8 @@ static void computeKnownBitsFromOperator(const Operator *I,
       case Intrinsic::x86_ssse3_phadd_w_128:
       case Intrinsic::x86_avx2_phadd_d:
       case Intrinsic::x86_avx2_phadd_w: {
-        Known = computeKnownBitsForHorizontalOperation(
-            I, DemandedElts, Depth, Q,
-            [](const KnownBits &KnownLHS, const KnownBits &KnownRHS) {
-              return KnownBits::computeForAddSub(/*Add=*/true, /*NSW=*/false,
-                                                 /*NUW=*/false, KnownLHS,
-                                                 KnownRHS);
-            });
+        Known = computeKnownBitsForHorizontalOperation(I, DemandedElts, Depth,
+                                                       Q, KnownBits::add);
         break;
       }
       case Intrinsic::x86_ssse3_phadd_sw_128:
@@ -1819,13 +1814,8 @@ static void computeKnownBitsFromOperator(const Operator *I,
       case Intrinsic::x86_ssse3_phsub_w_128:
       case Intrinsic::x86_avx2_phsub_d:
       case Intrinsic::x86_avx2_phsub_w: {
-        Known = computeKnownBitsForHorizontalOperation(
-            I, DemandedElts, Depth, Q,
-            [](const KnownBits &KnownLHS, const KnownBits &KnownRHS) {
-              return KnownBits::computeForAddSub(/*Add=*/false, /*NSW=*/false,
-                                                 /*NUW=*/false, KnownLHS,
-                                                 KnownRHS);
-            });
+        Known = computeKnownBitsForHorizontalOperation(I, DemandedElts, Depth,
+                                                       Q, KnownBits::sub);
         break;
       }
       case Intrinsic::x86_ssse3_phsub_sw_128:
diff --git a/llvm/lib/Target/X86/X86ISelLowering.cpp b/llvm/lib/Target/X86/X86ISelLowering.cpp
index 881e06e5f78b4..16d32d5fe040c 100644
--- a/llvm/lib/Target/X86/X86ISelLowering.cpp
+++ b/llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -37511,15 +37511,14 @@ void X86TargetLowering::computeKnownBitsForTargetNode(const SDValue Op,
     }
     break;
   }
-  case X86ISD::HADD:
+  case X86ISD::HADD: {
+    Known = computeKnownBitsForHorizontalOperation(Op, DemandedElts, Depth, DAG,
+                                                   KnownBits::add);
+    break;
+  }
   case X86ISD::HSUB: {
-    Known = computeKnownBitsForHorizontalOperation(
-        Op, DemandedElts, Depth, DAG,
-        [Opc](const KnownBits &KnownLHS, const KnownBits &KnownRHS) {
-          return KnownBits::computeForAddSub(
-              /*Add=*/Opc == X86ISD::HADD, /*NSW=*/false, /*NUW=*/false,
-              KnownLHS, KnownRHS);
-        });
+    Known = computeKnownBitsForHorizontalOperation(Op, DemandedElts, Depth, DAG,
+                                                   KnownBits::sub);
     break;
   }
   case ISD::INTRINSIC_WO_CHAIN: {
diff --git a/llvm/unittests/Support/KnownBitsTest.cpp b/llvm/unittests/Support/KnownBitsTest.cpp
index 84882550117d8..9d2fcf77b29cc 100644
--- a/llvm/unittests/Support/KnownBitsTest.cpp
+++ b/llvm/unittests/Support/KnownBitsTest.cpp
@@ -300,6 +300,14 @@ TEST(KnownBitsTest, BinaryExhaustive) {
         return Known1 ^ Known2;
       },
       [](const APInt &N1, const APInt &N2) { return N1 ^ N2; });
+  testBinaryOpExhaustive(
+      "add", KnownBits::add,
+      [](const APInt &N1, const APInt &N2) { return N1 + N2; },
+      /*CheckOptimality=*/false);
+  testBinaryOpExhaustive(
+      "sub", KnownBits::sub,
+      [](const APInt &N1, const APInt &N2) { return N1 - N2; },
+      /*CheckOptimality=*/false);
   testBinaryOpExhaustive("umax", KnownBits::umax, APIntOps::umax);
   testBinaryOpExhaustive("umin", KnownBits::umin, APIntOps::umin);
   testBinaryOpExhaustive("smax", KnownBits::smax, APIntOps::smax);

llvmbot · 2024-07-18T10:45:22Z

@llvm/pr-subscribers-llvm-analysis

Author: Simon Pilgrim (RKSimon)

Changes

Assumes NSW = NUW = false

Simplifies uses in callbacks by avoiding the need for a custom KnownBits::computeForAddSub wrapper

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

4 Files Affected:

(modified) llvm/include/llvm/Support/KnownBits.h (+13)
(modified) llvm/lib/Analysis/ValueTracking.cpp (+4-14)
(modified) llvm/lib/Target/X86/X86ISelLowering.cpp (+7-8)
(modified) llvm/unittests/Support/KnownBitsTest.cpp (+8)

diff --git a/llvm/include/llvm/Support/KnownBits.h b/llvm/include/llvm/Support/KnownBits.h
index 7ed3d525bd8fb..c49b0395b268b 100644
--- a/llvm/include/llvm/Support/KnownBits.h
+++ b/llvm/include/llvm/Support/KnownBits.h
@@ -329,6 +329,19 @@ struct KnownBits {
   static KnownBits computeForSubBorrow(const KnownBits &LHS, KnownBits RHS,
                                        const KnownBits &Borrow);
 
+  /// Compute knownbits resulting from addition of LHS and RHS.
+  /// NSW and NUW flags are assumed to be false.
+  static KnownBits add(const KnownBits& LHS, const KnownBits& RHS) {
+    return computeForAddSub(/*Add=*/true, /*NSW=*/false, /*NUW=*/false, LHS,
+                            RHS);
+  }
+  /// Compute knownbits resulting from subtraction of LHS and RHS.
+  /// NSW and NUW flags are assumed to be false.
+  static KnownBits sub(const KnownBits& LHS, const KnownBits& RHS) {
+    return computeForAddSub(/*Add=*/false, /*NSW=*/false, /*NUW=*/false, LHS,
+                            RHS);
+  }
+
   /// Compute knownbits resulting from llvm.sadd.sat(LHS, RHS)
   static KnownBits sadd_sat(const KnownBits &LHS, const KnownBits &RHS);
 
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 535a248a5f1a2..64386f6c11c08 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -1800,13 +1800,8 @@ static void computeKnownBitsFromOperator(const Operator *I,
       case Intrinsic::x86_ssse3_phadd_w_128:
       case Intrinsic::x86_avx2_phadd_d:
       case Intrinsic::x86_avx2_phadd_w: {
-        Known = computeKnownBitsForHorizontalOperation(
-            I, DemandedElts, Depth, Q,
-            [](const KnownBits &KnownLHS, const KnownBits &KnownRHS) {
-              return KnownBits::computeForAddSub(/*Add=*/true, /*NSW=*/false,
-                                                 /*NUW=*/false, KnownLHS,
-                                                 KnownRHS);
-            });
+        Known = computeKnownBitsForHorizontalOperation(I, DemandedElts, Depth,
+                                                       Q, KnownBits::add);
         break;
       }
       case Intrinsic::x86_ssse3_phadd_sw_128:
@@ -1819,13 +1814,8 @@ static void computeKnownBitsFromOperator(const Operator *I,
       case Intrinsic::x86_ssse3_phsub_w_128:
       case Intrinsic::x86_avx2_phsub_d:
       case Intrinsic::x86_avx2_phsub_w: {
-        Known = computeKnownBitsForHorizontalOperation(
-            I, DemandedElts, Depth, Q,
-            [](const KnownBits &KnownLHS, const KnownBits &KnownRHS) {
-              return KnownBits::computeForAddSub(/*Add=*/false, /*NSW=*/false,
-                                                 /*NUW=*/false, KnownLHS,
-                                                 KnownRHS);
-            });
+        Known = computeKnownBitsForHorizontalOperation(I, DemandedElts, Depth,
+                                                       Q, KnownBits::sub);
         break;
       }
       case Intrinsic::x86_ssse3_phsub_sw_128:
diff --git a/llvm/lib/Target/X86/X86ISelLowering.cpp b/llvm/lib/Target/X86/X86ISelLowering.cpp
index 881e06e5f78b4..16d32d5fe040c 100644
--- a/llvm/lib/Target/X86/X86ISelLowering.cpp
+++ b/llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -37511,15 +37511,14 @@ void X86TargetLowering::computeKnownBitsForTargetNode(const SDValue Op,
     }
     break;
   }
-  case X86ISD::HADD:
+  case X86ISD::HADD: {
+    Known = computeKnownBitsForHorizontalOperation(Op, DemandedElts, Depth, DAG,
+                                                   KnownBits::add);
+    break;
+  }
   case X86ISD::HSUB: {
-    Known = computeKnownBitsForHorizontalOperation(
-        Op, DemandedElts, Depth, DAG,
-        [Opc](const KnownBits &KnownLHS, const KnownBits &KnownRHS) {
-          return KnownBits::computeForAddSub(
-              /*Add=*/Opc == X86ISD::HADD, /*NSW=*/false, /*NUW=*/false,
-              KnownLHS, KnownRHS);
-        });
+    Known = computeKnownBitsForHorizontalOperation(Op, DemandedElts, Depth, DAG,
+                                                   KnownBits::sub);
     break;
   }
   case ISD::INTRINSIC_WO_CHAIN: {
diff --git a/llvm/unittests/Support/KnownBitsTest.cpp b/llvm/unittests/Support/KnownBitsTest.cpp
index 84882550117d8..9d2fcf77b29cc 100644
--- a/llvm/unittests/Support/KnownBitsTest.cpp
+++ b/llvm/unittests/Support/KnownBitsTest.cpp
@@ -300,6 +300,14 @@ TEST(KnownBitsTest, BinaryExhaustive) {
         return Known1 ^ Known2;
       },
       [](const APInt &N1, const APInt &N2) { return N1 ^ N2; });
+  testBinaryOpExhaustive(
+      "add", KnownBits::add,
+      [](const APInt &N1, const APInt &N2) { return N1 + N2; },
+      /*CheckOptimality=*/false);
+  testBinaryOpExhaustive(
+      "sub", KnownBits::sub,
+      [](const APInt &N1, const APInt &N2) { return N1 - N2; },
+      /*CheckOptimality=*/false);
   testBinaryOpExhaustive("umax", KnownBits::umax, APIntOps::umax);
   testBinaryOpExhaustive("umin", KnownBits::umin, APIntOps::umin);
   testBinaryOpExhaustive("smax", KnownBits::smax, APIntOps::smax);

github-actions · 2024-07-18T10:48:47Z

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

jayfoad · 2024-07-18T10:53:33Z

llvm/include/llvm/Support/KnownBits.h

@@ -329,6 +329,19 @@ struct KnownBits {
  static KnownBits computeForSubBorrow(const KnownBits &LHS, KnownBits RHS,
                                       const KnownBits &Borrow);

+  /// Compute knownbits resulting from addition of LHS and RHS.
+  /// NSW and NUW flags are assumed to be false.
+  static KnownBits add(const KnownBits& LHS, const KnownBits& RHS) {


Why not call it operator+?

Mainly because I loathe operators for this kind of stuff :)

Well we already use them for &, | and ^...

*cough* and we shouldn't *cough*

goldsteinn · 2024-07-18T12:12:14Z

I'm opposed to no option for NUW/NSW. I think this will probably become the defacto API and think this basically encourages people, especially new contributors, to forgetting/ignoring the flags.

Can you add them as default false params?

RKSimon · 2024-08-09T13:19:46Z

I've added NSW/NUW arguments that default to false - although this means we can't easily use these in llvm::function_ref callbacks anymore

goldsteinn · 2024-08-10T15:02:11Z

I've added NSW/NUW arguments that default to false - although this means we can't easily use these in llvm::function_ref callbacks anymore

You could add a wrapper addNoWrap and subNoWrap for the callback case.

goldsteinn · 2024-08-10T15:04:58Z

LGTM

RKSimon requested review from jayfoad and goldsteinn July 18, 2024 10:44

RKSimon requested a review from nikic as a code owner July 18, 2024 10:44

llvmbot added backend:X86 llvm:support llvm:analysis Includes value tracking, cost tables and constant folding labels Jul 18, 2024

jayfoad reviewed Jul 18, 2024

View reviewed changes

[KnownBits] Add KnownBits::add and KnownBits::sub helper wrappers.

9cc7de8

RKSimon force-pushed the knownbits_addsub branch from 3339ffe to 9cc7de8 Compare August 9, 2024 13:17

llvmbot added backend:ARM backend:AMDGPU llvm:globalisel llvm:transforms labels Aug 9, 2024

goldsteinn approved these changes Aug 10, 2024

View reviewed changes

arsenm approved these changes Aug 12, 2024

View reviewed changes

RKSimon merged commit 11ba72e into llvm:main Aug 12, 2024
12 checks passed

RKSimon deleted the knownbits_addsub branch August 12, 2024 09:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[KnownBits] Add KnownBits::add and KnownBits::sub helper wrappers. #99468

[KnownBits] Add KnownBits::add and KnownBits::sub helper wrappers. #99468

Uh oh!

RKSimon commented Jul 18, 2024 •

edited

Loading

Uh oh!

llvmbot commented Jul 18, 2024 •

edited

Loading

Uh oh!

llvmbot commented Jul 18, 2024

Uh oh!

github-actions bot commented Jul 18, 2024 •

edited

Loading

Uh oh!

jayfoad Jul 18, 2024

Uh oh!

RKSimon Jul 18, 2024

Uh oh!

jayfoad Jul 18, 2024

Uh oh!

goldsteinn Jul 18, 2024

Uh oh!

goldsteinn commented Jul 18, 2024

Uh oh!

RKSimon commented Aug 9, 2024

Uh oh!

goldsteinn commented Aug 10, 2024

Uh oh!

goldsteinn commented Aug 10, 2024

Uh oh!

Uh oh!

Uh oh!

[KnownBits] Add KnownBits::add and KnownBits::sub helper wrappers. #99468

[KnownBits] Add KnownBits::add and KnownBits::sub helper wrappers. #99468

Uh oh!

Conversation

RKSimon commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 18, 2024

Uh oh!

github-actions bot commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayfoad Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

RKSimon Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

jayfoad Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

goldsteinn Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

goldsteinn commented Jul 18, 2024

Uh oh!

RKSimon commented Aug 9, 2024

Uh oh!

goldsteinn commented Aug 10, 2024

Uh oh!

goldsteinn commented Aug 10, 2024

Uh oh!

Uh oh!

Uh oh!

RKSimon commented Jul 18, 2024 •

edited

Loading

llvmbot commented Jul 18, 2024 •

edited

Loading

github-actions bot commented Jul 18, 2024 •

edited

Loading