RFR: 8321001: RISC-V: C2 SignumVF [v3]

[Hamlin Li]

On Wed, 6 Dec 2023 13:17:11 GMT, Fei Yang <fyang at openjdk.org> wrote:

>> Do you mean some patch like below?
>> 
>> diff --git a/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp b/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp
>> index ed421c9e287..aa82447b943 100644
>> --- a/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp
>> +++ b/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp
>> @@ -1676,15 +1676,14 @@ void C2_MacroAssembler::signum_fp(FloatRegister dst, FloatRegister one, bool is_
>>    bind(done);
>>  }
>>  
>> -void C2_MacroAssembler::signum_fp_v(VectorRegister dst, BasicType bt, int vlen,
>> -                                    VectorRegister zero, VectorRegister one) {
>> +void C2_MacroAssembler::signum_fp_v(VectorRegister dst, BasicType bt, int vlen, VectorRegister one) {
>>    vsetvli_helper(bt, vlen);
>>  
>>    // check if input is -0, +0, signaling NaN or quiet NaN
>>    vfclass_v(v0, dst);
>>    mv(t0, fclass_mask::zero | fclass_mask::nan);
>>    vand_vx(v0, v0, t0);
>> -  vmseq_vv(v0, v0, zero);
>> +  vmseq_vi(v0, v0, 0);
>>  
>>    // use floating-point 1.0 with a sign of input
>>    vfsgnj_vv(dst, one, dst, v0_t);
>> diff --git a/src/hotspot/cpu/riscv/riscv_v.ad b/src/hotspot/cpu/riscv/riscv_v.ad
>> index 9b3c9125b3b..2940655f44e 100644
>> --- a/src/hotspot/cpu/riscv/riscv_v.ad
>> +++ b/src/hotspot/cpu/riscv/riscv_v.ad
>> @@ -3664,15 +3664,15 @@ instruct vexpand(vReg dst, vReg src, vRegMask_V0 v0, vReg tmp) %{
>>  
>>  // Vector Math.signum
>>  
>> -instruct vsignum_reg(vReg dst, vReg zero, vReg one, vRegMask_V0 v0) %{
>> -  match(Set dst (SignumVF dst (Binary zero one)));
>> -  match(Set dst (SignumVD dst (Binary zero one)));
>> +instruct vsignum_reg(vReg dst, vReg one, vRegMask_V0 v0) %{
>> +  match(Set dst (SignumVF dst one));
>> +  match(Set dst (SignumVD dst one));
>>    effect(TEMP_DEF dst, TEMP v0);
>>    format %{ "vsignum $dst, $dst\t" %}
>>    ins_encode %{
>>      BasicType bt = Matcher::vector_element_basic_type(this);
>>      __ signum_fp_v(as_VectorRegister($dst$$reg), bt, Matcher::vector_length(this),
>> -                  as_VectorRegister($zero$$reg), as_VectorRegister($one$$reg));
>> +                   as_VectorRegister($one$$reg));
>>    %}
>>    ins_pipe(pipe_slow);
>>  %}
>> 
>> 
>> In fact this is also my initial patch, but at runtime it will report a `mismatch` issue.
>> And in x86 and aarch64, they all have `zero` too.
>> 
>> 
>> o749  SignumVD  === _ o746 o1276  [[ o750 ]]  #vectora[4]:{double}
>> 
>> --N: o749  SignumVD  === _ o746 o1276  [[ o750 ]]  #vectora[4]:{double}
>> 
>>    --N: o746  LoadVector  === o440 o865 o697  |o250  [[...
>
> Hi, sorry for not being clear enough. In fact, I am suggesting following add-on change:
> 
> diff --git a/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp b/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp
> index 18b5df3f68c..b732e70ed84 100644
> --- a/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp
> +++ b/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp
> @@ -1677,15 +1677,15 @@ void C2_MacroAssembler::signum_fp(FloatRegister dst, FloatRegister one, bool is_
>    bind(done);
>  }
> 
> -void C2_MacroAssembler::signum_fp_v(VectorRegister dst, BasicType bt, int vlen,
> -                                    VectorRegister zero, VectorRegister one) {
> +void C2_MacroAssembler::signum_fp_v(VectorRegister dst, VectorRegister one,
> +                                    BasicType bt, int vlen) {
>    vsetvli_helper(bt, vlen);
> 
>    // check if input is -0, +0, signaling NaN or quiet NaN
>    vfclass_v(v0, dst);
>    mv(t0, fclass_mask::zero | fclass_mask::nan);
>    vand_vx(v0, v0, t0);
> -  vmseq_vv(v0, v0, zero);
> +  vmseq_vi(v0, v0, 0);
> 
>    // use floating-point 1.0 with a sign of input
>    vfsgnj_vv(dst, one, dst, v0_t);
> diff --git a/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.hpp b/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.hpp
> index 0ed7c3686dc..b9a7749631a 100644
> --- a/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.hpp
> +++ b/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.hpp
> @@ -163,7 +163,7 @@
> 
>    void signum_fp(FloatRegister dst, FloatRegister one, bool is_double);
> 
> -  void signum_fp_v(VectorRegister dst, BasicType bt, int vlen, VectorRegister zero, VectorRegister one);
> +  void signum_fp_v(VectorRegister dst, VectorRegister one, BasicType bt, int vlen);
> 
>    // intrinsic methods implemented by rvv instructions
> 
> diff --git a/src/hotspot/cpu/riscv/riscv_v.ad b/src/hotspot/cpu/riscv/riscv_v.ad
> index 9c2349a6c92..c163325fc81 100644
> --- a/src/hotspot/cpu/riscv/riscv_v.ad
> +++ b/src/hotspot/cpu/riscv/riscv_v.ad
> @@ -3671,8 +3671,8 @@ instruct vsignum_reg(vReg dst, vReg zero, vReg one, vRegMask_V0 v0) %{
>    format %{ "vsignum $dst, $dst\t" %}
>    ins_encode %{
>      BasicType bt = Matcher::vector_element_basic_type(this);
> -    __ signum_fp_v(as_VectorRegister($dst$$reg), bt, Matcher::vector_length(this),
> -                   as_VectorRegister($zero$$reg), as_VectorRegister($one$$reg));
> +    __ signum_fp_v(as_VectorRegister($dst$$reg), as_VectorRegister($one$$reg),
> +                   bt, Matcher::vector_length(this));
>    %}
>    ins_pipe(pipe_slow);
>  %}

Good catch of the bug! Fixed.
I did not realize it before. I think the reason that tests did not catch it is because both int and float zero has the exact same binary content.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16925#discussion_r1417432947