RFR: 8319716: RISC-V: Add SHA-2 [v8]
Fei Yang
fyang at openjdk.org
Fri Dec 22 05:47:53 UTC 2023
On Thu, 21 Dec 2023 14:41:06 GMT, Robbin Ehn <rehn at openjdk.org> wrote:
>> Hi, please consider.
>>
>> Main author is @luhenry, I only fixed some minor things and tested it.
>>
>> Such as:
>> test/hotspot/jtreg/compiler/intrinsics/sha/
>> test/jdk/java/security/MessageDigest/
>> test/jdk/jdk/security/
>> tier1
>>
>> And still running some test.
>
> Robbin Ehn has updated the pull request incrementally with one additional commit since the last revision:
>
> index store state back
Thanks for the update. I still have several comments after a more closer look.
src/hotspot/cpu/riscv/stubGenerator_riscv.cpp line 3705:
> 3703: // vl1reXX.v v15, ofs
> 3704: //
> 3705: // // Increment word contant address by stride (16/32 bytes, 4*4B/8B, 128b/256b)
Nit: s/contant/constant/
src/hotspot/cpu/riscv/stubGenerator_riscv.cpp line 3812:
> 3810: if (vset_sew == Assembler::e64 && !multi_block) return "sha512_implCompress";
> 3811: if (vset_sew == Assembler::e64 && multi_block) return "sha512_implCompressMB";
> 3812: return "bad name lookup";
Maybe place a `ShouldNotReachHere();` immediately before the last return statement?
src/hotspot/cpu/riscv/stubGenerator_riscv.cpp line 3935:
> 3933: // x0 is not written, we known the number of vector elements.
> 3934:
> 3935: __ vsetivli(x0, 4, vset_sew, Assembler::m1, Assembler::ma, Assembler::ta);
Shouldn't we set `LMUL` to `Assembler::m2` when `vset_sew` is `e64`? A single 128-bit wide vector register won't be able to hold 4 `e64` elements. I guess you were testing against 256-bit wide vector register so that this issue won't trigger? This also means most of the code won't work for sha512 with 128-bit RVV as `Assembler::m2` means register group of two. This menifests in the corresponding openssl sha512 implementation [1].
[1] https://github.com/openssl/openssl/blob/master/crypto/sha/asm/sha512-riscv64-zvkb-zvknhb.pl
src/hotspot/cpu/riscv/stubGenerator_riscv.cpp line 3942:
> 3940:
> 3941: // Step-over a,b, so we are pointing to c.
> 3942: // const_add is equal to 4x state variable, div by 2 is thus 2, a,b
I don't quite understand this code comment.
-------------
Changes requested by fyang (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16562#pullrequestreview-1793885724
PR Review Comment: https://git.openjdk.org/jdk/pull/16562#discussion_r1434664804
PR Review Comment: https://git.openjdk.org/jdk/pull/16562#discussion_r1434718482
PR Review Comment: https://git.openjdk.org/jdk/pull/16562#discussion_r1434708758
PR Review Comment: https://git.openjdk.org/jdk/pull/16562#discussion_r1434729652
More information about the hotspot-dev
mailing list