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

[Fei Yang]

On Wed, 6 Dec 2023 11:33:40 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 1687:
>> 
>>> 1685:   mv(t0, fclass_mask::zero | fclass_mask::nan);
>>> 1686:   vand_vx(v0, v0, t0);
>>> 1687:   vmseq_vv(v0, v0, zero);
>> 
>> I don't think that the input `zero` (a vector of floating-point 0.0) is appropriate here for `vmseq_vv` which does vector integer comparison. Why not do `vmseq_vi(v0, v0, 0)` instead? This will also help remove the `zero` parameter of this function.
>
> 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  [[ o749 ]]  @double[int:>=0] (java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=6; mismatched #vectora[...

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);
 %}

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

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