On Fri, 28 Oct 2022 20:39:44 GMT, vpaprotsk <d...@openjdk.org> wrote:
>> Handcrafted x86_64 asm for Poly1305. Main optimization is to process 16
>> message blocks at a time. For more details, left a lot of comments in
>> `macroAssembler_x86_poly.cpp`.
>>
>> - Added new KAT test for Poly1305 and a fuzz test to compare intrinsic and
>> java.
>> - Would like to add an `InvalidKeyException` in `Poly1305.java` (see
>> commented out block in that file), but that conflicts with the KAT. I do
>> think we should detect (R==0 || S ==0) so would like advice please.
>> - Added a JMH perf test.
>> - JMH test had to use reflection (instead of existing `MacBench.java`),
>> since Poly1305 is not 'properly' registered with the provider.
>>
>> Perf before:
>>
>> Benchmark (dataSize) (provider) Mode Cnt Score
>> Error Units
>> Poly1305DigestBench.digest 64 thrpt 8 2961300.661
>> ± 110554.162 ops/s
>> Poly1305DigestBench.digest 256 thrpt 8 1791912.962
>> ± 86696.037 ops/s
>> Poly1305DigestBench.digest 1024 thrpt 8 637413.054
>> ± 14074.655 ops/s
>> Poly1305DigestBench.digest 16384 thrpt 8 48762.991
>> ± 390.921 ops/s
>> Poly1305DigestBench.digest 1048576 thrpt 8 769.872
>> ± 1.402 ops/s
>>
>> and after:
>>
>> Benchmark (dataSize) (provider) Mode Cnt Score
>> Error Units
>> Poly1305DigestBench.digest 64 thrpt 8 2841243.668
>> ± 154528.057 ops/s
>> Poly1305DigestBench.digest 256 thrpt 8 1662003.873
>> ± 95253.445 ops/s
>> Poly1305DigestBench.digest 1024 thrpt 8 1770028.718
>> ± 100847.766 ops/s
>> Poly1305DigestBench.digest 16384 thrpt 8 765547.287
>> ± 25883.825 ops/s
>> Poly1305DigestBench.digest 1048576 thrpt 8 14508.458
>> ± 56.147 ops/s
>
> vpaprotsk has updated the pull request incrementally with one additional
> commit since the last revision:
>
> invalidkeyexception and some review comments
src/hotspot/cpu/x86/macroAssembler_x86.hpp line 970:
> 968:
> 969: void addmq(int disp, Register r1, Register r2);
> 970:
All Poly1305-related methods can be moved to `StubGenerator`. They are used
solely during stub creation.
src/hotspot/cpu/x86/macroAssembler_x86_poly.cpp line 32:
> 30: #include "macroAssembler_x86.hpp"
> 31:
> 32: #ifdef _LP64
You could rename the file to `macroAssembler_x86_64_poly.cpp` and get rid of
`#ifdef _LP64`.
Once you move the declarations to `StubGenerator`, it'll be
`stubGenerator_x86_64_poly.cpp`.
src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 2002:
> 2000: }
> 2001:
> 2002: address StubGenerator::generate_poly1305_masksCP() {
I suggest to turn it into a C++ literal constant and move the declaration next
to `poly1305_process_blocks_avx512` where they are used. As an example, here's
how it is handled in GHASH stubs:
https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/x86/stubGenerator_x86_64_ghash.cpp#L35
That would allow to avoid to simplify the code a bit (no need in
`StubRoutines::x86::_poly1305_mask_addr`/`poly1305_mask_addr()` and no need to
generate the constants during VM startup).
You could split it into 3 constants, but then using a single base register
(`polyCP`) won't work anymore.
Thinking more about it, I'm not sure why you can't just do the split and use
address literals instead to access individual constants (and repurpose `r13`
to be used as a scratch register when RIP-relative addressing mode doesn't
work).
src/hotspot/share/runtime/globals.hpp line 241:
> 239: "Use intrinsics for java.util.Base64")
> \
> 240:
> \
> 241: product(bool, UsePolyIntrinsics, false,
> \
I'm not a fan of introducing new flags for individual intrinsics (there's
already `-XX:DisableIntrinsic=_name` specifically for that), but since we
already have many, shouldn't it be declared as a diagnostic flag, at least?
-------------
PR: https://git.openjdk.org/jdk/pull/10582
