[APInt] Remove multiplicativeInverse with explicit modulus #87644
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
All callers have been changed to use the new simpler overload with an implicit modulus of 2^BitWidth. The old form was never used or tested with non-power-of-two modulus anyway.
|
@llvm/pr-subscribers-llvm-adt Author: Jay Foad (jayfoad) ChangesAll callers have been changed to use the new simpler overload with an Full diff: https://github.com/llvm/llvm-project/pull/87644.diff 3 Files Affected:
diff --git a/llvm/include/llvm/ADT/APInt.h b/llvm/include/llvm/ADT/APInt.h
index bd1716219ee5fc..8d3c029b2e7e91 100644
--- a/llvm/include/llvm/ADT/APInt.h
+++ b/llvm/include/llvm/ADT/APInt.h
@@ -1740,9 +1740,6 @@ class [[nodiscard]] APInt {
return *this;
}
- /// \returns the multiplicative inverse for a given modulo.
- APInt multiplicativeInverse(const APInt &modulo) const;
-
/// \returns the multiplicative inverse of an odd APInt modulo 2^BitWidth.
APInt multiplicativeInverse() const;
diff --git a/llvm/lib/Support/APInt.cpp b/llvm/lib/Support/APInt.cpp
index f8f699f8f6ccd7..224ea0924f0aaa 100644
--- a/llvm/lib/Support/APInt.cpp
+++ b/llvm/lib/Support/APInt.cpp
@@ -1240,55 +1240,6 @@ APInt APInt::sqrt() const {
return x_old + 1;
}
-/// Computes the multiplicative inverse of this APInt for a given modulo. The
-/// iterative extended Euclidean algorithm is used to solve for this value,
-/// however we simplify it to speed up calculating only the inverse, and take
-/// advantage of div+rem calculations. We also use some tricks to avoid copying
-/// (potentially large) APInts around.
-/// WARNING: a value of '0' may be returned,
-/// signifying that no multiplicative inverse exists!
-APInt APInt::multiplicativeInverse(const APInt& modulo) const {
- assert(ult(modulo) && "This APInt must be smaller than the modulo");
-
- // Using the properties listed at the following web page (accessed 06/21/08):
- // http://www.numbertheory.org/php/euclid.html
- // (especially the properties numbered 3, 4 and 9) it can be proved that
- // BitWidth bits suffice for all the computations in the algorithm implemented
- // below. More precisely, this number of bits suffice if the multiplicative
- // inverse exists, but may not suffice for the general extended Euclidean
- // algorithm.
-
- APInt r[2] = { modulo, *this };
- APInt t[2] = { APInt(BitWidth, 0), APInt(BitWidth, 1) };
- APInt q(BitWidth, 0);
-
- unsigned i;
- for (i = 0; r[i^1] != 0; i ^= 1) {
- // An overview of the math without the confusing bit-flipping:
- // q = r[i-2] / r[i-1]
- // r[i] = r[i-2] % r[i-1]
- // t[i] = t[i-2] - t[i-1] * q
- udivrem(r[i], r[i^1], q, r[i]);
- t[i] -= t[i^1] * q;
- }
-
- // If this APInt and the modulo are not coprime, there is no multiplicative
- // inverse, so return 0. We check this by looking at the next-to-last
- // remainder, which is the gcd(*this,modulo) as calculated by the Euclidean
- // algorithm.
- if (r[i] != 1)
- return APInt(BitWidth, 0);
-
- // The next-to-last t is the multiplicative inverse. However, we are
- // interested in a positive inverse. Calculate a positive one from a negative
- // one if necessary. A simple addition of the modulo suffices because
- // abs(t[i]) is known to be less than *this/2 (see the link above).
- if (t[i].isNegative())
- t[i] += modulo;
-
- return std::move(t[i]);
-}
-
/// \returns the multiplicative inverse of an odd APInt modulo 2^BitWidth.
APInt APInt::multiplicativeInverse() const {
assert((*this)[0] &&
diff --git a/llvm/unittests/ADT/APIntTest.cpp b/llvm/unittests/ADT/APIntTest.cpp
index 23f9ee2d39c441..76fc26412407e7 100644
--- a/llvm/unittests/ADT/APIntTest.cpp
+++ b/llvm/unittests/ADT/APIntTest.cpp
@@ -3249,22 +3249,11 @@ TEST(APIntTest, SolveQuadraticEquationWrap) {
}
TEST(APIntTest, MultiplicativeInverseExaustive) {
- for (unsigned BitWidth = 1; BitWidth <= 16; ++BitWidth) {
- for (unsigned Value = 0; Value < (1u << BitWidth); ++Value) {
+ for (unsigned BitWidth = 1; BitWidth <= 8; ++BitWidth) {
+ for (unsigned Value = 1; Value < (1u << BitWidth); Value += 2) {
+ // Multiplicative inverse exists for all odd numbers.
APInt V = APInt(BitWidth, Value);
- APInt MulInv =
- V.zext(BitWidth + 1)
- .multiplicativeInverse(APInt::getSignedMinValue(BitWidth + 1))
- .trunc(BitWidth);
- APInt One = V * MulInv;
- if (V[0]) {
- // Multiplicative inverse exists for all odd numbers.
- EXPECT_TRUE(One.isOne());
- EXPECT_TRUE((V * V.multiplicativeInverse()).isOne());
- } else {
- // Multiplicative inverse does not exist for even numbers (and 0).
- EXPECT_TRUE(MulInv.isZero());
- }
+ EXPECT_EQ(V * V.multiplicativeInverse(), 1);
}
}
}
|
|
@llvm/pr-subscribers-llvm-support Author: Jay Foad (jayfoad) ChangesAll callers have been changed to use the new simpler overload with an Full diff: https://github.com/llvm/llvm-project/pull/87644.diff 3 Files Affected:
diff --git a/llvm/include/llvm/ADT/APInt.h b/llvm/include/llvm/ADT/APInt.h
index bd1716219ee5fc..8d3c029b2e7e91 100644
--- a/llvm/include/llvm/ADT/APInt.h
+++ b/llvm/include/llvm/ADT/APInt.h
@@ -1740,9 +1740,6 @@ class [[nodiscard]] APInt {
return *this;
}
- /// \returns the multiplicative inverse for a given modulo.
- APInt multiplicativeInverse(const APInt &modulo) const;
-
/// \returns the multiplicative inverse of an odd APInt modulo 2^BitWidth.
APInt multiplicativeInverse() const;
diff --git a/llvm/lib/Support/APInt.cpp b/llvm/lib/Support/APInt.cpp
index f8f699f8f6ccd7..224ea0924f0aaa 100644
--- a/llvm/lib/Support/APInt.cpp
+++ b/llvm/lib/Support/APInt.cpp
@@ -1240,55 +1240,6 @@ APInt APInt::sqrt() const {
return x_old + 1;
}
-/// Computes the multiplicative inverse of this APInt for a given modulo. The
-/// iterative extended Euclidean algorithm is used to solve for this value,
-/// however we simplify it to speed up calculating only the inverse, and take
-/// advantage of div+rem calculations. We also use some tricks to avoid copying
-/// (potentially large) APInts around.
-/// WARNING: a value of '0' may be returned,
-/// signifying that no multiplicative inverse exists!
-APInt APInt::multiplicativeInverse(const APInt& modulo) const {
- assert(ult(modulo) && "This APInt must be smaller than the modulo");
-
- // Using the properties listed at the following web page (accessed 06/21/08):
- // http://www.numbertheory.org/php/euclid.html
- // (especially the properties numbered 3, 4 and 9) it can be proved that
- // BitWidth bits suffice for all the computations in the algorithm implemented
- // below. More precisely, this number of bits suffice if the multiplicative
- // inverse exists, but may not suffice for the general extended Euclidean
- // algorithm.
-
- APInt r[2] = { modulo, *this };
- APInt t[2] = { APInt(BitWidth, 0), APInt(BitWidth, 1) };
- APInt q(BitWidth, 0);
-
- unsigned i;
- for (i = 0; r[i^1] != 0; i ^= 1) {
- // An overview of the math without the confusing bit-flipping:
- // q = r[i-2] / r[i-1]
- // r[i] = r[i-2] % r[i-1]
- // t[i] = t[i-2] - t[i-1] * q
- udivrem(r[i], r[i^1], q, r[i]);
- t[i] -= t[i^1] * q;
- }
-
- // If this APInt and the modulo are not coprime, there is no multiplicative
- // inverse, so return 0. We check this by looking at the next-to-last
- // remainder, which is the gcd(*this,modulo) as calculated by the Euclidean
- // algorithm.
- if (r[i] != 1)
- return APInt(BitWidth, 0);
-
- // The next-to-last t is the multiplicative inverse. However, we are
- // interested in a positive inverse. Calculate a positive one from a negative
- // one if necessary. A simple addition of the modulo suffices because
- // abs(t[i]) is known to be less than *this/2 (see the link above).
- if (t[i].isNegative())
- t[i] += modulo;
-
- return std::move(t[i]);
-}
-
/// \returns the multiplicative inverse of an odd APInt modulo 2^BitWidth.
APInt APInt::multiplicativeInverse() const {
assert((*this)[0] &&
diff --git a/llvm/unittests/ADT/APIntTest.cpp b/llvm/unittests/ADT/APIntTest.cpp
index 23f9ee2d39c441..76fc26412407e7 100644
--- a/llvm/unittests/ADT/APIntTest.cpp
+++ b/llvm/unittests/ADT/APIntTest.cpp
@@ -3249,22 +3249,11 @@ TEST(APIntTest, SolveQuadraticEquationWrap) {
}
TEST(APIntTest, MultiplicativeInverseExaustive) {
- for (unsigned BitWidth = 1; BitWidth <= 16; ++BitWidth) {
- for (unsigned Value = 0; Value < (1u << BitWidth); ++Value) {
+ for (unsigned BitWidth = 1; BitWidth <= 8; ++BitWidth) {
+ for (unsigned Value = 1; Value < (1u << BitWidth); Value += 2) {
+ // Multiplicative inverse exists for all odd numbers.
APInt V = APInt(BitWidth, Value);
- APInt MulInv =
- V.zext(BitWidth + 1)
- .multiplicativeInverse(APInt::getSignedMinValue(BitWidth + 1))
- .trunc(BitWidth);
- APInt One = V * MulInv;
- if (V[0]) {
- // Multiplicative inverse exists for all odd numbers.
- EXPECT_TRUE(One.isOne());
- EXPECT_TRUE((V * V.multiplicativeInverse()).isOne());
- } else {
- // Multiplicative inverse does not exist for even numbers (and 0).
- EXPECT_TRUE(MulInv.isZero());
- }
+ EXPECT_EQ(V * V.multiplicativeInverse(), 1);
}
}
}
|
jayfoad
commented
Apr 4, 2024
| @@ -3249,22 +3249,11 @@ TEST(APIntTest, SolveQuadraticEquationWrap) { | |||
| } | |||
|
|
|||
| TEST(APIntTest, MultiplicativeInverseExaustive) { | |||
| for (unsigned BitWidth = 1; BitWidth <= 16; ++BitWidth) { | |||
| for (unsigned Value = 0; Value < (1u << BitWidth); ++Value) { | |||
| for (unsigned BitWidth = 1; BitWidth <= 8; ++BitWidth) { | |||
There was a problem hiding this comment.
Testing 8 different bitwidths seems exhaustive enough to me.
kuhar
approved these changes
Apr 4, 2024
|
What is the impetus for removing this? We have out-of-tree uses of this method for non-power-of-two moduli. |
|
Note, this will also have valid uses in MLIR as part of a new polynomial dialect (cf. #72081, subsequent PRs will have lowerings that need this inverse calculation) |
Just that there are no in-tree uses. If you need it out-of-tree you can reinstate it out-of-tree. |
Fair enough, when there are in-tree MLIR uses we can reinstate it (but hopefully with proper test coverage). |
j2kun
added a commit
to j2kun/llvm-project
that referenced
this pull request
Apr 5, 2024
…lvm#87644)" This reverts commit 0b293e8. There are out-of-tree uses of this method, and it is planned to be used as part of a new polynomial dialect in MLIR, a starting PR of which is llvm#72081 (later PRs will add lowerings that need the removed functionality)
copybara-service bot
pushed a commit
to google/heir
that referenced
this pull request
Apr 5, 2024
This function was removed in llvm/llvm-project#87644. Fixes #596 PiperOrigin-RevId: 622229242
copybara-service bot
pushed a commit
to google/heir
that referenced
this pull request
Apr 5, 2024
This function was removed in llvm/llvm-project#87644. Fixes #596 PiperOrigin-RevId: 622242876
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All callers have been changed to use the new simpler overload with an
implicit modulus of 2^BitWidth. The old form was never used or tested
with non-power-of-two modulus anyway.