RFR: 8315716: RISC-V: implement ChaCha20 intrinsic [v2]

[Fei Yang]

On Thu, 28 Sep 2023 16:54:14 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> Only vector version is included in this patch.
>> 
>> ### Test
>> The patch passed the jdk tests found via `find test/jdk/ -iname *ChaCha*`
>
> Hamlin Li has updated the pull request incrementally with one additional commit since the last revision:
> 
>   revert adding t3-t6

Hi, some comments from a brief look. Thanks.

src/hotspot/cpu/riscv/macroAssembler_riscv.hpp line 1293:

> 1291:   // vector pseudo instructions
> 1292:   // rotate vector register left with shift bits, 32-bit version
> 1293:   inline void vrol_vwi(VectorRegister vd, uint32_t shift, VectorRegister tmp_vr) {

Since this is not a narrowing/widening vector opertation, I don't think it's appropriate to use the `_vwi` suffix according to the naming convension of the RVV spec. Maybe rename this as `vrole32_vi` ? Also note that support for vector rotate left/right has been added by RISC-V cryptography extensions which have been ratified recently.

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

> 4285:     __ vrol_vwi(dVec, 16, tmp_vr);
> 4286: 
> 4287:     // rev32(dVec, T8H, dVec);

Irrelevant comment?

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

> 4310:    *  c_rarg1   - key_stream, the array that will hold the result of the ChaCha20 block function
> 4311:    */
> 4312:   address generate_chacha20Block() {

I think we should add some more comments to help understand the code.
The code comments in the aarch64 counterpart might be a good reference: https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp#L4244

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

> 4316:     StubCodeMark mark(this, "StubRoutines", "chacha20Block");
> 4317:     address start = __ pc();
> 4318: 

Should we start a new frame with `enter()` on entry and `leave()` on exit for proper stackwalking of RuntimeStub frame?

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

> 4322:     const Register key_stream = c_rarg1;
> 4323:     const Register length = t0;
> 4324:     const Register tmp_addr = t1;

`t0` as scratch register are frequently and sometimes implicitly used/clobbered by various assembler functions. So it's not a good idea to let `length` alias 't0' as it is live across this stub. Maybe swap `t0` and `t1` for `length` and `tmp_addr` respectively.

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

> 4326:     const VectorRegister work_vrs[16] = {
> 4327:       v4,  v5,  v6,  v7,  v16, v17, v18, v19,
> 4328:       v20, v21, v22, v23, v24, v25, v26, v27

Is there a rule on how those vector registers are chosen here? Can we simply start from `v0`?

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

> 4336:       // in java level.
> 4337:       __ mv(avl, 16);
> 4338:       __ vsetvli(length, avl, Assembler::e32, Assembler::m1);

I think we can make use of `vsetivli` instruction to save the use of register `t2` here.
RVV Spec: `For the vsetivli instruction, the AVL is encoded as a 5-bit zero-extended immediate (0 - 31) in the rs1 field`

src/hotspot/cpu/riscv/vm_version_riscv.cpp line 257:

> 255:   } else if (UseChaCha20Intrinsics) {
> 256:     if (!FLAG_IS_DEFAULT(UseChaCha20Intrinsics)) {
> 257:       warning("Chacha20 Intrinsics requires RVV instructions (not available on this CPU)");

Suggestion: s/Intrinsics/intrinsic/

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

PR Review: https://git.openjdk.org/jdk/pull/15899#pullrequestreview-1650751154
PR Review Comment: https://git.openjdk.org/jdk/pull/15899#discussion_r1342042824
PR Review Comment: https://git.openjdk.org/jdk/pull/15899#discussion_r1342131300
PR Review Comment: https://git.openjdk.org/jdk/pull/15899#discussion_r1349837665
PR Review Comment: https://git.openjdk.org/jdk/pull/15899#discussion_r1342129509
PR Review Comment: https://git.openjdk.org/jdk/pull/15899#discussion_r1349836272
PR Review Comment: https://git.openjdk.org/jdk/pull/15899#discussion_r1342130428
PR Review Comment: https://git.openjdk.org/jdk/pull/15899#discussion_r1342043706
PR Review Comment: https://git.openjdk.org/jdk/pull/15899#discussion_r1341347405