RFR: 8322179: RISC-V: Implement SHA-1 intrinsic [v9]

[Fei Yang]

On Mon, 5 Feb 2024 10:19:14 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> Hi,
>> Can you review this patch to implement SHA-1 intrinsic for riscv?
>> Thanks!
>> 
>> 
>> ## Test
>> 
>> ### Functionality
>> 
>> tests under `test/hotspot/jtreg/compiler/intrinsics/sha`
>> tests found via `find test/jdk -iname "*SHA1*.java"`
>> 
>> ### Performance
>> 
>> tested on `T-HEAD Light Lichee Pi 4A`
>> 
>> JMH_PARAMS="-f 1 -wi 10 -i 20" // for every loop of jmh test
>> 
>> benchmark tests `MessageDigests.java GetMessageDigest.java MessageDigestBench.java MacBench.java` which are under `test/micro/org/openjdk/bench/`, more spcifically `TESTS="MessageDigests.digest MessageDigests.getAndDigest MessageDigestBench.digest"`
>> 
>> 
>> // After
>> o.o.b.java.security.MessageDigests.digest                   N/A         N/A           SHA-1        64     DEFAULT  avgt   20      1845.446 ?     27.052  ns/op
>> o.o.b.java.security.MessageDigests.digest                   N/A         N/A           SHA-1     16384     DEFAULT  avgt   20    181455.350 ?    532.258  ns/op
>> o.o.b.java.security.MessageDigests.getAndDigest             N/A         N/A           SHA-1        64     DEFAULT  avgt   20      2447.674 ?     10.239  ns/op
>> o.o.b.java.security.MessageDigests.getAndDigest             N/A         N/A           SHA-1     16384     DEFAULT  avgt   20    182896.083 ?   1242.774  ns/op
>> o.o.b.javax.crypto.small.MessageDigestBench.digest         SHA1     1048576             N/A       N/A              avgt   20  11599227.792 ? 121442.390  ns/op
>> // Before
>> o.o.b.java.security.MessageDigests.digest                   N/A         N/A           SHA-1        64     DEFAULT  avgt   20      2352.475 ?    11.198  ns/op
>> o.o.b.java.security.MessageDigests.digest                   N/A         N/A           SHA-1     16384     DEFAULT  avgt   20    188495.684 ?  1467.942  ns/op
>> o.o.b.java.security.MessageDigests.getAndDigest             N/A         N/A           SHA-1        64     DEFAULT  avgt   20      2437.347 ?     6.398  ns/op
>> o.o.b.java.security.MessageDigests.getAndDigest             N/A         N/A           SHA-1     16384     DEFAULT  avgt   20    196086.570 ?  1140.998  ns/op
>> o.o.b.javax.crypto.small.MessageDigestBench.digest         SHA1     1048576             N/A       N/A              avgt   20  12362160.119 ? 38788.109  ns/op
>> 
>> 
>> **getAndDigest when size == 64**
>> The data is not stable for test getAndDigest when size == 64, which I think is introduced by  j.s.MessageDigest.getInstance itself, which we don't touch in this patch.
>> Check more details at [1](ht...
>
> Hamlin Li has updated the pull request incrementally with one additional commit since the last revision:
> 
>   not pass in temp register explicitly

Thanks for the update. A few more minor comments.

src/hotspot/cpu/riscv/stubGenerator_riscv.cpp line 4832:

> 4830:   //    M't,                                      0 <=  t <= 15
> 4831:   //    ROTL'1(W't-3 ^ W't-8 ^ W't-14 ^ W't-16),  16 <= t <= 79
> 4832:   void sha1_prepare_w(int round, Register cur_w, Register ws[], Register buf, Register tmp) {

It seems that we could use `t1` in places where `tmp` is used. Maybe we could further remove this `tmp` formal param (and thus frees `t2`)?

src/hotspot/cpu/riscv/stubGenerator_riscv.cpp line 4951:

> 4949:       __ add(cur_w, cur_k, cur_w);
> 4950:       __ add(t, t, cur_w);
> 4951:       __ rolw_imm(cur_w, a, 5, t1);

Since `rolw_imm` uses `t0` as the default temp register, I think a more simpler `__ rolw_imm(cur_w, a, 5);` will do? This also makes `t1` available in places where `tmp` is used. Hopefully, we can free register `t2` and let `src` alias this instead of `x9` thus saving one register saving/restoring respectively on stub entry/exit.

test/hotspot/jtreg/compiler/testlibrary/sha/predicate/IntrinsicPredicates.java line 72:

> 70:     public static final BooleanSupplier SHA1_INSTRUCTION_AVAILABLE
> 71:             = new OrPredicate(new CPUSpecificPredicate("aarch64.*", new String[] { "sha1" }, null),
> 72:               // on riscv64, SHA-1 intrinsic is implemented with basic instructions

How about rephasing this as: `// SHA-1 intrinsic is implemented with scalar instructions on riscv64`?

-------------

PR Review: https://git.openjdk.org/jdk/pull/17130#pullrequestreview-1862634576
PR Review Comment: https://git.openjdk.org/jdk/pull/17130#discussion_r1479949713
PR Review Comment: https://git.openjdk.org/jdk/pull/17130#discussion_r1478182348
PR Review Comment: https://git.openjdk.org/jdk/pull/17130#discussion_r1478138172