RFR: 8288047: Accelerate Poly1305 on x86_64 using AVX512 instructions [v6]

Vladimir Ivanov vlivanov at openjdk.org
Tue Nov 1 23:34:34 UTC 2022


On Fri, 28 Oct 2022 20:39:44 GMT, vpaprotsk <duke at 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



More information about the security-dev mailing list