RFR: 8302908: RISC-V: Support masked vector arithmetic instructions for Vector API [v23]

[Gui Cao]

On Thu, 20 Apr 2023 14:20:43 GMT, Fei Yang <fyang at openjdk.org> wrote:

>> Dingli Zhang has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Rename vmaskcmp_DF
>
> src/hotspot/cpu/riscv/riscv_v.ad line 199:
> 
>> 197: // vector mask float compare
>> 198: 
>> 199: instruct vmaskcmp_FD(vRegMask dst, vReg src1, vReg src2, immI cond, vReg tmp1, vReg tmp2) %{
> 
> I would prefer renaming this into something like 'vmaskcmp_fp'. Also, would you mind renaming macro-assember functions minmax_FD, minmax_FD_v and reduce_minmax_FD_v into something like minmax_fp, minmax_fp_v, reduce_minmax_fp_v respectively? Thanks.

Fixed.

> src/hotspot/cpu/riscv/riscv_v.ad line 216:
> 
>> 214: %}
>> 215: 
>> 216: instruct vmaskcmp_FD_masked(vRegMask dst, vReg src1, vReg src2, immI cond, vRegMask_V0 vmask, vReg tmp1, vReg tmp2, vReg tmp3) %{
> 
> Similar here, we can rename this into something like 'vmaskcmp_fp_masked'.

Fixed.

> src/hotspot/cpu/riscv/riscv_v.ad line 413:
> 
>> 411: %}
>> 412: 
>> 413: instruct vaddF_masked(vReg dst_src1, vReg src2, vRegMask_V0 vmask) %{
> 
> Can we combine this one with the next 'vaddD_masked' into a new instruct and give it a new name like 'vadd_fp_masked'? It looks to me that we can do similar thing for other instructs like 'vmulF_masked/vmulD_masked', etc.

Fixed.

> src/hotspot/cpu/riscv/riscv_v.ad line 1491:
> 
>> 1489: // vector shift
>> 1490: 
>> 1491: instruct vasrBS(vReg dst, vReg src, vReg shift, vRegMask_V0 tmp) %{
> 
> I really don't like instruct names like 'vasrBS' and 'vasrIL' which looks kind of misleading. I would perfer keep those seperated like before.

Fixed.

> src/hotspot/cpu/riscv/riscv_v.ad line 2393:
> 
>> 2391:   format %{ "vmask_gen_I $dst, $src" %}
>> 2392:   ins_encode %{
>> 2393:     __ clear_register_v(as_VectorRegister($dst$$reg));
> 
> Could you explain why we need to clear 'dst' here? And can we use vector mask-register logical instruction 'vmclr.m' instead if it is really needed?

It was indeed redundant here and has been removed.

> src/hotspot/cpu/riscv/riscv_v.ad line 2406:
> 
>> 2404:     __ clear_register_v(as_VectorRegister($dst$$reg));
>> 2405:     __ vsetvli(t0, $src$$Register, Assembler::e8);
>> 2406:     __ vmxnor_mm(as_VectorRegister($dst$$reg), as_VectorRegister($dst$$reg), as_VectorRegister($dst$$reg));
> 
> We could introduce assembler pseudoinstructions 'vmmv.m', 'vmclr.m', 'vmset.m' and 'vmnot.m' from the spec to make the vector mask computation more readable.
> 
> vmmv.m vd, vs => vmand.mm vd, vs, vs # Copy mask register
> vmclr.m vd => vmxor.mm vd, vd, vd # Clear mask register
> vmset.m vd => vmxnor.mm vd, vd, vd # Set mask register
> vmnot.m vd, vs => vmnand.mm vd, vs, vs # Invert bits

Fixed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/12682#discussion_r1173762016
PR Review Comment: https://git.openjdk.org/jdk/pull/12682#discussion_r1173762191
PR Review Comment: https://git.openjdk.org/jdk/pull/12682#discussion_r1173762356
PR Review Comment: https://git.openjdk.org/jdk/pull/12682#discussion_r1173762484
PR Review Comment: https://git.openjdk.org/jdk/pull/12682#discussion_r1173763532
PR Review Comment: https://git.openjdk.org/jdk/pull/12682#discussion_r1173763835