On Thu, 29 Oct 2026 00:12:00 GMT, Shawn Emery <[email protected]> wrote:
> This set of changes implements multiplication and squaring field operations > on the P-384 curve using AArch64 intrinsics. The performance gains from this > enhancement yields: > > Key pair generation - P-384: +36% > Key agreement - P-384: +36% > KEM decapsulation - secP384r1: +35% > KEM encapsulation - secP384r1: +36% > > Key pair generation - secP384r1MLKEM1024: +35% > KEM decapsulation - secP384r1MLKEM1024: +35% > KEM encapsulation - secP384r1MLKEM1024: +36% > > --------- > - [X] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). Looks correct to me but could do with a few tidy-ups. Can you also clarify how you have tested this? src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 9152: > 9150: auto saved_regs = RegSet::range(r19, r20); > 9151: > 9152: const int32_t limbs = 7; I'm getting DEJA_VU here. These could be defined as constants at file scope (caps with a suitable P384 suffix) allowing them to be shared by both top level intrinsic generators and also any auxiliary helpers you might want to introduce (see below). src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 9175: > 9173: for (int i = 0; i < 7; i++) { > 9174: __ ldr(aArg, Address(aLimbs, i * 8)); > 9175: __ smulh(high, aArg, aArg); Hmm, yet more DEJA_VU. Can we have an auxiliary generator method mul55(c_lo, c_hi, a, b, h, l) for this repeated sequence. That serves to clarify that the same higher level operation that is being performed and makes it harder for a maintainer to avoid introducing errors. src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 9192: > 9190: } > 9191: > 9192: Register v = a; Is this not just a cut and paste of the same generated code for `carryReduce` implementation that we see in `generate_intpoly_mult_P384`. If it is not then where (and why) do the two implementations differ? If it is the same then we should really be generating this using an auxiliary method (`generate_carry_reduce_P384`). src/hotspot/share/runtime/stubDeclarations.hpp line 823: > 821: do_entry(compiler, intpoly_square_25519, \ > 822: intpoly_square_25519, intpoly_square_25519) \ > 823: do_stub(compiler, intpoly_mult_P384) \ Adding an implementation for a new intrinsic on any given architecture increases the amount of code generated. That could potentially lead to overflow of the compiler StubGen blob on that architecture. Can you report how much this change contributes to the blob on AArch64. If it is significant then you will need to update the arch-specific StubGen compiler blob sizing in file stubDeclarations_aarch64.hpp n.b. You can get a printout of the StubGen blob sizes by running with `-Xlog:stubs=info` and enabling/disabling the intrinsic to obtain a delta. Alternatively, you could eyeball the actual stub code and measure its size by running with `-XX:+UnlockDiagnosticVMOptions -XX:+PrintStubCode`. ------------- Changes requested by adinn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/33618#pullrequestreview-5857804625 PR Review Comment: https://git.openjdk.org/jdk/pull/33618#discussion_r4537184279 PR Review Comment: https://git.openjdk.org/jdk/pull/33618#discussion_r4537243930 PR Review Comment: https://git.openjdk.org/jdk/pull/33618#discussion_r4537143245 PR Review Comment: https://git.openjdk.org/jdk/pull/33618#discussion_r4537407983
