8355992: Add unsignedMultiplyExact and *powExact methods to Math and StrictMath #25003
Conversation
|
👋 Welcome back rgiulietti! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
openjdk
bot
added
csr
|
@rgiulietti The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
|
Tests will be added once the CSR has been approved. |
Webrevs
|
rgiulietti
changed the title
|
Maybe it is too late, but shouldn't there be a better way to structure all these methods and variants in Math and MathExact? x(), xExact() and all the different parameter types create a rather big Cartesian product. |
|
@minborg I'm open to suggestions for the |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| * The reason is that the number of loop iterations below can be kept | ||
| * very small when |x| > 1, but not necessarily when |x| <= 1. | ||
| */ | ||
| if (x == 0 || x == 1) { |
There was a problem hiding this comment.
| if (x == 0 || x == 1) { | |
| if (x == 0 || x == 1 || n == 1) { |
| if (n == 0) { | ||
| return 1; | ||
| } | ||
| if (x == 0 || x == 1) { |
There was a problem hiding this comment.
| if (x == 0 || x == 1) { | |
| if (x == 0 || x == 1 || n == 1) { |
| if (x == -1) { | ||
| return (n & 0b1) == 0 ? 1 : -1; | ||
| } | ||
|
|
There was a problem hiding this comment.
| if (x == 2) { | |
| if (n >= Integer.SIZE - 1) | |
| throw new ArithmeticException("integer overflow"); | |
| return 1 << n; | |
| } | |
| if (x == -2) { | |
| if (n >= Integer.SIZE) | |
| throw new ArithmeticException("integer overflow"); | |
| // if n == Integer.SIZE - 1, result is correct | |
| return (n & 0b1) == 0 ? 1 << n : -(1 << n); | |
| } | |
| if ((java.math.BigInteger.bitLengthForInt(Math.abs(x)) - 1L) * n + 1L > Integer.SIZE) { | |
| throw new ArithmeticException("integer overflow"); | |
| } |
With also a check for the condition java.math.BigInteger.bitLengthForInt(Math.abs(x)) * n < Integer.SIZE, when it is true the path could be led to a loop that skips the checks.
There was a problem hiding this comment.
return (n & 0b1) == 0 ? 1 << n : -(1 << n);Equivalent to
return ((1 << n) ^ -(n & 1)) + (n & 1);Without branches it should be faster
There was a problem hiding this comment.
return ((1 << n) ^ -(n & 1)) + (n & 1);
It should have a comment that explains that this does the two's complement if n is odd, and it does nothing otherwise. Anyway, probably the optimization for x == -2 will not be included.
| if (n == 0) { | ||
| return 1; | ||
| } | ||
| if (x == 0 || x == 1) { |
There was a problem hiding this comment.
| if (x == 0 || x == 1) { | |
| if (x == 0 || x == 1 || n == 1) { |
| if (x == 0 || x == 1) { | ||
| return x; | ||
| } | ||
|
|
There was a problem hiding this comment.
| if (x == 2) { | |
| if (n >= Integer.SIZE) | |
| throw new ArithmeticException("unsigned integer overflow"); | |
| return 1 << n; | |
| } | |
| if ((java.math.BigInteger.bitLengthForInt(x) - 1L) * n + 1L > Integer.SIZE) { | |
| throw new ArithmeticException("unsigned integer overflow"); | |
| } |
With also a check for the condition java.math.BigInteger.bitLengthForInt(x) * n <= Integer.SIZE, when it is true the path could be led to a loop that skips the checks.
| if (x == -1) { | ||
| return (n & 0b1) != 0 ? -1 : 1; | ||
| } | ||
|
|
There was a problem hiding this comment.
| if (x == 2) { | |
| if (n >= Long.SIZE - 1) | |
| throw new ArithmeticException("long overflow"); | |
| return 1L << n; | |
| } | |
| if (x == -2) { | |
| if (n >= Long.SIZE) | |
| throw new ArithmeticException("long overflow"); | |
| // if n == Long.SIZE - 1, result is correct | |
| return (n & 0b1) == 0 ? 1L << n : -(1L << n); | |
| } | |
| if ((java.math.BigInteger.bitLengthForLong(Math.abs(x)) - 1L) * n + 1L > Long.SIZE) { | |
| throw new ArithmeticException("long overflow"); | |
| } |
With also a check for the condition java.math.BigInteger.bitLengthForLong(Math.abs(x)) * n < Long.SIZE, when it is true the path could be led to a loop that skips the checks.
| if (x == 0 || x == 1) { | ||
| return x; | ||
| } | ||
|
|
There was a problem hiding this comment.
| if (x == 2) { | |
| if (n >= Long.SIZE) | |
| throw new ArithmeticException("unsigned long overflow"); | |
| return 1L << n; | |
| } | |
| if ((java.math.BigInteger.bitLengthForLong(x) - 1L) * n + 1L > Long.SIZE) { | |
| throw new ArithmeticException("unsigned long overflow"); | |
| } |
With also a check for the condition java.math.BigInteger.bitLengthForLong(x) * n <= Long.SIZE, when it is true the path could be led to a loop that skips the checks.
There was a problem hiding this comment.
I don't think there's a quick, precise pre-check that would ensure that the loop can just use simple, unchecked * multiplications.
Consider unsignedPowExact(3L, 40), which does not overflow, versus unsignedPowExact(3L, 41), which does.
How would you pre-check these two cases using integer arithmetic?
IMO, you still need checked multiplications in the loop.
(Besides, the product in your checks can overflow, so you would have to add a guard.)
There was a problem hiding this comment.
I don't think there's a quick, precise pre-check that would ensure that the loop can just use simple, unchecked
*multiplications.Consider
unsignedPowExact(3L, 40), which does not overflow, versusunsignedPowExact(3L, 41), which does. How would you pre-check these two cases using integer arithmetic?IMO, you still need checked multiplications in the loop.
(Besides, the product in your checks can overflow, so you would have to add a guard.)
-
BigInteger.bitLengthForLong(x) * n <= Long.SIZEis a sufficient condition to ensure no overflow; -
(BigInteger.bitLengthForLong(x) - 1L) * n + 1L > Long.SIZEis a sufficient condition to ensure the overflow.
Thus, there remain only the cases when Long.SIZE < BigInteger.bitLengthForLong(x) * n && (BigInteger.bitLengthForLong(x) - 1L) * n + 1L <= Long.SIZE, in this cases checked multiplications in the loop are needed.
Moreover, BigInteger.bitLengthForLong(x) - 1L is a long, so the product does not overflow, so the condition at point 1 never overflows if it is evaluated only if the condition at point 2 is false.
There was a problem hiding this comment.
@fabioromano1 Well, there are two checks. In one the product can overflow, you'd need to convert one of the operands to long.
Anyway, since the pre-checks are not precise, that would lead to an implementation with a loop with checked, and another one with unchecked multiplications. I don't think this buys you anything.
There was a problem hiding this comment.
Anyway, since the pre-checks are not precise, that would lead to an implementation with a loop with checked, and another one with unchecked multiplications. I don't think this buys you anything.
It serves to skip the checks in the loop if in the common cases the length of the results are way more little with respect to Long.SIZE and to fail fast if in the common cases the length of the results are way bigger than Long.SIZE.
@fabioromano1 Well, there are two checks. In one the product can overflow, you'd need to convert one of the operands to
long.
If the condition at point 1 is evaluated only if the condition at point 2 is false, then it can never overflow.
There was a problem hiding this comment.
If that check would be a couple of instructions or so, then I could agree.
True, there are no overflows in the checks.
There was a problem hiding this comment.
@rgiulietti If you think that these checks might be useful, the choice is yours.
| if (n == 0) { | ||
| return 1; | ||
| } | ||
| if (x == 0 || x == 1) { |
There was a problem hiding this comment.
| if (x == 0 || x == 1) { | |
| if (x == 0 || x == 1 || n == 1) { |
There was a problem hiding this comment.
@fabioromano1 Unless there's evidence that these cases are very very common, there's no point in adding fast paths.
See this comment in unsignedPowExact(long,int)
/*
* To keep the code as simple as possible, there are intentionally
* no fast paths, except for |x| <= 1.
* The reason is that the number of loop iterations below can be kept
* very small when |x| > 1, but not necessarily when |x| <= 1.
*/
There was a problem hiding this comment.
@rgiulietti I would keep at least n == 1 and (bitLength(x) - 1L) * n + 1L > SIZE cases
There was a problem hiding this comment.
Again, I don't think that n == 1 is a frequent case which would make any practical difference.
As for the bitLength check, the product might overflow.
Further, bitLength might not be that cheap.
Finally, the test would just help to fail faster at the expense of making the successful runs slightly slower.
There was a problem hiding this comment.
As for the
bitLengthcheck, the product might overflow. Further,bitLengthmight not be that cheap.
The current implementations of bitLength() call numberOfLeadingZeros(), which are:
public static int numberOfLeadingZeros(long i) {
int x = (int)(i >>> 32);
return x == 0 ? 32 + Integer.numberOfLeadingZeros((int)i)
: Integer.numberOfLeadingZeros(x);
}
public static int numberOfLeadingZeros(int i) {
// HD, Count leading 0's
if (i <= 0)
return i == 0 ? 32 : 0;
int n = 31;
if (i >= 1 << 16) { n -= 16; i >>>= 16; }
if (i >= 1 << 8) { n -= 8; i >>>= 8; }
if (i >= 1 << 4) { n -= 4; i >>>= 4; }
if (i >= 1 << 2) { n -= 2; i >>>= 2; }
return n - (i >>> 1);
}There was a problem hiding this comment.
Yes, I'm familiar with both this Java code and the intrinsic code.
Compare this with the much simpler proposed code.
The checked multiplication unsignedMultiplyExact apparently performs two 64x64->64 multiplications, but on some architectures it might end up in a single 64x64->128 multiplication and one check.
So the proposed code performs 6 such multiplications if the method returns + 5 ordinary multiplications in the worst case.
As a general rule, the simpler the code, the better the outcome of the optimizing compiler.
Again, to me there's no point in failing fast at the expense of the successful case.
There was a problem hiding this comment.
Yes, I'm familiar with both this Java code and the intrinsic code.
Compare this with the much simpler proposed code. The checked multiplication
unsignedMultiplyExactapparently performs two 64x64->64 multiplications, but on some architectures it might end up in a single 64x64->128 multiplication and one check. So the proposed code performs 6 such multiplications if the method returns + 5 ordinary multiplications in the worst case.As a general rule, the simpler the code, the better the outcome of the optimizing compiler.
Again, to me there's no point in failing fast at the expense of the successful case.
Yes; we can always try to make simpler code faster if the need or interest arises.
Add
powExact()andunsignedPowExact()methods that operate on integer values arguments.Further, add
unsignedMultiplyExactmethods as well.Progress
Issues
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25003/head:pull/25003$ git checkout pull/25003Update a local copy of the PR:
$ git checkout pull/25003$ git pull https://git.openjdk.org/jdk.git pull/25003/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25003View PR using the GUI difftool:
$ git pr show -t 25003Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25003.diff
Using Webrev
Link to Webrev Comment