Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Mon, 15 Jun 2026 12:38:15 GMT, Andrew Dinn wrote: >>> Should these new stubs be AOT-saved/restored? See how other stubs are doing >>> `load_archive_data` / `save_archive_data` dance. >> >> Yes, thanks. Done. >> >>> A few drive-by comments. I have not dug into the core of the implementation. >>> >>> How would you test this for correctness? >> >> We already have tests. E.g. " >> make test "jtreg:open/test/jdk/sun/security/ec/ECDSAPrimitive.java" >> tests correctness. >> >> @adinn thanks a lot for the thorough review. Unfortunately, from May 26th to >> June 10th I will not be able to work on this, so, please take that into >> consideration with the re-review. > > @ferakocz A few more comments, suggestions and a question. Nearly there. > I think I have addressed every review comment, @adinn, if you are satisfied > with the changes, please approve (and sponsor, please) Have you pushed your most recent changes? Last one I am seeing is from 17 Jun 2026 09:57:05 GMT. - PR Comment: https://git.openjdk.org/jdk/pull/30941#issuecomment-4741773054
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Thu, 18 Jun 2026 07:57:05 GMT, Andrew Haley wrote: >> Hmm, interesting result. Yes, please stick with the current version. But >> also update the comment to note that this lsl;lsr;orr sequence out-performs >> extract. > > ... on one particular implementation. Let's not forget that we're writing for > the architecture. OK, I leave it as is and add the comment. - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3434888143
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Thu, 18 Jun 2026 09:15:50 GMT, Andrew Dinn wrote: >> Perhaps the `Curve25519OrderField` and `Curve448OrderField` can be >> refactored away from `IntegerPolynomialModBinP`, but I would not suggest >> that for this PR. >> >> I'm ok with changing the constructor to be private, but it's not accessible >> from the public API, so it's not a necessary change. > > Ok, so if the only clients can be in this package then I guess that it is > very unlikely anyone is ever going pass in an `IntegerPolynomialModBinP` > instance with zero limbs. > @ferakocz could you add a comment above the declaration of ctr stating that > the loop assumes the limb count in ctr is positive. Added the comment. - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3435336901
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Wed, 17 Jun 2026 23:26:56 GMT, Anthony Scarpino wrote: >> 0 as number of limbs doesn't make sense as no number can be represented that >> way. I think the IntegerPolynomialModBinP class was created for testing >> purposes mainly and only the two inner classes should subclass it, i.e. the >> constructor should rather be private and in that case the 0 number of limbs >> would not be a concern. @ascarpino, do you remember what this class was made >> for and whether it is obsolete now? > > Perhaps the `Curve25519OrderField` and `Curve448OrderField` can be refactored > away from `IntegerPolynomialModBinP`, but I would not suggest that for this > PR. > > I'm ok with changing the constructor to be private, but it's not accessible > from the public API, so it's not a necessary change. Ok, so if the only clients can be in this package then I guess that it is very unlikely anyone is ever going pass in an `IntegerPolynomialModBinP` instance with zero limbs. @ferakocz could you add a comment above the declaration of ctr stating that the loop assumes the limb count in ctr is positive. - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3434590275
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Wed, 17 Jun 2026 19:58:03 GMT, Andrew Dinn wrote: >> @adinn : Well, I have changed it, but that caused a small, but consistent >> performance drop (~0.7%) in the benchmark on the M1 chip that I measured it >> on. Since this PR is for performance, I am inclined to revert this change, >> if you agree. > > Hmm, interesting result. Yes, please stick with the current version. But also > update the comment to note that this lsl;lsr;orr sequence out-performs > extract. ... on one particular implementation. Let's not forget that we're writing for the architecture. - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3434112064
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Wed, 17 Jun 2026 19:08:57 GMT, Ferenc Rakoczi wrote:
>> Yes, the definition of Java class `IntegerPolynomial` does limit the
>> possible subclasses. However, as I mentioned in my earlier comment, that
>> list includes `IntegerPolynomialModBinP`
>>
>>
>> public abstract sealed class IntegerPolynomial implements IntegerFieldModuloP
>> permits IntegerPolynomial1305, IntegerPolynomial25519,
>> IntegerPolynomial448, IntegerPolynomialP256,
>> MontgomeryIntegerPolynomialP256, IntegerPolynomialP384,
>> IntegerPolynomialP521, IntegerPolynomialModBinP, P256OrderField,
>> P384OrderField, P521OrderField, Curve25519OrderField,
>> Curve448OrderField {
>>
>>
>> This specific subclass employs a limb count specified by a caller in the
>> constructor
>>
>>
>> public sealed class IntegerPolynomialModBinP extends IntegerPolynomial {
>> ...
>> public IntegerPolynomialModBinP(int bitsPerLimb,
>> int numLimbs,
>> int power,
>> BigInteger subtrahend) {
>> super(bitsPerLimb, numLimbs, 1,
>> BigInteger.valueOf(2).pow(power).subtract(subtrahend));
>>
>>
>> So, for this class it looks as if a user could specify numLimbs == 0 when
>> creating an `IntegerPolynomialModBinP` and that same count will be passed on
>> up to `IntegerPolynomial`.
>>
>> Now, that doesn't guarantee that any subsequent call to
>> `IntegerPolynomial::mult` will end up being passed a zero length array but
>> I'd like to be reassured why this won't happen.
>
> 0 as number of limbs doesn't make sense as no number can be represented that
> way. I think the IntegerPolynomialModBinP class was created for testing
> purposes mainly and only the two inner classes should subclass it, i.e. the
> constructor should rather be private and in that case the 0 number of limbs
> would not be a concern. @ascarpino, do you remember what this class was made
> for and whether it is obsolete now?
Perhaps the `Curve25519OrderField` and `Curve448OrderField` can be refactored
away from `IntegerPolynomialModBinP`, but I would not suggest that for this PR.
I'm ok with changing the constructor to be private, but it's not accessible
from the public API, so it's not a necessary change.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3432048761
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Wed, 17 Jun 2026 18:58:05 GMT, Ferenc Rakoczi wrote: >> @ferakocz you could use the `extr` instruction to do what you want here i.e. >> >> // combine 40 + 12 bits into hi result >> __ lsl(hi, hi, montMulP256Shift1); >> __ lsr(tmp, lo, montMulP256Shift2); >> __ orr(hi, hi, tmp); >> // mask off 52 bits of lo result >> __ andr(lo, lo, mask); >> >> can be replaced with >> >> // combine 40 + 12 bits into hi result >> __ extr(hi, hi, low, montMulP256Shift2); >> // mask off 52 bits of lo result >> __ andr(lo, lo, mask); >> >> >> That has the advantage of not requiring you to use `tmp`. > > @adinn : Well, I have changed it, but that caused a small, but consistent > performance drop (~0.7%) in the benchmark on the M1 chip that I measured it > on. Since this PR is for performance, I am inclined to revert this change, if > you agree. Hmm, interesting result. Yes, please stick with the current version. But also update the comment to note that this lsl;lsr;orr sequence out-performs extract. - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3431070038
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Wed, 17 Jun 2026 11:33:20 GMT, Andrew Dinn wrote:
>> It is true that IntegerPolynomial::conditionalAssign() is not only for the
>> IntegerPolynomialP256 subclass, but it has a fixed set of permitted
>> subclasses, with the number of limbs in the set {5, 10, 14, 16, 19} as one
>> can infer from the comment at the beginning of this function. So the 0-check
>> is not necessary.
>
> Yes, the definition of Java class `IntegerPolynomial` does limit the possible
> subclasses. However, as I mentioned in my earlier comment, that list includes
> `IntegerPolynomialModBinP`
>
>
> public abstract sealed class IntegerPolynomial implements IntegerFieldModuloP
> permits IntegerPolynomial1305, IntegerPolynomial25519,
> IntegerPolynomial448, IntegerPolynomialP256,
> MontgomeryIntegerPolynomialP256, IntegerPolynomialP384,
> IntegerPolynomialP521, IntegerPolynomialModBinP, P256OrderField,
> P384OrderField, P521OrderField, Curve25519OrderField,
> Curve448OrderField {
>
>
> This specific subclass employs a limb count specified by a caller in the
> constructor
>
>
> public sealed class IntegerPolynomialModBinP extends IntegerPolynomial {
> ...
> public IntegerPolynomialModBinP(int bitsPerLimb,
> int numLimbs,
> int power,
> BigInteger subtrahend) {
> super(bitsPerLimb, numLimbs, 1,
> BigInteger.valueOf(2).pow(power).subtract(subtrahend));
>
>
> So, for this class it looks as if a user could specify numLimbs == 0 when
> creating an `IntegerPolynomialModBinP` and that same count will be passed on
> up to `IntegerPolynomial`.
>
> Now, that doesn't guarantee that any subsequent call to
> `IntegerPolynomial::mult` will end up being passed a zero length array but
> I'd like to be reassured why this won't happen.
0 as number of limbs doesn't make sense as no number can be represented that
way. I think the IntegerPolynomialModBinP class was created for testing
purposes mainly and only the two inner classes should subclass it, i.e. the
constructor should rather be private and in that case the 0 number of limbs
would not be a concern. @ascarpino, do you remember what this class was made
for and whether it is obsolete now?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3430793498
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Wed, 17 Jun 2026 11:53:57 GMT, Andrew Dinn wrote: >> This idea sounds great, but, unfortunately, in st2(v1, v2, T, post(base, >> 32)) it is required that the register index of v2 be one more than that of >> v1, so I just added a comment at the consuming part. > > Ah, yes. That's a shame as saving the data in canonical order would be much > better. We could salvage this by redeclaring the sequences so that adjacent > elements A[i] and D[i] are adjacent vector registers. > > VSeq<4> A(16, 2); > VSeq<4> D(17, 2); > VSeq<4> B(24); > VSeq<4> C(28); > > or equivalently > > VSeq<8> A_D(16); > VSeq<4> A = vs_even(A_D); > VSeq<4> D = vs_odd(A_D); > VSeq<4> B(24); > VSeq<4> C(28); > > but then a reader needs to work out why this is being done. > > I'll settle for your comment that restates the permuted layout just prior to > the ldrs. I was also considering the above, but thought that the comment was the more readable solution. - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3430753417
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Mon, 15 Jun 2026 14:10:14 GMT, Andrew Dinn wrote: >> Added the comments, but as for clarity of bfm, it is one less instruction, >> but to me it is not as intuitive as the shift and or. > > @ferakocz you could use the `extr` instruction to do what you want here i.e. > > // combine 40 + 12 bits into hi result > __ lsl(hi, hi, montMulP256Shift1); > __ lsr(tmp, lo, montMulP256Shift2); > __ orr(hi, hi, tmp); > // mask off 52 bits of lo result > __ andr(lo, lo, mask); > > can be replaced with > > // combine 40 + 12 bits into hi result > __ extr(hi, hi, low, montMulP256Shift2); > // mask off 52 bits of lo result > __ andr(lo, lo, mask); > > > That has the advantage of not requiring you to use `tmp`. @adinn : Well, I have changed it, but that caused a small, but consistent performance drop (~0.7%) in the benchmark on the M1 chip that I measured it on. Since this PR is for performance, I am inclined to revert this change, if you agree. - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3430734670
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Wed, 17 Jun 2026 09:27:19 GMT, Ferenc Rakoczi wrote:
>> Addendum:
>>
>> There is no need to include the static assert for even length in the
>> overloaded variant of `vs_st2_post` i.e. you can just use this:
>>
>>
>> // store 2 x N-vector sequences interleaved into 2 * N quadword
>> // memory locations via the address supplied in base using
>> // post-increment addressing.
>> template
>> void vs_st2_post(const VSeq& v1, const VSeq& v2,
>>Assembler::SIMD_Arrangement T, Register base) {
>> for (int i = 0; i < N; i++) {
>> __ st2(v1[i], v2[i], T, __ post(base, 32));
>> }
>> }```
>
> This idea sounds great, but, unfortunately, in st2(v1, v2, T, post(base, 32))
> it is required that the register index of v2 be one more than that of v1, so
> I just added a comment at the consuming part.
Ah, yes. That's a shame as saving the data in canonical order would be much
better. We could salvage this by redeclaring the sequences so that adjacent
elements A[i] and D[i] are adjacent vector registers.
VSeq<4> A(16, 2);
VSeq<4> D(17, 2);
VSeq<4> B(24);
VSeq<4> C(28);
or equivalently
VSeq<8> A_D(16);
VSeq<4> A = vs_even(A_D);
VSeq<4> D = vs_odd(A_D);
VSeq<4> B(24);
VSeq<4> C(28);
but then a reader needs to work out why this is being done.
I'll settle for your comment that restates the permuted layout just prior to
the ldrs.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3427880307
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Tue, 16 Jun 2026 21:06:36 GMT, Ferenc Rakoczi wrote:
>> I believe the method being intrinsified here is the one on the generic class
>> i.e. `IntegerPolynomial::conditionalAssign(int set, long[] a, long[] b)`.
>> So, in theory, it could be called from any subclass of `IntegerPolynomial`,
>> not just for `IntegerPolynomialP256`. Can you provide a more detailed
>> explanation of why it won't be called from other subtypes, including types
>> where the number of limbs may be 0?
>
> It is true that IntegerPolynomial::conditionalAssign() is not only for the
> IntegerPolynomialP256 subclass, but it has a fixed set of permitted
> subclasses, with the number of limbs in the set {5, 10, 14, 16, 19} as one
> can infer from the comment at the beginning of this function. So the 0-check
> is not necessary.
Yes, the definition of Java class `IntegerPolynomial` does limit the possible
subclasses. However, as I mentioned in my earlier comment, that list includes
`IntegerPolynomialModBinP`
public abstract sealed class IntegerPolynomial implements IntegerFieldModuloP
permits IntegerPolynomial1305, IntegerPolynomial25519,
IntegerPolynomial448, IntegerPolynomialP256,
MontgomeryIntegerPolynomialP256, IntegerPolynomialP384,
IntegerPolynomialP521, IntegerPolynomialModBinP, P256OrderField,
P384OrderField, P521OrderField, Curve25519OrderField,
Curve448OrderField {
This specific subclass employs a limb count specified by a caller in the
constructor
public sealed class IntegerPolynomialModBinP extends IntegerPolynomial {
...
public IntegerPolynomialModBinP(int bitsPerLimb,
int numLimbs,
int power,
BigInteger subtrahend) {
super(bitsPerLimb, numLimbs, 1,
BigInteger.valueOf(2).pow(power).subtract(subtrahend));
So, for this class it looks as if a user could specify numLimbs == 0 when
creating an `IntegerPolynomialModBinP` and that same count will be passed on up
to `IntegerPolynomial`.
Now, that doesn't guarantee that any subsequent call to
`IntegerPolynomial::mult` will end up being passed a zero length array but I'd
like to be reassured why this won't happen.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3427760518
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Mon, 15 Jun 2026 12:36:27 GMT, Andrew Dinn wrote:
>> This intrinsic is for such a subtype that has a limb count of 5, so 0 is not
>> possible here.
>
> I believe the method being intrinsified here is the one on the generic class
> i.e. `IntegerPolynomial::conditionalAssign(int set, long[] a, long[] b)`. So,
> in theory, it could be called from any subclass of `IntegerPolynomial`, not
> just for `IntegerPolynomialP256`. Can you provide a more detailed explanation
> of why it won't be called from other subtypes, including types where the
> number of limbs may be 0?
It is true that IntegerPolynomial::conditionalAssign() is not only for the
IntegerPolynomialP256 subclass, but it has a fixed set of permitted subclasses,
with the number of limbs in the set {5, 10, 14, 16, 19} as one can infer from
the comment at the beginning of this function. So the 0-check is not necessary.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3424035171
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Mon, 15 Jun 2026 12:25:20 GMT, Andrew Dinn wrote: >> Accepted. > > But not yet changed? Changed at many places and now here, too. - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3424166869
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Mon, 15 Jun 2026 12:24:38 GMT, Andrew Dinn wrote: >> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8009: >> >>> 8007: // a[3] >>> 8008: // >>> 8009: >> >> Suggestion: >> >> >> // For a_3 and a_4 we have already computed the cross-products >> // with b_0 ... b_3 and stored them on the stack relative to >> // `mul_ptr` i.e. the current `sp`. > > is there a reason not to include this comment? Unintentionally missed it, sorry :-( . - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3422998044
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Mon, 15 Jun 2026 12:44:43 GMT, Andrew Dinn wrote:
>> I still have a problem with this.
>>
>> With your definition of `vs_st1_interleaved` we end up with this weird
>> organization of the multiplication products where we have two low pairs and
>> then their corresponding high pairs repeated x 4. There is nothing in the
>> code that explains this layout other than the comment here at the point of
>> write (n.b. my 'over-engineered' suggestion was actually a way of making it
>> explicit what was going on here).
>>
>> That results in some rather unobvious indexes for the loads we see later
>> which consume these x-products:
>>
>>
>> __ ldr(low_1, Address(sp));
>> __ ldr(high_1, Address(sp, 2 * BytesPerLong));
>> __ ldr(low, Address(sp, BytesPerLong));
>> __ ldr(high, Address(sp, 3 * BytesPerLong));
>> . . .
>> __ ldr(low_1, Address(sp, 4 * BytesPerLong));
>> __ ldr(high_1, Address(sp, 6 * BytesPerLong));
>> . . .
>> __ ldr(low, Address(sp, 5 * BytesPerLong));
>> __ ldr(high, Address(sp, 7 * BytesPerLong));
>> . . .
>>
>>
>> Any maintainer looking at these `ldr` instructions (including me in about 6
>> months time) will end up having to search back through the preceding code to
>> connect the offsets with the comment at the point of call to
>> `vs_st1_interleaved`. We can avoid that redundant effort either with
>> comments or clearer code. So, the choice is:
>>
>> 1. Comment each ldr to point back to the explanation of the saved
>> cross-product data layout provided at the call site.
>> 2. Change the write method to store the 64-bit elements interleaved in the
>> more obvious order `(lo0, hi0, lo1, hi1, ...)`.
>>
>> The second option seems far preferable to me.
>>
>> We need to drop `vs_st1_interleaved` and replace it with he following
>> overloaded variant of `vs_st2_post`:
>>
>> // store 2 x N-vector sequences interleaved into 2 * N quadword
>> // memory locations via the address supplied in base using
>> // post-increment addressing.
>> template
>> void vs_st2_post(const VSeq& v1, const VSeq& v2,
>>Assembler::SIMD_Arrangement T, Register base) {
>> static_assert((N & (N - 1)) == 0, "sequence length must be even");
>> for (int i = 0; i < N; i++) {
>> __ st2(v1[i], v2[i], T, __ post(base, 32));
>> }
>> }
>>
>>
>> Then change the current call and comment:
>>
>>
>> // Interleaves the 8 low products in A (l0, ..., l7) and 8
>> // high products in D (h1, ..., h7) into 8 quadwords on the
>> // stack at mulptr: ((l0, h0), ..., (l7, h7))
>> v2_st2_post(A, D, __ T2D, ...
>
> Addendum:
>
> There is no need to include the static assert for even length in the
> overloaded variant of `vs_st2_post` i.e. you can just use this:
>
>
> // store 2 x N-vector sequences interleaved into 2 * N quadword
> // memory locations via the address supplied in base using
> // post-increment addressing.
> template
> void vs_st2_post(const VSeq& v1, const VSeq& v2,
>Assembler::SIMD_Arrangement T, Register base) {
> for (int i = 0; i < N; i++) {
> __ st2(v1[i], v2[i], T, __ post(base, 32));
> }
> }```
This idea sounds great, but, unfortunately, in st2(v1, v2, T, post(base, 32))
it is required that the register index of v2 be one more than that of v1, so I
just added a comment at the consuming part.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3427018182
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Fri, 22 May 2026 13:06:15 GMT, Ferenc Rakoczi wrote: >> There is certainly the possibility of this happening. Most subtypes employ a >> hard-wired non-zero limb count but the constructor for >> `IntegerPolynomialModBinP` accepts a limb count argument which could >> certainly be passed as zero -- with zero length arrays then being employed >> for the limb data (obviously a negative count won't cut it). I'm not clear >> whether that would trip over other assumptions before a mult call could >> occur but it costs little to defend against the possibility that it might >> not so probably best to be safe? > > This intrinsic is for such a subtype that has a limb count of 5, so 0 is not > possible here. I believe the method being intrinsified here is the one on the generic class i.e. `IntegerPolynomial::conditionalAssign(int set, long[] a, long[] b)`. So, in theory, it could be called from any subclass of `IntegerPolynomial`, not just for `IntegerPolynomialP256`. Can you provide a more detailed explanation of why it won't be called from other subtypes, including types where the number of limbs may be 0? - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3413473103
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Mon, 15 Jun 2026 12:24:08 GMT, Andrew Dinn wrote:
>> Yes. Accepted
>
> I still have a problem with this.
>
> With your definition of `vs_st1_interleaved` we end up with this weird
> organization of the multiplication products where we have two low pairs and
> then their corresponding high pairs repeated x 4. There is nothing in the
> code that explains this layout other than the comment here at the point of
> write (n.b. my 'over-engineered' suggestion was actually a way of making it
> explicit what was going on here).
>
> That results in some rather unobvious indexes for the loads we see later
> which consume these x-products:
>
>
> __ ldr(low_1, Address(sp));
> __ ldr(high_1, Address(sp, 2 * BytesPerLong));
> __ ldr(low, Address(sp, BytesPerLong));
> __ ldr(high, Address(sp, 3 * BytesPerLong));
> . . .
> __ ldr(low_1, Address(sp, 4 * BytesPerLong));
> __ ldr(high_1, Address(sp, 6 * BytesPerLong));
> . . .
> __ ldr(low, Address(sp, 5 * BytesPerLong));
> __ ldr(high, Address(sp, 7 * BytesPerLong));
> . . .
>
>
> Any maintainer looking at these `ldr` instructions (including me in about 6
> months time) will end up having to search back through the preceding code to
> connect the offsets with the comment at the point of call to
> `vs_st1_interleaved`. We can avoid that redundant effort either with comments
> or clearer code can avoid it. So, the choice is:
>
> 1. Comment each ldr to point back to the explanation of the saved
> cross-product data layout provided at the call site.
> 2. Change the write method to store the 64-bit elements interleaved in the
> more obvious order `(lo0, hi0, lo1, hi1, ...)`.
>
> The second option seems far preferable to me.
>
> We need to drop `vs_st1_interleaved` and replace it with he following
> overloaded variant of `vs_st2_post`:
>
> // store 2 x N-vector sequences interleaved into 2 * N quadword
> // memory locations via the address supplied in base using
> // post-increment addressing.
> template
> void vs_st2_post(const VSeq& v1, const VSeq& v2,
>Assembler::SIMD_Arrangement T, Register base) {
> static_assert((N & (N - 1)) == 0, "sequence length must be even");
> for (int i = 0; i < N; i++) {
> __ st2(v1[i], v2[i], T, __ post(base, 32));
> }
> }
>
>
> Then change the current call and comment:
>
>
> // Interleaves the 8 low products in A (l0, ..., l7) and 8
> // high products in D (h1, ..., h7) into 8 quadwords on the
> // stack at mulptr: ((l0, h0), ..., (l7, h7))
> v2_st2_post(A, D, __ T2D, mul_ptr);
Addendum:
There is no need to include the static assert for even length in the overloaded
variant of `vs_st2_post` i.e. you can just use this:
// store 2 x N-vector sequences interleaved into 2 * N quadword
// memory locations via the address supplied in base using
// post-increment addressing.
template
void vs_st2_post(const VSeq& v1, const VSeq& v2,
Assembler::SIMD_Arrangement T, Register base) {
for (int i = 0; i < N; i++) {
__ st2(v1[i], v2[i], T, __ post(base, 32));
}
}```
-
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3413524508
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Fri, 22 May 2026 13:04:06 GMT, Ferenc Rakoczi wrote: >> It might be better and clearer to use `bfm` rather that shifting, masking, >> and ORing. > > Added the comments, but as for clarity of bfm, it is one less instruction, > but to me it is not as intuitive as the shift and or. @ferakocz you could use the `extr` instruction to do what you want here i.e. // combine 40 + 12 bits into hi result __ lsl(hi, hi, montMulP256Shift1); __ lsr(tmp, lo, montMulP256Shift2); __ orr(hi, hi, tmp); // mask off 52 bits of lo result __ andr(lo, lo, mask); can be replaced with // combine 40 + 12 bits into hi result __ extr(hi, hi, low, montMulP256Shift2); // mask off 52 bits of lo result __ andr(lo, lo, mask); That has the advantage of not requiring you to use `tmp`. - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3414104096
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Fri, 22 May 2026 13:09:56 GMT, Ferenc Rakoczi wrote: >> Should these new stubs be AOT-saved/restored? See how other stubs are doing >> `load_archive_data` / `save_archive_data` dance. > >> Should these new stubs be AOT-saved/restored? See how other stubs are doing >> `load_archive_data` / `save_archive_data` dance. > > Yes, thanks. Done. > >> A few drive-by comments. I have not dug into the core of the implementation. >> >> How would you test this for correctness? > > We already have tests. E.g. " > make test "jtreg:open/test/jdk/sun/security/ec/ECDSAPrimitive.java" > tests correctness. > > @adinn thanks a lot for the thorough review. Unfortunately, from May 26th to > June 10th I will not be able to work on this, so, please take that into > consideration with the re-review. @ferakocz A few more comments, suggestions and a question. Nearly there. - PR Comment: https://git.openjdk.org/jdk/pull/30941#issuecomment-4708000134
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Tue, 19 May 2026 16:31:00 GMT, Andrew Dinn wrote: >> Ferenc Rakoczi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Accepting more suggestions from Andrew Dinn. > > src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8009: > >> 8007: // a[3] >> 8008: // >> 8009: > > Suggestion: > > > // For a_3 and a_4 we have already computed the cross-products > // with b_0 ... b_3 and stored them on the stack relative to > // `mul_ptr` i.e. the current `sp`. is there a reason not to include this comment? - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3413403924
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Fri, 22 May 2026 13:00:58 GMT, Ferenc Rakoczi wrote:
>> Actually, I see now that my comment was not correct. The write interleaves 4
>> successive pairs of low values with four pairs of successive high values
>> i.e. (low0, low1) (high0, high1) . . .
>> Suggestion:
>>
>> // the write interleaves the 4 successive pairs of low and
>> // high results: (l0, l1), (h0, h1), ... (l6, l7), (h6, h7)
>> vs_st1_interleaved(A, D, mul_ptr);
>
> Yes. Accepted
I still have a problem with this.
With your definition of `vs_st1_interleaved` we end up with this weird
organization of the multiplication products where we have two low pairs and
then their corresponding high pairs repeated x 4. There is nothing in the code
that explains this layout other than the comment here at the point of write
(n.b. my 'over-engineered' suggestion was actually a way of making it explicit
what was going on here).
That results in some rather unobvious indexes for the loads we see later which
consume these x-products:
__ ldr(low_1, Address(sp));
__ ldr(high_1, Address(sp, 2 * BytesPerLong));
__ ldr(low, Address(sp, BytesPerLong));
__ ldr(high, Address(sp, 3 * BytesPerLong));
. . .
__ ldr(low_1, Address(sp, 4 * BytesPerLong));
__ ldr(high_1, Address(sp, 6 * BytesPerLong));
. . .
__ ldr(low, Address(sp, 5 * BytesPerLong));
__ ldr(high, Address(sp, 7 * BytesPerLong));
. . .
Any maintainer looking at these `ldr` instructions (including me in about 6
months time) will end up having to search back through the preceding code to
connect the offsets with the comment at the point of call to
`vs_st1_interleaved`. We can avoid that redundant effort either with comments
or clearer code can avoid it. So, the choice is:
1. Comment each ldr to point back to the explanation of the saved cross-product
data layout provided at the call site.
2. Change the write method to store the 64-bit elements interleaved in the more
obvious order `(lo0, hi0, lo1, hi1, ...)`.
The second option seems far preferable to me.
We need to drop `vs_st1_interleaved` and replace it with he following
overloaded variant of `vs_st2_post`:
// store 2 x N-vector sequences interleaved into 2 * N quadword
// memory locations via the address supplied in base using
// post-increment addressing.
template
void vs_st2_post(const VSeq& v1, const VSeq& v2,
Assembler::SIMD_Arrangement T, Register base) {
static_assert((N & (N - 1)) == 0, "sequence length must be even");
for (int i = 0; i < N; i++) {
__ st2(v1[i], v2[i], T, __ post(base, 32));
}
}
Then change the current call and comment:
// Interleaves the 8 low products in A (l0, ..., l7) and 8
// high products in D (h1, ..., h7) into 8 quadwords on the
// stack at mulptr: ((l0, h0), ..., (l7, h7))
v2_st2_post(A, D, __ T2D, mul_ptr);
-
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3413400993
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Fri, 22 May 2026 13:06:55 GMT, Ferenc Rakoczi wrote: >> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8377: >> >>> 8375: VSeq<4> b_vec(20); >>> 8376: >>> 8377: __ ldr(a9, Address(aLimbs, 64)); >> >> It makes things more obvious to a maintainer and avoids mistakes should >> anything need patching if you use 8 * BytesPerLong here instead of 64 and >> likewise for all the other hard-wired Address offsets used throughout this >> method. That way the offsets directly relate to the vector and GPR counts >> mentioned in the comments. > > Accepted. But not yet changed? - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3413407998
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Thu, 21 May 2026 08:59:02 GMT, Andrew Dinn wrote: >> Ferenc Rakoczi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Accepting more suggestions from Andrew Dinn. > > @ferakocz I have done a full pass of the PR and made all the recommendations > I have to offer for now. Please address them plus the comments made by > @shipilev and @theRealAph and then I will do a second pass. > @adinn thanks a lot for the thorough review. Unfortunately, from May 26th to > June 10th I will not be able to work on this, so, please take that into > consideration with the re-review. @ferakocz I too will be away from 29th May back on 8th June. I'll do my best to respond to the latest update in the two days before you are back. - PR Comment: https://git.openjdk.org/jdk/pull/30941#issuecomment-4567028830
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Fri, 22 May 2026 13:04:06 GMT, Ferenc Rakoczi wrote: > Added the comments, but as for clarity of bfm, it is one less instruction, > but to me it is not as intuitive as the shift and or. That's a matter of getting used to it, I think. - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3292571437
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Mon, 18 May 2026 15:06:41 GMT, Andrew Dinn wrote:
>> Ferenc Rakoczi has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Accepting more suggestions from Andrew Dinn.
>
> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7869:
>
>> 7867: __ ldr(a_i, __ post(a, 8));
>> 7868: gpr_partial_mult_52(a_i, b_0, high, low, tmp, limb_mask);
>> 7869: __ andr(n, low, limb_mask);
>
> It would be clearer to use `mov(n, low)` here or, at the very least `orr(n,
> zr, low)`. `low` has already been masked with `limb_mask`, which makes the
> `andr` look redundant when, in fact, the point is to save the initial value
> computed for `low`.
Accepted.
> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7873:
>
>> 7871: neon_partial_mult_64(B, b_highs, a_vals, 0);
>> 7872:
>> 7873: // Limb 0 cont
>
> Suggestion:
>
> // Limb 0 modulus computation
> // n.b. modulus computation requires multiplying successive
> // limbs of the product by corresponding limbs of the p256
> // prime adding the result to the limb and folding this
> // partial result into a running 256-bit sum in c_i. Limbs
> // of c_i are stored via c_ptr once carries are included.
> // n.b. the mul + add is omitted for limb 2 since the
> // corresponding prime bits are zero.
Accepted.
> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7899:
>
>> 7897: gpr_partial_mult_52(a_i, b_2, high, low, tmp, limb_mask);
>> 7898: __ add(c_i, c_i, low);
>> 7899: __ str(c_i, Address(c_ptr, 8));
>
> I don't really like the hard-coded numbers in these `c_ptr` address
> expressions. At the very least it would be better if they were be specified
> using a const expression like `nnn * BytesPerLong` to show that we are
> indexing an array of 64 bit data at index `nnn`.
>
> A better idea would be to define a static helper method that also checks the
> index is in range
>
>
> #define C_DATA_COUNT 5
> static int c_slot_offset(index i) {
> assert(0 <= i && i < C_SLOT_COUNT, "invalid index for c_i data");
> return i * BytesPerLong;
> }
>
>
> You can then define the address as `Address(c_ptr, c_offset(1))` etc.
>
> Also, once you have defined `C_DATA_COUNT` you can also define a constant
> C_DATA_SIZE for use when you extend the stack:
>
>
> #define C_DATA_SIZE align_up(C_DATA_COUNT, 2) * BytesPerLong;
>
> . . .
> __ sub(sp, sp, C_DATA_SIZE);
> . . .
>
>
> The same consideration applies with even more force as regards the hard-wired
> offsets used when accessing the Neon multiplication product data via `sp`.
>
>
> __ ldr(low_1, Address(sp));
> __ ldr(high_1, Address(sp, 16));
>
>
> This also allows you to fold the details of the data layout into the function:
>
>
> #define MUL_DATA_COUNT 8
> static int mul_data_offset(index i, boolean want_high) {
> assert(0 <= i && i < MUL_DATA_COUNT, "invalid index for mul data");
> // data is stored as successive low/high pairs
> // (l0, l1), (h0, h1), ... (l6, l7), (h6, h7)
> int pair_start = (i >> 1) * 4;
> int high_select = (want_high ? 2 : 0)
> int pair_element = (i & 1);
> int idx = pair_start + high_select + pair_element;
> return (idx * BytesPerLong);
> }
>
> . . .
> __ ldr(low_1, Address(sp, mul_data_offset(0, false));
> __ ldr(high_1, Address(sp, mul_data_offset(0, true)));
> . . .
>
>
> Likewise you can define a constant MUL_DATA_SIZE for use when you extend the
> stack:
>
>
> #define MUL_DATA_SIZE (MUL_DATA_COUNT * 2) * BytesPerLong;
>
> . . .
> __ sub(sp, sp, MUL_DATA_SIZE);
> . . .
I think this would be over-engineering. We are talking about fixed length
arrays here, k * BytesPerLong clearly enough indicates the index k in my
opinion (even with "precomputing" for indicies 0 and 1).
> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8077:
>
>> 8075: + b_0 + b_1 + b_2;
>> 8076: b_0 = b_1 = b_2 = noreg;
>> 8077:
>
> This freeing of registers would be better done at line 8002 (i.e. before
> processing a3) and with register b_3 also freed. It highlights to the
> maintainer that b0 - b3 are no longer needed from that point on.
Done.
> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8267:
>
>> 8265: // P521OrderField: 19 = 8 + 8 + 2 + 1
>> 8266: // Special Cases 5, 10, 14, 16, 19
>> 8267:
>
> You need to insert the standard call `load_archive_data` here and return any
> stub that is found.
Done.
> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8377:
>
>> 8375: VSeq<4> b_vec(20);
>> 8376:
>> 8377: __ ldr(a9, Address(aLimbs, 64));
>
> It makes things more obvious to a maintainer and avoids mistakes should
> anything need patching if you use 8 * BytesPerLong here instead of 64 and
> likewise for all the other hard-wired Address offsets used throughout this
> method. That way the offsets directly relate to the vector and
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Thu, 21 May 2026 08:51:14 GMT, Andrew Dinn wrote: >> I think x86 has an explicit test for `length == 0`: >> https://github.com/openjdk/jdk/blob/6fb07f9b26bcd23f622a50c6ca5f23363b432f85/src/hotspot/cpu/x86/stubGenerator_x86_64_poly_mont.cpp#L740-L742 > > There is certainly the possibility of this happening. Most subtypes employ a > hard-wired non-zero limb count but the constructor for > `IntegerPolynomialModBinP` accepts a limb count argument which could > certainly be passed as zero -- with zero length arrays then being employed > for the limb data (obviously a negative count won't cut it). I'm not clear > whether that would trip over other assumptions before a mult call could occur > but it costs little to defend against the possibility that it might not so > probably best to be safe? This intrinsic is for such a subtype that has a limb count of 5, so 0 is not possible here. - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3288553610
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Tue, 19 May 2026 08:27:53 GMT, Andrew Haley wrote: >> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7758: >> >>> 7756: __ lsr(tmp, lo, montMulP256Shift2); >>> 7757: __ orr(hi, hi, tmp); >>> 7758: __ andr(lo, lo, mask); >> >> Suggestion: >> >> // compute 104-bit (40 + 64) full product >> __ umulh(hi, a, b); >> __ mul(lo, a, b); >> // combine 40 + 12 bits into hi result >> __ lsl(hi, hi, montMulP256Shift1); >> __ lsr(tmp, lo, montMulP256Shift2); >> __ orr(hi, hi, tmp); >> // mask off 52 bits of lo result >> __ andr(lo, lo, mask); > > It might be better and clearer to use `bfm` rather that shifting, masking, > and ORing. Added the comments, but as for clarity of bfm, it is one less instruction, but to me it is not as intuitive as the shift and or. - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3288539803
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Wed, 20 May 2026 15:00:47 GMT, Andrew Dinn wrote:
>> src/hotspot/cpu/aarch64/register_aarch64.hpp line 546:
>>
>>> 544: template
>>> 545: VSeq vs_tail(const VSeq& v) {
>>> 546: static_assert(N > 1, "tail sequence length must be greater than 2");
>>
>> Which one is it, `N > 1`, or `greater than 2`?
>
> Full disclosure -- this is my copypasta introduced during an earlier round of
> reviewing.
>
> Given that we have a static assert that `N > 2` in the definition of class
> `VSeq` I guess this is redundant. Likewise, the assertion in `vs_head`.
>
> It doesn't really make any sense to /declare/ a `VSeq` with less than 2
> elements as the purpose of the class is to provide a way to manage and access
> more than one FloatRegister using indexed notation (`vs[i]`). However, the
> provision of `vs_tail` gives rise to the possibility that we might create a
> `VSeq<1>` from a `VSeq<2>` so we could consider adjusting the asserts
> accordingly.
>
> In fact, `vs_head` and `vs_tail` were added to deal with the need for the
> current proposed kernel to operate on groups of 5 registers at a time. The
> code uses `vs_head` to access the first register in the group and `vs_tail`
> to access a sequence over the remaining four registers (n.b. calling
> `vs_head(vs)` is not strictly needed -- you can just use `vs[0]` -- so maybe
> we don't need to provide it).
>
> So, for current requirements, we could just delete the assert here and in
> `vs_head` and rely on the one in the definition of `VSeq`. If we ever need to
> split off the tail of a `VSeq<2>` we could reconsider this.
>
> Likewise we could drop `vs_head`. I thought it would be good to provide it
> because it emphasise the way that the sequence is being recursively split in
> two and ... well ... Lisp!
Dropped the assert for now.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3288555091
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Thu, 21 May 2026 08:42:21 GMT, Aleksey Shipilev wrote: >> It doesn't. AArch64 assembler throws an assertion if any field of an >> instruction is not set. I think it's because `Vm` may have to be <15, so >> that bi twill be clear. But the comment is more confusing than helpful. > > Still confused on correctness here, bear with me for a sec. > > So current code means we set bit 20 implicitly to `0`, when `Ts == H`, > because it implies `Vm < 16`. But with `H` variant, don't we need to encode > lane bits as H:L:M explicitly? Meaning, `M` is the lower bit of the lane, > shouldn't it be dependent on `lane`, rather than being always `0`? > > Actually, a few lines above, when `size == 0b01`, we do not take the lower > bit out of `lane` at all? That's our `M`? I leave it like that for now. - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3288557404
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Wed, 20 May 2026 07:48:53 GMT, Aleksey Shipilev wrote:
> Should these new stubs be AOT-saved/restored? See how other stubs are doing
> `load_archive_data` / `save_archive_data` dance.
Yes, thanks. Done.
> A few drive-by comments. I have not dug into the core of the implementation.
>
> How would you test this for correctness?
We already have tests. E.g. "
make test "jtreg:open/test/jdk/sun/security/ec/ECDSAPrimitive.java"
tests correctness.
@adinn thanks a lot for the thorough review. Unfortunately, from May 26th to
June 10th I will not be able to work on this, so, please take that into
consideration with the re-review.
> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8018:
>
>> 8016: __ ldr(c_i, c_ptr);
>> 8017:
>> 8018: // Limb 1
>
> The numbering is a bit off here. There are duplicate `// Limb 2` comments.
> Should this one be `// Limb 0`?
Fixed.
> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8183:
>
>> 8181: tmp2 = *common_regs++,
>> 8182: tmp3 = *common_regs++,
>> 8183: tmp4 = *common_regs++;
>
> Cannot quite see where these `tmp*`-s are used?
Nowhere. Deleted them.
> src/hotspot/cpu/aarch64/vm_version_aarch64.cpp line 462:
>
>> 460: if (FLAG_IS_DEFAULT(UseIntPolyIntrinsics)) {
>> 461: UseIntPolyIntrinsics = true;
>> 462: }
>
> Indenting is a bit off here. Also, should this depend on `CPU_ASIMD`, like
> ChaCha20 intrinsic enablement a few blocks above?
It is Intellij's fault :-) . Fixed.
-
PR Comment: https://git.openjdk.org/jdk/pull/30941#issuecomment-4518944273
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3288554269
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3288555846
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3288552061
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Tue, 19 May 2026 13:24:01 GMT, Andrew Dinn wrote: >> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7939: >> >>> 7937: vs_addv(D, __ T2D, D, B); >>> 7938: >>> 7939: vs_st1_interleaved(A, D, mul_ptr); >> >> Suggestion: >> >> // interleaved write outputs 8 successive (low, high) 56-bit pairs >> vs_st1_interleaved(A, D, mul_ptr); > > Actually, I see now that my comment was not correct. The write interleaves 4 > successive pairs of low values with four pairs of successive high values i.e. > (low0, low1) (high0, high1) . . . > Suggestion: > > // the write interleaves the 4 successive pairs of low and > // high results: (l0, l1), (h0, h1), ... (l6, l7), (h6, h7) > vs_st1_interleaved(A, D, mul_ptr); Yes. Accepted - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3288519651
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Mon, 18 May 2026 09:10:20 GMT, Andrew Dinn wrote: >> Ferenc Rakoczi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Accepting more suggestions from Andrew Dinn. > > src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7863: > >> 7861: __ sub(sp, sp, 128); >> 7862: __ mov(mul_ptr, sp); >> 7863: > > Suggestion: > > // neon and gpr computations are interleaved to maximize parallelism Accepted. > src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7864: > >> 7862: __ mov(mul_ptr, sp); >> 7863: >> 7864: neon_partial_mult_64(A, b_lows, a_vals, 0); > > Suggestion: > > // cross-multiply low * low for limbs b0-b3 and a3-a4 in parallel > neon_partial_mult_64(A, b_lows, a_vals, 0); Accepted. > src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7871: > >> 7869: __ andr(n, low, limb_mask); >> 7870: >> 7871: neon_partial_mult_64(B, b_highs, a_vals, 0); > > Suggestion: > > // cross-multiply high * low for limbs b0-b3 and a3-a4 in parallel > neon_partial_mult_64(B, b_highs, a_vals, 0); Accepted. > src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7880: > >> 7878: __ add(c_i, c_i, high); >> 7879: >> 7880: neon_partial_mult_64(C, b_lows, a_vals, 1); > > Suggestion: > > // cross-multiply low * high for limbs b0-b3 and a3-a4 in parallel > neon_partial_mult_64(C, b_lows, a_vals, 1); Accepted. > src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7885: > >> 7883: gpr_partial_mult_52(a_i, b_1, high, low, tmp, limb_mask); >> 7884: >> 7885: neon_partial_mult_64(D, b_highs, a_vals, 1); > > Suggestion: > > // cross-multiply high * high for limbs b0-b3 and a3-a4 in parallel > neon_partial_mult_64(D, b_highs, a_vals, 1); Accepted. > src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7894: > >> 7892: __ mov(c_i, high); >> 7893: >> 7894: vs_addv(B, __ T2D, B, C); // Store (B+C) in B > > Suggestion: > > // combine neon 32-bit partial products, regrouping to produce > // 8*52-bit low products in A and 8*52-bit high products in D > > // add low*high/high*low intermediate products before regrouping > vs_addv(B, __ T2D, B, C); // Store (B+C) in B Accepted. > src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7902: > >> 7900: __ mov(c_i, high); >> 7901: >> 7902: vs_shl(D, __ T2D, D, montMulP256Shift1); > > Suggestion: > > // shift high*high (40-bit) product up into 52-bits of output > vs_shl(D, __ T2D, D, montMulP256Shift1); Accepted. > src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7907: > >> 7905: gpr_partial_mult_52(a_i, b_3, high, low, tmp, limb_mask); >> 7906: >> 7907: vs_ushr(C, __ T2D, B, 32 - montMulP256Shift1); // Use C for ((B+C) >> >>> 20) > > Suggestion: > > // shift high 32 (or 33) bits of intermediate products for addition to D > vs_ushr(C, __ T2D, B, 32 - montMulP256Shift1); // Use C for ((B+C) >>> 20) Accepted. > src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7916: > >> 7914: __ mov(c_i, high); >> 7915: >> 7916: vs_shl(B, __ T2D, B, 32); > > Suggestion: > > // shift low 32 bits of intermediate product up for masking and addition > to A > vs_shl(B, __ T2D, B, 32); Accepted. > src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7921: > >> 7919: gpr_partial_mult_52(a_i, b_4, high, low, tmp, limb_mask); >> 7920: >> 7921: vs_addv(D, __ T2D, D, C); > > Suggestion: > > // add high bits of intermediate product into D > vs_addv(D, __ T2D, D, C); Accepted. > src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7930: > >> 7928: __ str(high, Address(c_ptr, 32)); >> 7929: >> 7930: vs_ushr(C, __ T2D, A, 52); // C now holds (A >>> 52) > > Suggestion: > > // top 12 bits of 32*32 bit product in A need adding into high 56-bit > output > vs_ushr(C, __ T2D, A, 52); // C now holds (A >>> 52) Accepted. > src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7931: > >> 7929: >> 7930: vs_ushr(C, __ T2D, A, 52); // C now holds (A >>> 52) >> 7931: vs_andr(B, B, limb_mask_vec); > > Suggestion: > > // Only 20 of the 32 bits now in the top of B should be added into A > vs_andr(B, B, limb_mask_vec); Accepted. > src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7932: > >> 7930: vs_ushr(C, __ T2D, A, 52); // C now holds (A >>> 52) >> 7931: vs_andr(B, B, limb_mask_vec); >> 7932: vs_andr(A, A, limb_mask_vec); > > Suggestion: > > // reduce original 64-bit product to 52-bits > vs_andr(A, A, limb_mask_vec); Accepted. > src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7933: > >> 7931: vs_andr(B, B, limb_mask_vec); >> 7932: vs_andr(A, A, limb_mask_vec); >> 7933: vs_addv(D, __ T2D, D, C); > > Suggestion: > > // add intermediate products to high 52-bit result in D > vs_addv(D, __ T2D, D, C); Accepted. >
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Fri, 15 May 2026 09:52:20 GMT, Ferenc Rakoczi wrote: >> An aarch64 implementation of the MontgomeryIntegerPolynomial256.mult() >> method and IntegerPolynomial.conditionalAssign(). Since 64-bit >> multiplication is not supported on Neon and manually performing this >> operation with 32-bit limbs is slower than with GPRs, a hybrid neon/gpr >> approach is used. Neon instructions are used to compute intermediate values >> used in the last two iterations of the main "loop", while the GPRs compute >> the first few iterations. At the method level this improves performance by >> ~9% and at the API level roughly 5%. >> >> >> >> - >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Ferenc Rakoczi has updated the pull request incrementally with one additional > commit since the last revision: > > Accepting more suggestions from Andrew Dinn. @ferakocz I have done a full pass of the PR and made all the recommendations I have to offer for now. Please address them plus the comments made by @shipilev and @theRealAph and then I will do a second pass. - PR Review: https://git.openjdk.org/jdk/pull/30941#pullrequestreview-4335460156
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Wed, 20 May 2026 15:54:18 GMT, Aleksey Shipilev wrote: >> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8551: >> >>> 8549: >>> 8550: Label default_loop; >>> 8551: __ BIND(default_loop); >> >> How many iterations does this loop do when `length` is `0`? The `sub` below >> would underflow to `-1`, right? Is it possible to get here with `length == >> 0`? > > I think x86 has an explicit test for `length == 0`: > https://github.com/openjdk/jdk/blob/6fb07f9b26bcd23f622a50c6ca5f23363b432f85/src/hotspot/cpu/x86/stubGenerator_x86_64_poly_mont.cpp#L740-L742 There is certainly the possibility of this happening. Most subtypes employ a hard-wired non-zero limb count but the constructor for `IntegerPolynomialModBinP` accepts a limb count argument which could certainly be passed as zero -- with zero length arrays then being employed for the limb data (obviously a negative count won't cut it). I'm not clear whether that would trip over other assumptions before a mult call could occur but it costs little to defend against the possibility that it might not so probably best to be safe? - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3279862454
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Thu, 21 May 2026 08:24:31 GMT, Andrew Haley wrote:
>> src/hotspot/cpu/aarch64/assembler_aarch64.hpp line 3166:
>>
>>> 3164: assert(Ts == H ? Vm->encoding() < 16 : Vm->encoding() < 32,
>>> "umull{2}v requires Vm to be in range V0..V15 when Ts is H");
>>> 3165: f(0, 31), f(q, 30), f(0b10, 29, 24), f(size, 23, 22), f(l,
>>> 21); //f(m, 20);
>>> 3166: rf(Vm, 16), f(0b1010, 15, 12), f(h, 11), f(0, 10), rf(Vn, 5),
>>> rf(Vd, 0);
>>
>> Why `f(m, 20)` is commented here? Does it need to be set?
>
> It doesn't. AArch64 assembler throws an assertion if any field of an
> instruction is not set. I think it's because `Vm` may have to be <15, so that
> bi twill be clear. But the comment is more confusing than helpful.
Still confused on correctness here, bear with me for a sec.
So current code means we set bit 20 implicitly to `0`, when `Ts == H`, because
it implies `Vm < 16`. But with `H` variant, don't we need to encode lane bits
as H:L:M explicitly? Meaning, `M` is the lower bit of the lane, shouldn't it be
dependent on `lane`, rather than being always `0`?
Actually, a few lines above, when `size == 0b01`, we do not take the lower bit
out of `lane` at all? That's our `M`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3279810188
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Fri, 15 May 2026 09:52:20 GMT, Ferenc Rakoczi wrote: >> An aarch64 implementation of the MontgomeryIntegerPolynomial256.mult() >> method and IntegerPolynomial.conditionalAssign(). Since 64-bit >> multiplication is not supported on Neon and manually performing this >> operation with 32-bit limbs is slower than with GPRs, a hybrid neon/gpr >> approach is used. Neon instructions are used to compute intermediate values >> used in the last two iterations of the main "loop", while the GPRs compute >> the first few iterations. At the method level this improves performance by >> ~9% and at the API level roughly 5%. >> >> >> >> - >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Ferenc Rakoczi has updated the pull request incrementally with one additional > commit since the last revision: > > Accepting more suggestions from Andrew Dinn. src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8377: > 8375: VSeq<4> b_vec(20); > 8376: > 8377: __ ldr(a9, Address(aLimbs, 64)); It makes things more obvious to a maintainer and avoids mistakes should anything need patching if you use 8 * BytesPerLong here instead of 64 and likewise for all the other hard-wired Address offsets used throughout this method. That way the offsets directly relate to the vector and GPR counts mentioned in the comments. - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3279780174
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Wed, 20 May 2026 16:24:59 GMT, Aleksey Shipilev wrote:
>> Ferenc Rakoczi has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Accepting more suggestions from Andrew Dinn.
>
> src/hotspot/cpu/aarch64/assembler_aarch64.hpp line 3166:
>
>> 3164: assert(Ts == H ? Vm->encoding() < 16 : Vm->encoding() < 32,
>> "umull{2}v requires Vm to be in range V0..V15 when Ts is H");
>> 3165: f(0, 31), f(q, 30), f(0b10, 29, 24), f(size, 23, 22), f(l,
>> 21); //f(m, 20);
>> 3166: rf(Vm, 16), f(0b1010, 15, 12), f(h, 11), f(0, 10), rf(Vn, 5),
>> rf(Vd, 0);
>
> Why `f(m, 20)` is commented here? Does it need to be set?
It doesn't. AArch64 assembler throws an assertion if any field of an
instruction is not set. I think it's because `Vm` may have to be <15, so that
bi twill be clear. But the comment is more confusing than helpful.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3279709250
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Fri, 15 May 2026 09:52:20 GMT, Ferenc Rakoczi wrote:
>> An aarch64 implementation of the MontgomeryIntegerPolynomial256.mult()
>> method and IntegerPolynomial.conditionalAssign(). Since 64-bit
>> multiplication is not supported on Neon and manually performing this
>> operation with 32-bit limbs is slower than with GPRs, a hybrid neon/gpr
>> approach is used. Neon instructions are used to compute intermediate values
>> used in the last two iterations of the main "loop", while the GPRs compute
>> the first few iterations. At the method level this improves performance by
>> ~9% and at the API level roughly 5%.
>>
>>
>>
>> -
>> - [x] I confirm that I make this contribution in accordance with the
>> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai).
>
> Ferenc Rakoczi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Accepting more suggestions from Andrew Dinn.
src/hotspot/cpu/aarch64/assembler_aarch64.hpp line 3166:
> 3164: assert(Ts == H ? Vm->encoding() < 16 : Vm->encoding() < 32,
> "umull{2}v requires Vm to be in range V0..V15 when Ts is H");
> 3165: f(0, 31), f(q, 30), f(0b10, 29, 24), f(size, 23, 22), f(l, 21);
> //f(m, 20);
> 3166: rf(Vm, 16), f(0b1010, 15, 12), f(h, 11), f(0, 10), rf(Vn, 5),
> rf(Vd, 0);
Why `f(m, 20)` is commented here? Does it need to be set?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3275515091
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Fri, 15 May 2026 09:52:20 GMT, Ferenc Rakoczi wrote: >> An aarch64 implementation of the MontgomeryIntegerPolynomial256.mult() >> method and IntegerPolynomial.conditionalAssign(). Since 64-bit >> multiplication is not supported on Neon and manually performing this >> operation with 32-bit limbs is slower than with GPRs, a hybrid neon/gpr >> approach is used. Neon instructions are used to compute intermediate values >> used in the last two iterations of the main "loop", while the GPRs compute >> the first few iterations. At the method level this improves performance by >> ~9% and at the API level roughly 5%. >> >> >> >> - >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Ferenc Rakoczi has updated the pull request incrementally with one additional > commit since the last revision: > > Accepting more suggestions from Andrew Dinn. src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8267: > 8265: // P521OrderField: 19 = 8 + 8 + 2 + 1 > 8266: // Special Cases 5, 10, 14, 16, 19 > 8267: You need to insert the standard call `load_archive_data` here and return any stub that is found. src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8568: > 8566: __ mov(r0, zr); // return 0 > 8567: __ ret(lr); > 8568: return start; You need to call `store_archive_data` here. - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3275486612 PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3275489536
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Wed, 20 May 2026 07:52:35 GMT, Aleksey Shipilev wrote: >> Ferenc Rakoczi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Accepting more suggestions from Andrew Dinn. > > src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8551: > >> 8549: >> 8550: Label default_loop; >> 8551: __ BIND(default_loop); > > How many iterations does this loop do when `length` is `0`? The `sub` below > would underflow to `-1`, right? Is it possible to get here with `length == 0`? I think x86 has an explicit test for `length == 0`: https://github.com/openjdk/jdk/blob/6fb07f9b26bcd23f622a50c6ca5f23363b432f85/src/hotspot/cpu/x86/stubGenerator_x86_64_poly_mont.cpp#L740-L742 - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3275327310
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Wed, 20 May 2026 07:54:50 GMT, Aleksey Shipilev wrote:
>> Ferenc Rakoczi has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Accepting more suggestions from Andrew Dinn.
>
> src/hotspot/cpu/aarch64/register_aarch64.hpp line 546:
>
>> 544: template
>> 545: VSeq vs_tail(const VSeq& v) {
>> 546: static_assert(N > 1, "tail sequence length must be greater than 2");
>
> Which one is it, `N > 1`, or `greater than 2`?
Full disclosure -- this is my copypasta introduced during an earlier round of
reviewing.
Given that we have a static assert that `N > 2` in the definition of class
`VSeq` I guess this is redundant. Likewise, the assertion in `vs_head`.
It doesn't really make any sense to /declare/ a `VSeq` with less than 2
elements as the purpose of the class is to provide a way to manage and access
more than one FloatRegister using indexed notation (`vs[i]`). However, the
provision of `vs_tail` gives rise to the possibility that we might create a
`VSeq<1>` from a `VSeq<2>` so we could consider adjusting the asserts
accordingly.
In fact, `vs_head` and `vs_tail` were added to deal with the need for the
current proposed kernel to operate on groups of 5 registers at a time. The
code uses `vs_head` to access the first register in the group and `vs_tail` to
access a sequence over the remaining four registers (n.b. calling `vs_head(vs)`
is not strictly needed -- you can just use `vs[0]` -- so maybe we don't need
to provide it).
So, for current requirements, we could just delete the assert here and in
`vs_head` and rely on the one in the definition of `VSeq`. If we ever need to
split off the tail of a `VSeq<2>` we could reconsider this.
Likewise we could drop `vs_head`. I thought it would be good to provide it
because it emphasise the way that the sequence is being recursively split in
two and ... well ... Lisp!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3274978618
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Fri, 15 May 2026 09:52:20 GMT, Ferenc Rakoczi wrote:
>> An aarch64 implementation of the MontgomeryIntegerPolynomial256.mult()
>> method and IntegerPolynomial.conditionalAssign(). Since 64-bit
>> multiplication is not supported on Neon and manually performing this
>> operation with 32-bit limbs is slower than with GPRs, a hybrid neon/gpr
>> approach is used. Neon instructions are used to compute intermediate values
>> used in the last two iterations of the main "loop", while the GPRs compute
>> the first few iterations. At the method level this improves performance by
>> ~9% and at the API level roughly 5%.
>>
>>
>>
>> -
>> - [x] I confirm that I make this contribution in accordance with the
>> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai).
>
> Ferenc Rakoczi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Accepting more suggestions from Andrew Dinn.
A few drive-by comments. I have not dug into the core of the implementation.
How would you test this for correctness?
src/hotspot/cpu/aarch64/register_aarch64.hpp line 546:
> 544: template
> 545: VSeq vs_tail(const VSeq& v) {
> 546: static_assert(N > 1, "tail sequence length must be greater than 2");
Which one is it, `N > 1`, or `greater than 2`?
src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8018:
> 8016: __ ldr(c_i, c_ptr);
> 8017:
> 8018: // Limb 1
The numbering is a bit off here. There are duplicate `// Limb 2` comments.
Should this one be `// Limb 0`?
src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8183:
> 8181: tmp2 = *common_regs++,
> 8182: tmp3 = *common_regs++,
> 8183: tmp4 = *common_regs++;
Cannot quite see where these `tmp*`-s are used?
src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8551:
> 8549:
> 8550: Label default_loop;
> 8551: __ BIND(default_loop);
How many iterations does this loop do when `length` is `0`? The `sub` below
would underflow to `-1`, right? Is it possible to get here with `length == 0`?
src/hotspot/cpu/aarch64/vm_version_aarch64.cpp line 462:
> 460: if (FLAG_IS_DEFAULT(UseIntPolyIntrinsics)) {
> 461: UseIntPolyIntrinsics = true;
> 462: }
Indenting is a bit off here. Also, should this depend on `CPU_ASIMD`, like
ChaCha20 intrinsic enablement a few blocks above?
-
PR Review: https://git.openjdk.org/jdk/pull/30941#pullrequestreview-4326274500
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3272151694
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3272147603
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3272187350
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3272136785
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3272122856
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Fri, 15 May 2026 09:52:20 GMT, Ferenc Rakoczi wrote: >> An aarch64 implementation of the MontgomeryIntegerPolynomial256.mult() >> method and IntegerPolynomial.conditionalAssign(). Since 64-bit >> multiplication is not supported on Neon and manually performing this >> operation with 32-bit limbs is slower than with GPRs, a hybrid neon/gpr >> approach is used. Neon instructions are used to compute intermediate values >> used in the last two iterations of the main "loop", while the GPRs compute >> the first few iterations. At the method level this improves performance by >> ~9% and at the API level roughly 5%. >> >> >> >> - >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Ferenc Rakoczi has updated the pull request incrementally with one additional > commit since the last revision: > > Accepting more suggestions from Andrew Dinn. Should these new stubs be AOT-saved/restored? See how other stubs are doing `load_archive_data` / `save_archive_data` dance. - PR Review: https://git.openjdk.org/jdk/pull/30941#pullrequestreview-4326265716
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Fri, 15 May 2026 09:52:20 GMT, Ferenc Rakoczi wrote: >> An aarch64 implementation of the MontgomeryIntegerPolynomial256.mult() >> method and IntegerPolynomial.conditionalAssign(). Since 64-bit >> multiplication is not supported on Neon and manually performing this >> operation with 32-bit limbs is slower than with GPRs, a hybrid neon/gpr >> approach is used. Neon instructions are used to compute intermediate values >> used in the last two iterations of the main "loop", while the GPRs compute >> the first few iterations. At the method level this improves performance by >> ~9% and at the API level roughly 5%. >> >> >> >> - >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Ferenc Rakoczi has updated the pull request incrementally with one additional > commit since the last revision: > > Accepting more suggestions from Andrew Dinn. src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8009: > 8007: // a[3] > 8008: // > 8009: Suggestion: // For a_3 and a_4 we have already computed the cross-products // with b_0 ... b_3 and stored them on the stack relative to // `mul_ptr` i.e. the current `sp`. - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3267924105
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Fri, 15 May 2026 09:52:20 GMT, Ferenc Rakoczi wrote: >> An aarch64 implementation of the MontgomeryIntegerPolynomial256.mult() >> method and IntegerPolynomial.conditionalAssign(). Since 64-bit >> multiplication is not supported on Neon and manually performing this >> operation with 32-bit limbs is slower than with GPRs, a hybrid neon/gpr >> approach is used. Neon instructions are used to compute intermediate values >> used in the last two iterations of the main "loop", while the GPRs compute >> the first few iterations. At the method level this improves performance by >> ~9% and at the API level roughly 5%. >> >> >> >> - >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Ferenc Rakoczi has updated the pull request incrementally with one additional > commit since the last revision: > > Accepting more suggestions from Andrew Dinn. src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8077: > 8075: + b_0 + b_1 + b_2; > 8076: b_0 = b_1 = b_2 = noreg; > 8077: This freeing of registers would be better done at line 8002 (i.e. before processing a3) and with register b_3 also freed. It highlights to the maintainer that b0 - b3 are no longer needed from that point on. src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8136: > 8134: __ add(high, high, mod_high); > 8135: > 8136: // Reallocate regs b_3, b_4 Likewise at this point we should be freeing `b_4` having already freed `b_3`. src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8238: > 8236: > 8237: // End intrinsic call > 8238: __ add(sp, sp, 176); This constant could be replaced with `C_DATA_SIZE + MUL_DATA_SIZE` - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3267784068 PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3267795849 PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3267814052
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Fri, 15 May 2026 09:52:20 GMT, Ferenc Rakoczi wrote:
>> An aarch64 implementation of the MontgomeryIntegerPolynomial256.mult()
>> method and IntegerPolynomial.conditionalAssign(). Since 64-bit
>> multiplication is not supported on Neon and manually performing this
>> operation with 32-bit limbs is slower than with GPRs, a hybrid neon/gpr
>> approach is used. Neon instructions are used to compute intermediate values
>> used in the last two iterations of the main "loop", while the GPRs compute
>> the first few iterations. At the method level this improves performance by
>> ~9% and at the API level roughly 5%.
>>
>>
>>
>> -
>> - [x] I confirm that I make this contribution in accordance with the
>> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai).
>
> Ferenc Rakoczi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Accepting more suggestions from Andrew Dinn.
@ferakocz Still working my way through this. Here are a few more
recommendations for comments and code cleanups. Also, I offered a correction to
a comment where I misread the data layout for the Neon data written to the
stack.
src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7869:
> 7867: __ ldr(a_i, __ post(a, 8));
> 7868: gpr_partial_mult_52(a_i, b_0, high, low, tmp, limb_mask);
> 7869: __ andr(n, low, limb_mask);
It would be clearer to use `mov(n, low)` here or, at the very least `orr(n, zr,
low)`. `low` has already been masked with `limb_mask`, which makes the `andr`
look redundant when, in fact, the point is to save the initial value computed
for `low`.
src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7873:
> 7871: neon_partial_mult_64(B, b_highs, a_vals, 0);
> 7872:
> 7873: // Limb 0 cont
Suggestion:
// Limb 0 modulus computation
// n.b. modulus computation requires multiplying successive
// limbs of the product by corresponding limbs of the p256
// prime adding the result to the limb and folding this
// partial result into a running 256-bit sum in c_i. Limbs
// of c_i are stored via c_ptr once carries are included.
// n.b. the mul + add is omitted for limb 2 since the
// corresponding prime bits are zero.
src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7899:
> 7897: gpr_partial_mult_52(a_i, b_2, high, low, tmp, limb_mask);
> 7898: __ add(c_i, c_i, low);
> 7899: __ str(c_i, Address(c_ptr, 8));
I don't really like the hard-coded numbers in these `c_ptr` address
expressions. At the very least it would be better if they were be specified
using a const expression like `nnn * BytesPerLong` to show that we are indexing
an array of 64 bit data at index `nnn`.
A better idea would be to define a static helper method that also checks the
index is in range
#define C_DATA_COUNT 5
static int c_slot_offset(index i) {
assert(0 <= i && i < C_SLOT_COUNT, "invalid index for c_i data");
return i * BytesPerLong;
}
You can then define the address as `Address(c_ptr, c_offset(1))` etc.
Also, once you have defined `C_DATA_COUNT` you can also define a constant
C_DATA_SIZE for use when you extend the stack:
#define C_DATA_SIZE align_up(C_DATA_COUNT, 2) * BytesPerLong;
. . .
__ sub(sp, sp, C_DATA_SIZE);
. . .
The same consideration applies with even more force as regards the hard-wired
offsets used when accessing the Neon multiplication product data via `sp`.
__ ldr(low_1, Address(sp));
__ ldr(high_1, Address(sp, 16));
This also allows you to fold the details of the data layout into the function:
#define MUL_DATA_COUNT 8
static int mul_data_offset(index i, boolean want_high) {
assert(0 <= i && i < MUL_DATA_COUNT, "invalid index for mul data");
// data is stored as successive low/high pairs
// (lo0, l1), (hi0, hi1), ... (hi2, hi3)
int pair_start = (i >> 1) * 4;
int high_select = (want_high ? 2 : 0)
int pair_element = (i & 1);
int idx = pair_start + high_select + pair_element;
return (idx * BytesPerLong);
}
. . .
__ ldr(low_1, Address(sp, mul_data_offset(0, false));
__ ldr(high_1, Address(sp, mul_data_offset(0, true)));
. . .
Likewise you can define a constant MUL_DATA_SIZE for use when you extend the
stack:
#define MUL_DATA_SIZE (MUL_DATA_COUNT * 2) * BytesPerLong;
. . .
__ sub(sp, sp, MUL_DATA_SIZE);
. . .
-
PR Review: https://git.openjdk.org/jdk/pull/30941#pullrequestreview-4311333998
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3259942006
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3260445976
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3267260201
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Mon, 18 May 2026 10:19:05 GMT, Andrew Dinn wrote: >> Ferenc Rakoczi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Accepting more suggestions from Andrew Dinn. > > src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7939: > >> 7937: vs_addv(D, __ T2D, D, B); >> 7938: >> 7939: vs_st1_interleaved(A, D, mul_ptr); > > Suggestion: > > // interleaved write outputs 8 successive (low, high) 56-bit pairs > vs_st1_interleaved(A, D, mul_ptr); Actually, I see now that my comment was not correct. The write interleaves 4 successive pairs of low values with four pairs of successive high values i.e. (low0, low1) (high0, high1) . . . Suggestion: // the write interleaves the 4 successive pairs of low and // high results: (l0, l1), (h0, h1), ... (l6, l7), (h6, h7) vs_st1_interleaved(A, D, mul_ptr); - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3266623672
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Mon, 18 May 2026 14:59:38 GMT, Andrew Dinn wrote: >> Ferenc Rakoczi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Accepting more suggestions from Andrew Dinn. > > src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7758: > >> 7756: __ lsr(tmp, lo, montMulP256Shift2); >> 7757: __ orr(hi, hi, tmp); >> 7758: __ andr(lo, lo, mask); > > Suggestion: > > // compute 104-bit (40 + 64) full product > __ umulh(hi, a, b); > __ mul(lo, a, b); > // combine 40 + 12 bits into hi result > __ lsl(hi, hi, montMulP256Shift1); > __ lsr(tmp, lo, montMulP256Shift2); > __ orr(hi, hi, tmp); > // mask off 52 bits of lo result > __ andr(lo, lo, mask); It might be better and clearer to use `bfm` rather that shifting, masking, and ORing. - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3264769223
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Fri, 15 May 2026 09:52:20 GMT, Ferenc Rakoczi wrote: >> An aarch64 implementation of the MontgomeryIntegerPolynomial256.mult() >> method and IntegerPolynomial.conditionalAssign(). Since 64-bit >> multiplication is not supported on Neon and manually performing this >> operation with 32-bit limbs is slower than with GPRs, a hybrid neon/gpr >> approach is used. Neon instructions are used to compute intermediate values >> used in the last two iterations of the main "loop", while the GPRs compute >> the first few iterations. At the method level this improves performance by >> ~9% and at the API level roughly 5%. >> >> >> >> - >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Ferenc Rakoczi has updated the pull request incrementally with one additional > commit since the last revision: > > Accepting more suggestions from Andrew Dinn. src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7758: > 7756: __ lsr(tmp, lo, montMulP256Shift2); > 7757: __ orr(hi, hi, tmp); > 7758: __ andr(lo, lo, mask); Suggestion: // compute 104-bit (40 + 64) full product __ umulh(hi, a, b); __ mul(lo, a, b); // combine 40 + 12 bits into hi result __ lsl(hi, hi, montMulP256Shift1); __ lsr(tmp, lo, montMulP256Shift2); __ orr(hi, hi, tmp); // mask off 52 bits of lo result __ andr(lo, lo, mask); - PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3259892902
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
On Fri, 15 May 2026 09:52:20 GMT, Ferenc Rakoczi wrote: >> An aarch64 implementation of the MontgomeryIntegerPolynomial256.mult() >> method and IntegerPolynomial.conditionalAssign(). Since 64-bit >> multiplication is not supported on Neon and manually performing this >> operation with 32-bit limbs is slower than with GPRs, a hybrid neon/gpr >> approach is used. Neon instructions are used to compute intermediate values >> used in the last two iterations of the main "loop", while the GPRs compute >> the first few iterations. At the method level this improves performance by >> ~9% and at the API level roughly 5%. >> >> >> >> - >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Ferenc Rakoczi has updated the pull request incrementally with one additional > commit since the last revision: > > Accepting more suggestions from Andrew Dinn. @ferakocz I have made some suggestions for comments that will aid maintainers in understanding what the Neon-based subset of the implementation is doing. I still need to work through the GPR-based code. src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7863: > 7861: __ sub(sp, sp, 128); > 7862: __ mov(mul_ptr, sp); > 7863: Suggestion: // neon and gpr computations are interleaved to maximize parallelism src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7864: > 7862: __ mov(mul_ptr, sp); > 7863: > 7864: neon_partial_mult_64(A, b_lows, a_vals, 0); Suggestion: // cross-multiply low * low for limbs b0-b3 and a3-a4 in parallel neon_partial_mult_64(A, b_lows, a_vals, 0); src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7871: > 7869: __ andr(n, low, limb_mask); > 7870: > 7871: neon_partial_mult_64(B, b_highs, a_vals, 0); Suggestion: // cross-multiply high * low for limbs b0-b3 and a3-a4 in parallel neon_partial_mult_64(B, b_highs, a_vals, 0); src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7880: > 7878: __ add(c_i, c_i, high); > 7879: > 7880: neon_partial_mult_64(C, b_lows, a_vals, 1); Suggestion: // cross-multiply low * high for limbs b0-b3 and a3-a4 in parallel neon_partial_mult_64(C, b_lows, a_vals, 1); src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7885: > 7883: gpr_partial_mult_52(a_i, b_1, high, low, tmp, limb_mask); > 7884: > 7885: neon_partial_mult_64(D, b_highs, a_vals, 1); Suggestion: // cross-multiply high * high for limbs b0-b3 and a3-a4 in parallel neon_partial_mult_64(D, b_highs, a_vals, 1); src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7894: > 7892: __ mov(c_i, high); > 7893: > 7894: vs_addv(B, __ T2D, B, C); // Store (B+C) in B Suggestion: // combine neon 32-bit partial products, regrouping to produce // 8*52-bit low products in A and 8*52-bit high products in D // add low*high/high*low intermediate products before regrouping vs_addv(B, __ T2D, B, C); // Store (B+C) in B src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7902: > 7900: __ mov(c_i, high); > 7901: > 7902: vs_shl(D, __ T2D, D, montMulP256Shift1); Suggestion: // shift high*high (40-bit) product up into 52-bits of output vs_shl(D, __ T2D, D, montMulP256Shift1); src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7907: > 7905: gpr_partial_mult_52(a_i, b_3, high, low, tmp, limb_mask); > 7906: > 7907: vs_ushr(C, __ T2D, B, 32 - montMulP256Shift1); // Use C for ((B+C) > >>> 20) Suggestion: // shift high 32 (or 33) bits of intermediate products for addition to D vs_ushr(C, __ T2D, B, 32 - montMulP256Shift1); // Use C for ((B+C) >>> 20) src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7916: > 7914: __ mov(c_i, high); > 7915: > 7916: vs_shl(B, __ T2D, B, 32); Suggestion: // shift low 32 bits of intermediate product up for masking and addition to A vs_shl(B, __ T2D, B, 32); src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7921: > 7919: gpr_partial_mult_52(a_i, b_4, high, low, tmp, limb_mask); > 7920: > 7921: vs_addv(D, __ T2D, D, C); Suggestion: // add high bits of intermediate product into D vs_addv(D, __ T2D, D, C); src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7930: > 7928: __ str(high, Address(c_ptr, 32)); > 7929: > 7930: vs_ushr(C, __ T2D, A, 52); // C now holds (A >>> 52) Suggestion: // top 12 bits of 32*32 bit product in A need adding into high 56-bit output vs_ushr(C, __ T2D, A, 52); // C now holds (A >>> 52) src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7931: > 7929: > 7930: vs_ushr(C, __ T2D, A, 52); // C now holds (A >>> 52) > 7931: vs_andr(B, B, limb_mask_vec); Suggestion: // Only 20 of the 32 bits now in the top of B should be added into A vs_andr(B, B, limb_mask_vec); src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 7932: > 7930: vs_ushr(C, __ T2D,
Re: RFR: 8355216: Accelerate P-256 arithmetic on aarch64 [v5]
> An aarch64 implementation of the MontgomeryIntegerPolynomial256.mult() method > and IntegerPolynomial.conditionalAssign(). Since 64-bit multiplication is not > supported on Neon and manually performing this operation with 32-bit limbs is > slower than with GPRs, a hybrid neon/gpr approach is used. Neon instructions > are used to compute intermediate values used in the last two iterations of > the main "loop", while the GPRs compute the first few iterations. At the > method level this improves performance by ~9% and at the API level roughly 5%. > > > > - > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). Ferenc Rakoczi has updated the pull request incrementally with one additional commit since the last revision: Accepting more suggestions from Andrew Dinn. - Changes: - all: https://git.openjdk.org/jdk/pull/30941/files - new: https://git.openjdk.org/jdk/pull/30941/files/c1d649b2..01baecea Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=30941&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=30941&range=03-04 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/30941.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/30941/head:pull/30941 PR: https://git.openjdk.org/jdk/pull/30941
