RFR: 8320999: RISC-V: C2 RotateLeftV [v4]

[Hamlin Li]

On Wed, 29 May 2024 07:40:11 GMT, Fei Yang <fyang at openjdk.org> wrote:

> Two minor comments remain. Otherwise looks good to me. BTW: You didn't mention the testing performed. Are these newly-added instructs properly test covered? Thanks.

Yes, I've checked the instructs are matched and invoked during tests running.

> src/hotspot/cpu/riscv/riscv_v.ad line 3082:
> 
>> 3080: //
>> 3081: // NOTE: for Long, its valid rotation value is 6 bits, although basic vector instruction only support 5 bit vector-immediate,
>> 3082: //       in Zvbb, vror.vi support 6 bits vector-immediate, so the imm implementation of Long and other types can be unified.
> 
> Maybe simply: `As vror.vi encodes 6-bits immediate rotate amount, which is different from other vector-immediate instructions, implementation of vector rotation for long and other types can be unified.`

modified

> src/hotspot/cpu/riscv/riscv_v.ad line 3130:
> 
>> 3128: instruct vrotate_right_masked(vReg dst_src, vReg shift, vRegMask_V0 v0) %{
>> 3129:   match(Set dst_src (RotateRightV (Binary dst_src shift) v0));
>> 3130:   effect(TEMP_DEF dst_src);
> 
> Is the `TEMP_DEF dst_src` needed for these newly-added masked versions?

Thanks for catching, removed.

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

PR Comment: https://git.openjdk.org/jdk/pull/19325#issuecomment-2136952218
PR Review Comment: https://git.openjdk.org/jdk/pull/19325#discussion_r1618547353
PR Review Comment: https://git.openjdk.org/jdk/pull/19325#discussion_r1618546816