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