diff mbox series

[1/5] RISC-V: Remove float vector eqne pattern

Message ID 20240301062711.207137-2-demin.han@starfivetech.com
State Deferred
Delegated to: Robin Dapp
Headers
Series RISC-V: Support vf and vx for autovec comparison of |

Checks

Context Check Description
rivoscibot/toolchain-ci-rivos-lint success Lint passed
rivoscibot/toolchain-ci-rivos-apply-patch success Patch applied
rivoscibot/toolchain-ci-rivos-build--newlib-rv64gc-lp64d-multilib success Build passed
rivoscibot/toolchain-ci-rivos-build--linux-rv32gc_zba_zbb_zbc_zbs-ilp32d-non-multilib success Build passed
rivoscibot/toolchain-ci-rivos-build--linux-rv64gc_zba_zbb_zbc_zbs-lp64d-non-multilib success Build passed
rivoscibot/toolchain-ci-rivos-build--newlib-rv64gcv-lp64d-multilib success Build passed
rivoscibot/toolchain-ci-rivos-build--linux-rv64gcv-lp64d-multilib success Build passed
rivoscibot/toolchain-ci-rivos-test success Testing passed
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Testing passed

Commit Message

demin.han March 1, 2024, 6:27 a.m. UTC 
  We can unify eqne and other comparison operations.

Tested on RV32 and RV64

gcc/ChangeLog:

	* config/riscv/riscv-vector-builtins-bases.cc: Remove eqne cond
	* config/riscv/vector.md (@pred_eqne<mode>_scalar): Remove patterns
	(*pred_eqne<mode>_scalar_merge_tie_mask): Ditto
	(*pred_eqne<mode>_scalar): Ditto
	(*pred_eqne<mode>_scalar_narrow): Ditto

Signed-off-by: demin.han <demin.han@starfivetech.com>
---
 .../riscv/riscv-vector-builtins-bases.cc      |  4 -
 gcc/config/riscv/vector.md                    | 86 -------------------
 2 files changed, 90 deletions(-)

Comments

juzhe.zhong@rivai.ai March 1, 2024, 7:23 a.m. UTC | #1 
Hello, han.&nbsp; Thanks for trying to optimize the codes.


But I believe those vector-scalar patterns (eq/ne) you remove in this patch are necessary.


This is the story:
1. For commutative RTL code in GCC like plus, eq, ne, ... etc,
&nbsp; &nbsp; we known in semantic Both (eq: (reg) (vec_duplicate ... ) and (eq: (vec_duplicate ...&nbsp;) (reg)) are right.
&nbsp; &nbsp; However, GCC prefer this order as I remembered - (eq: (vec_duplicate ...&nbsp;) (reg)).


2. Before this patch, the order of the comparison as follows (take eq and lt as an example):
&nbsp; &nbsp;&nbsp;
&nbsp; &nbsp; 1). (eq: (vec_duplicate ...&nbsp;) (reg))&nbsp; --&gt; commutative
&nbsp; &nbsp; 2). (lt: (reg) (vec_duplicate ... )&nbsp; &nbsp; &nbsp;--&gt; non-commutative
&nbsp; &nbsp;&nbsp;
&nbsp; &nbsp;These patterns order are different.
&nbsp; &nbsp;
&nbsp; &nbsp;So, you see we have dedicated patterns (seems duplicate patterns) for vector-scalar eq/ne, whereas, we unify eq/ne into other comparisons for vector-vector instructions.
&nbsp; &nbsp;If we unify eq/ne into other comparisons for vector-scalar instructions (like your patch does), we will end up have:
&nbsp; &nbsp;&nbsp;
&nbsp; &nbsp;(eq: (reg) (vec_duplicate ... ) [after this patch] instead of (eq: (vec_duplicate ...&nbsp;) (reg)) [Before this patch].


So, I think this patch may not be right.
I may be wrong, Robin/Jerff/kito feel free to correct me if I am wrong.
&nbsp;
&nbsp;
------------------&nbsp;Original&nbsp;------------------
From: &nbsp;"demin.han"<demin.han@starfivetech.com&gt;;
Date: &nbsp;Fri, Mar 1, 2024 02:27 PM
To: &nbsp;"gcc-patches"<gcc-patches@gcc.gnu.org&gt;; 
Cc: &nbsp;"juzhe.zhong"<juzhe.zhong@rivai.ai&gt;; "kito.cheng"<kito.cheng@gmail.com&gt;; "Li, Pan2"<pan2.li@intel.com&gt;; "jeffreyalaw"<jeffreyalaw@gmail.com&gt;; 
Subject: &nbsp;[PATCH 1/5] RISC-V: Remove float vector eqne pattern

&nbsp;

We can unify eqne and other comparison operations.

Tested on RV32 and RV64

gcc/ChangeLog:

	* config/riscv/riscv-vector-builtins-bases.cc: Remove eqne cond
	* config/riscv/vector.md (@pred_eqne<mode&gt;_scalar): Remove patterns
	(*pred_eqne<mode&gt;_scalar_merge_tie_mask): Ditto
	(*pred_eqne<mode&gt;_scalar): Ditto
	(*pred_eqne<mode&gt;_scalar_narrow): Ditto

Signed-off-by: demin.han <demin.han@starfivetech.com&gt;
---
&nbsp;.../riscv/riscv-vector-builtins-bases.cc&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; |&nbsp; 4 -
&nbsp;gcc/config/riscv/vector.md&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; | 86 -------------------
&nbsp;2 files changed, 90 deletions(-)

diff --git a/gcc/config/riscv/riscv-vector-builtins-bases.cc b/gcc/config/riscv/riscv-vector-builtins-bases.cc
index b6f6e4ff37e..d414721ede8 100644
--- a/gcc/config/riscv/riscv-vector-builtins-bases.cc
+++ b/gcc/config/riscv/riscv-vector-builtins-bases.cc
@@ -1420,10 +1420,6 @@ public:
&nbsp;&nbsp;&nbsp;&nbsp; switch (e.op_info-&gt;op)
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; {
&nbsp;	case OP_TYPE_vf: {
-	&nbsp; if (CODE == EQ || CODE == NE)
-	&nbsp;&nbsp;&nbsp; return e.use_compare_insn (CODE, code_for_pred_eqne_scalar (
-					&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; e.vector_mode ()));
-	&nbsp; else
&nbsp;	&nbsp;&nbsp;&nbsp; return e.use_compare_insn (CODE, code_for_pred_cmp_scalar (
&nbsp;					&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; e.vector_mode ()));
&nbsp;	}
diff --git a/gcc/config/riscv/vector.md b/gcc/config/riscv/vector.md
index ab6e099852d..9210d7c28ad 100644
--- a/gcc/config/riscv/vector.md
+++ b/gcc/config/riscv/vector.md
@@ -7520,92 +7520,6 @@ (define_insn "*pred_cmp<mode&gt;_scalar_narrow"
&nbsp;&nbsp;&nbsp; (set_attr "mode" "<MODE&gt;")
&nbsp;&nbsp;&nbsp; (set_attr "spec_restriction" "none,thv,thv,none,none")])
&nbsp;
-(define_expand "@pred_eqne<mode&gt;_scalar"
-&nbsp; [(set (match_operand:<VM&gt; 0 "register_operand")
-	(if_then_else:<VM&gt;
-	&nbsp; (unspec:<VM&gt;
-	&nbsp;&nbsp;&nbsp; [(match_operand:<VM&gt; 1 "vector_mask_operand")
-	&nbsp;&nbsp;&nbsp;&nbsp; (match_operand 6 "vector_length_operand")
-	&nbsp;&nbsp;&nbsp;&nbsp; (match_operand 7 "const_int_operand")
-	&nbsp;&nbsp;&nbsp;&nbsp; (match_operand 8 "const_int_operand")
-	&nbsp;&nbsp;&nbsp;&nbsp; (reg:SI VL_REGNUM)
-	&nbsp;&nbsp;&nbsp;&nbsp; (reg:SI VTYPE_REGNUM)] UNSPEC_VPREDICATE)
-	&nbsp; (match_operator:<VM&gt; 3 "equality_operator"
-	&nbsp;&nbsp;&nbsp;&nbsp; [(vec_duplicate:V_VLSF
-	&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; (match_operand:<VEL&gt; 5 "register_operand"))
-	&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; (match_operand:V_VLSF 4 "register_operand")])
-	&nbsp; (match_operand:<VM&gt; 2 "vector_merge_operand")))]
-&nbsp; "TARGET_VECTOR"
-&nbsp; {})
-
-(define_insn "*pred_eqne<mode&gt;_scalar_merge_tie_mask"
-&nbsp; [(set (match_operand:<VM&gt; 0 "register_operand"&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; "=vm")
-	(if_then_else:<VM&gt;
-	&nbsp; (unspec:<VM&gt;
-	&nbsp;&nbsp;&nbsp; [(match_operand:<VM&gt; 1 "register_operand"&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; "&nbsp; 0")
-	&nbsp;&nbsp;&nbsp;&nbsp; (match_operand 5 "vector_length_operand"&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; " rK")
-	&nbsp;&nbsp;&nbsp;&nbsp; (match_operand 6 "const_int_operand"&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; "&nbsp; i")
-	&nbsp;&nbsp;&nbsp;&nbsp; (match_operand 7 "const_int_operand"&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; "&nbsp; i")
-	&nbsp;&nbsp;&nbsp;&nbsp; (reg:SI VL_REGNUM)
-	&nbsp;&nbsp;&nbsp;&nbsp; (reg:SI VTYPE_REGNUM)] UNSPEC_VPREDICATE)
-	&nbsp; (match_operator:<VM&gt; 2 "equality_operator"
-	&nbsp;&nbsp;&nbsp;&nbsp; [(vec_duplicate:V_VLSF
-	&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; (match_operand:<VEL&gt; 4 "register_operand"&nbsp;&nbsp;&nbsp;&nbsp; "&nbsp; f"))
-	&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; (match_operand:V_VLSF 3 "register_operand"&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; " vr")])
-	&nbsp; (match_dup 1)))]
-&nbsp; "TARGET_VECTOR"
-&nbsp; "vmf%B2.vf\t%0,%3,%4,v0.t"
-&nbsp; [(set_attr "type" "vfcmp")
-&nbsp;&nbsp; (set_attr "mode" "<MODE&gt;")
-&nbsp;&nbsp; (set_attr "merge_op_idx" "1")
-&nbsp;&nbsp; (set_attr "vl_op_idx" "5")
-&nbsp;&nbsp; (set (attr "ma") (symbol_ref "riscv_vector::get_ma(operands[6])"))
-&nbsp;&nbsp; (set (attr "avl_type_idx") (const_int 7))])
-
-;; We don't use early-clobber for LMUL <= 1 to get better codegen.
-(define_insn "*pred_eqne<mode&gt;_scalar"
-&nbsp; [(set (match_operand:<VM&gt; 0 "register_operand"&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; "=vr,&nbsp;&nbsp; vr,&nbsp;&nbsp; &amp;vr,&nbsp;&nbsp; &amp;vr")
-	(if_then_else:<VM&gt;
-	&nbsp; (unspec:<VM&gt;
-	&nbsp;&nbsp;&nbsp; [(match_operand:<VM&gt; 1 "vector_mask_operand"&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; "vmWc1,vmWc1,vmWc1,vmWc1")
-	&nbsp;&nbsp;&nbsp;&nbsp; (match_operand 6 "vector_length_operand"&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; "&nbsp;&nbsp; rK,&nbsp;&nbsp; rK,&nbsp;&nbsp; rK,&nbsp;&nbsp; rK")
-	&nbsp;&nbsp;&nbsp;&nbsp; (match_operand 7 "const_int_operand"&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; "&nbsp;&nbsp;&nbsp; i,&nbsp;&nbsp;&nbsp; i,&nbsp;&nbsp;&nbsp; i,&nbsp;&nbsp;&nbsp; i")
-	&nbsp;&nbsp;&nbsp;&nbsp; (match_operand 8 "const_int_operand"&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; "&nbsp;&nbsp;&nbsp; i,&nbsp;&nbsp;&nbsp; i,&nbsp;&nbsp;&nbsp; i,&nbsp;&nbsp;&nbsp; i")
-	&nbsp;&nbsp;&nbsp;&nbsp; (reg:SI VL_REGNUM)
-	&nbsp;&nbsp;&nbsp;&nbsp; (reg:SI VTYPE_REGNUM)] UNSPEC_VPREDICATE)
-	&nbsp; (match_operator:<VM&gt; 3 "equality_operator"
-	&nbsp;&nbsp;&nbsp;&nbsp; [(vec_duplicate:V_VLSF
-	&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; (match_operand:<VEL&gt; 5 "register_operand"&nbsp;&nbsp;&nbsp;&nbsp; "&nbsp;&nbsp;&nbsp; f,&nbsp;&nbsp;&nbsp; f,&nbsp;&nbsp;&nbsp; f,&nbsp;&nbsp;&nbsp; f"))
-	&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; (match_operand:V_VLSF 4 "register_operand"&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; "&nbsp;&nbsp; vr,&nbsp;&nbsp; vr,&nbsp;&nbsp; vr,&nbsp;&nbsp; vr")])
-	&nbsp; (match_operand:<VM&gt; 2 "vector_merge_operand"&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; "&nbsp;&nbsp; vu,&nbsp;&nbsp;&nbsp; 0,&nbsp;&nbsp;&nbsp; vu,&nbsp;&nbsp;&nbsp; 0")))]
-&nbsp; "TARGET_VECTOR &amp;&amp; riscv_vector::cmp_lmul_le_one (<MODE&gt;mode)"
-&nbsp; "vmf%B3.vf\t%0,%4,%5%p1"
-&nbsp; [(set_attr "type" "vfcmp")
-&nbsp;&nbsp; (set_attr "mode" "<MODE&gt;")
-&nbsp;&nbsp; (set_attr "spec_restriction" "thv,thv,rvv,rvv")])
-
-;; We use early-clobber for source LMUL &gt; dest LMUL.
-(define_insn "*pred_eqne<mode&gt;_scalar_narrow"
-&nbsp; [(set (match_operand:<VM&gt; 0 "register_operand"&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; "=vm,&nbsp;&nbsp; vr,&nbsp;&nbsp; vr,&nbsp; &amp;vr,&nbsp; &amp;vr")
-	(if_then_else:<VM&gt;
-	&nbsp; (unspec:<VM&gt;
-	&nbsp;&nbsp;&nbsp; [(match_operand:<VM&gt; 1 "vector_mask_operand"&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; "&nbsp;&nbsp;&nbsp; 0,vmWc1,vmWc1,vmWc1,vmWc1")
-	&nbsp;&nbsp;&nbsp;&nbsp; (match_operand 6 "vector_length_operand"&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; "&nbsp;&nbsp; rK,&nbsp;&nbsp; rK,&nbsp;&nbsp; rK,&nbsp;&nbsp; rK,&nbsp;&nbsp; rK")
-	&nbsp;&nbsp;&nbsp;&nbsp; (match_operand 7 "const_int_operand"&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; "&nbsp;&nbsp;&nbsp; i,&nbsp;&nbsp;&nbsp; i,&nbsp;&nbsp;&nbsp; i,&nbsp;&nbsp;&nbsp; i,&nbsp;&nbsp;&nbsp; i")
-	&nbsp;&nbsp;&nbsp;&nbsp; (match_operand 8 "const_int_operand"&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; "&nbsp;&nbsp;&nbsp; i,&nbsp;&nbsp;&nbsp; i,&nbsp;&nbsp;&nbsp; i,&nbsp;&nbsp;&nbsp; i,&nbsp;&nbsp;&nbsp; i")
-	&nbsp;&nbsp;&nbsp;&nbsp; (reg:SI VL_REGNUM)
-	&nbsp;&nbsp;&nbsp;&nbsp; (reg:SI VTYPE_REGNUM)] UNSPEC_VPREDICATE)
-	&nbsp; (match_operator:<VM&gt; 3 "equality_operator"
-	&nbsp;&nbsp;&nbsp;&nbsp; [(vec_duplicate:V_VLSF
-	&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; (match_operand:<VEL&gt; 5 "register_operand"&nbsp;&nbsp;&nbsp;&nbsp; "&nbsp;&nbsp;&nbsp; f,&nbsp;&nbsp;&nbsp; f,&nbsp;&nbsp;&nbsp; f,&nbsp;&nbsp;&nbsp; f,&nbsp;&nbsp;&nbsp; f"))
-	&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; (match_operand:V_VLSF 4 "register_operand"&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; "&nbsp;&nbsp; vr,&nbsp;&nbsp;&nbsp; 0,&nbsp;&nbsp;&nbsp; 0,&nbsp;&nbsp; vr,&nbsp;&nbsp; vr")])
-	&nbsp; (match_operand:<VM&gt; 2 "vector_merge_operand"&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; "&nbsp;&nbsp; vu,&nbsp;&nbsp; vu,&nbsp;&nbsp;&nbsp; 0,&nbsp;&nbsp; vu,&nbsp;&nbsp;&nbsp; 0")))]
-&nbsp; "TARGET_VECTOR &amp;&amp; riscv_vector::cmp_lmul_gt_one (<MODE&gt;mode)"
-&nbsp; "vmf%B3.vf\t%0,%4,%5%p1"
-&nbsp; [(set_attr "type" "vfcmp")
-&nbsp;&nbsp; (set_attr "mode" "<MODE&gt;")
-&nbsp;&nbsp; (set_attr "spec_restriction" "none,thv,thv,none,none")])
-
&nbsp;;; -------------------------------------------------------------------------------
&nbsp;;; ---- Predicated floating-point merge
&nbsp;;; -------------------------------------------------------------------------------
-- 
2.43.2
demin.han March 1, 2024, 8:12 a.m. UTC | #2 
Hi juzhe,

I also thought it’s related to commutive firstly.

Following things make me to do the removal:

1.      No tests fails in regression

2.      When I write if (a == 2) and if (2 == a), the results are same

3.      The vec_duplicate operand  is the 5th operand in both cmp and eqne patterns. I think they are equal.


From: 钟居哲 <juzhe.zhong@rivai.ai>
Sent: 2024年3月1日 15:24
To: Demin Han <demin.han@starfivetech.com>; gcc-patches <gcc-patches@gcc.gnu.org>
Cc: kito.cheng <kito.cheng@gmail.com>; Li, Pan2 <pan2.li@intel.com>; jeffreyalaw <jeffreyalaw@gmail.com>; Robin Dapp <rdapp.gcc@gmail.com>
Subject: Re:[PATCH 1/5] RISC-V: Remove float vector eqne pattern

Hello, han.  Thanks for trying to optimize the codes.

But I believe those vector-scalar patterns (eq/ne) you remove in this patch are necessary.

This is the story:
1. For commutative RTL code in GCC like plus, eq, ne, ... etc,
    we known in semantic Both (eq: (reg) (vec_duplicate ... ) and (eq: (vec_duplicate ... ) (reg)) are right.
    However, GCC prefer this order as I remembered - (eq: (vec_duplicate ... ) (reg)).

2. Before this patch, the order of the comparison as follows (take eq and lt as an example):

    1). (eq: (vec_duplicate ... ) (reg))  --> commutative
    2). (lt: (reg) (vec_duplicate ... )     --> non-commutative

   These patterns order are different.

   So, you see we have dedicated patterns (seems duplicate patterns) for vector-scalar eq/ne, whereas, we unify eq/ne into other comparisons for vector-vector instructions.
   If we unify eq/ne into other comparisons for vector-scalar instructions (like your patch does), we will end up have:

   (eq: (reg) (vec_duplicate ... ) [after this patch] instead of (eq: (vec_duplicate ... ) (reg)) [Before this patch].

So, I think this patch may not be right.
I may be wrong, Robin/Jerff/kito feel free to correct me if I am wrong.


------------------ Original ------------------
From:  "demin.han"<demin.han@starfivetech.com<mailto:demin.han@starfivetech.com>>;
Date:  Fri, Mar 1, 2024 02:27 PM
To:  "gcc-patches"<gcc-patches@gcc.gnu.org<mailto:gcc-patches@gcc.gnu.org>>;
Cc:  "juzhe.zhong"<juzhe.zhong@rivai.ai<mailto:juzhe.zhong@rivai.ai>>; "kito.cheng"<kito.cheng@gmail.com<mailto:kito.cheng@gmail.com>>; "Li, Pan2"<pan2.li@intel.com<mailto:pan2.li@intel.com>>; "jeffreyalaw"<jeffreyalaw@gmail.com<mailto:jeffreyalaw@gmail.com>>;
Subject:  [PATCH 1/5] RISC-V: Remove float vector eqne pattern

We can unify eqne and other comparison operations.

Tested on RV32 and RV64

gcc/ChangeLog:

* config/riscv/riscv-vector-builtins-bases.cc: Remove eqne cond
* config/riscv/vector.md (@pred_eqne<mode>_scalar): Remove patterns
(*pred_eqne<mode>_scalar_merge_tie_mask): Ditto
(*pred_eqne<mode>_scalar): Ditto
(*pred_eqne<mode>_scalar_narrow): Ditto

Signed-off-by: demin.han <demin.han@starfivetech.com<mailto:demin.han@starfivetech.com>>
---
 .../riscv/riscv-vector-builtins-bases.cc      |  4 -
 gcc/config/riscv/vector.md                    | 86 -------------------
 2 files changed, 90 deletions(-)

diff --git a/gcc/config/riscv/riscv-vector-builtins-bases.cc b/gcc/config/riscv/riscv-vector-builtins-bases.cc
index b6f6e4ff37e..d414721ede8 100644
--- a/gcc/config/riscv/riscv-vector-builtins-bases.cc
+++ b/gcc/config/riscv/riscv-vector-builtins-bases.cc
@@ -1420,10 +1420,6 @@ public:
     switch (e.op_info->op)
       {
  case OP_TYPE_vf: {
-   if (CODE == EQ || CODE == NE)
-     return e.use_compare_insn (CODE, code_for_pred_eqne_scalar (
-        e.vector_mode ()));
-   else
      return e.use_compare_insn (CODE, code_for_pred_cmp_scalar (
         e.vector_mode ()));
  }
diff --git a/gcc/config/riscv/vector.md b/gcc/config/riscv/vector.md
index ab6e099852d..9210d7c28ad 100644
--- a/gcc/config/riscv/vector.md
+++ b/gcc/config/riscv/vector.md
@@ -7520,92 +7520,6 @@ (define_insn "*pred_cmp<mode>_scalar_narrow"
    (set_attr "mode" "<MODE>")
    (set_attr "spec_restriction" "none,thv,thv,none,none")])

-(define_expand "@pred_eqne<mode>_scalar"
-  [(set (match_operand:<VM> 0 "register_operand")
- (if_then_else:<VM>
-   (unspec:<VM>
-     [(match_operand:<VM> 1 "vector_mask_operand")
-      (match_operand 6 "vector_length_operand")
-      (match_operand 7 "const_int_operand")
-      (match_operand 8 "const_int_operand")
-      (reg:SI VL_REGNUM)
-      (reg:SI VTYPE_REGNUM)] UNSPEC_VPREDICATE)
-   (match_operator:<VM> 3 "equality_operator"
-      [(vec_duplicate:V_VLSF
-         (match_operand:<VEL> 5 "register_operand"))
-       (match_operand:V_VLSF 4 "register_operand")])
-   (match_operand:<VM> 2 "vector_merge_operand")))]
-  "TARGET_VECTOR"
-  {})
-
-(define_insn "*pred_eqne<mode>_scalar_merge_tie_mask"
-  [(set (match_operand:<VM> 0 "register_operand"              "=vm")
- (if_then_else:<VM>
-   (unspec:<VM>
-     [(match_operand:<VM> 1 "register_operand"         "  0")
-      (match_operand 5 "vector_length_operand"         " rK")
-      (match_operand 6 "const_int_operand"             "  i")
-      (match_operand 7 "const_int_operand"             "  i")
-      (reg:SI VL_REGNUM)
-      (reg:SI VTYPE_REGNUM)] UNSPEC_VPREDICATE)
-   (match_operator:<VM> 2 "equality_operator"
-      [(vec_duplicate:V_VLSF
-         (match_operand:<VEL> 4 "register_operand"     "  f"))
-       (match_operand:V_VLSF 3 "register_operand"      " vr")])
-   (match_dup 1)))]
-  "TARGET_VECTOR"
-  "vmf%B2.vf\t%0,%3,%4,v0.t"
-  [(set_attr "type" "vfcmp")
-   (set_attr "mode" "<MODE>")
-   (set_attr "merge_op_idx" "1")
-   (set_attr "vl_op_idx" "5")
-   (set (attr "ma") (symbol_ref "riscv_vector::get_ma(operands[6])"))
-   (set (attr "avl_type_idx") (const_int 7))])
-
-;; We don't use early-clobber for LMUL <= 1 to get better codegen.
-(define_insn "*pred_eqne<mode>_scalar"
-  [(set (match_operand:<VM> 0 "register_operand"                "=vr,   vr,   &vr,   &vr")
- (if_then_else:<VM>
-   (unspec:<VM>
-     [(match_operand:<VM> 1 "vector_mask_operand"      "vmWc1,vmWc1,vmWc1,vmWc1")
-      (match_operand 6 "vector_length_operand"         "   rK,   rK,   rK,   rK")
-      (match_operand 7 "const_int_operand"             "    i,    i,    i,    i")
-      (match_operand 8 "const_int_operand"             "    i,    i,    i,    i")
-      (reg:SI VL_REGNUM)
-      (reg:SI VTYPE_REGNUM)] UNSPEC_VPREDICATE)
-   (match_operator:<VM> 3 "equality_operator"
-      [(vec_duplicate:V_VLSF
-         (match_operand:<VEL> 5 "register_operand"     "    f,    f,    f,    f"))
-       (match_operand:V_VLSF 4 "register_operand"      "   vr,   vr,   vr,   vr")])
-   (match_operand:<VM> 2 "vector_merge_operand"        "   vu,    0,    vu,    0")))]
-  "TARGET_VECTOR && riscv_vector::cmp_lmul_le_one (<MODE>mode)"
-  "vmf%B3.vf\t%0,%4,%5%p1"
-  [(set_attr "type" "vfcmp")
-   (set_attr "mode" "<MODE>")
-   (set_attr "spec_restriction" "thv,thv,rvv,rvv")])
-
-;; We use early-clobber for source LMUL > dest LMUL.
-(define_insn "*pred_eqne<mode>_scalar_narrow"
-  [(set (match_operand:<VM> 0 "register_operand"                "=vm,   vr,   vr,  &vr,  &vr")
- (if_then_else:<VM>
-   (unspec:<VM>
-     [(match_operand:<VM> 1 "vector_mask_operand"      "    0,vmWc1,vmWc1,vmWc1,vmWc1")
-      (match_operand 6 "vector_length_operand"         "   rK,   rK,   rK,   rK,   rK")
-      (match_operand 7 "const_int_operand"             "    i,    i,    i,    i,    i")
-      (match_operand 8 "const_int_operand"             "    i,    i,    i,    i,    i")
-      (reg:SI VL_REGNUM)
-      (reg:SI VTYPE_REGNUM)] UNSPEC_VPREDICATE)
-   (match_operator:<VM> 3 "equality_operator"
-      [(vec_duplicate:V_VLSF
-         (match_operand:<VEL> 5 "register_operand"     "    f,    f,    f,    f,    f"))
-       (match_operand:V_VLSF 4 "register_operand"      "   vr,    0,    0,   vr,   vr")])
-   (match_operand:<VM> 2 "vector_merge_operand"        "   vu,   vu,    0,   vu,    0")))]
-  "TARGET_VECTOR && riscv_vector::cmp_lmul_gt_one (<MODE>mode)"
-  "vmf%B3.vf\t%0,%4,%5%p1"
-  [(set_attr "type" "vfcmp")
-   (set_attr "mode" "<MODE>")
-   (set_attr "spec_restriction" "none,thv,thv,none,none")])
-
 ;; -------------------------------------------------------------------------------
 ;; ---- Predicated floating-point merge
 ;; -------------------------------------------------------------------------------
--
2.43.2
Robin Dapp March 1, 2024, 2:53 p.m. UTC | #3 
> 2.      When I write if (a == 2) and if (2 == a), the results are
> same
> 
> 3.      The vec_duplicate operand  is the 5th operand in both cmp and
> eqne patterns. I think they are equal.

A comparison with a constant is always canonicalized to have the
constant second, that's why you won't see a difference.
A vector constant follows the same rule because
swap_commutative_operands_p will place it second.

I'm not sure whether we need the vec_duplicate first, honestly.
I don't remember a canonicalization rule that puts it there.
We do have something for constants and vec_merge.  As long as
things come from expand I think a constant will always be
second and this patch removes the patterns where the duplicate
is first.

Generally with fast math we could invert the condition so
a comparison should be "commutative".  With NaNs I think we
also allow it if the unordered comparisons are supported.
But I'm not even certain that we try something like that with
vectors.  On the other hand - as there is no canonical order
nothing would prevent it from being first in the future?

Will need to think about it some more (and try with NaNs) but
we could give try removing the patterns with GCC15 I suppose.

The rest should still be handled in a more generic fashion.

Regards
 Robin
Robin Dapp May 15, 2024, 7:48 p.m. UTC | #4 
Hi Demin,

are you still going to continue with this?

Regards
 Robin
demin.han May 16, 2024, 1:54 a.m. UTC | #5 
Hi Robin,

Yes.
Can eqne pattern removal patches be committed firstly?

Regards,
Demin

> -----Original Message-----
> From: Robin Dapp <rdapp.gcc@gmail.com>
> Sent: 2024年5月16日 3:49
> To: Demin Han <demin.han@starfivetech.com>; 钟居哲
> <juzhe.zhong@rivai.ai>; gcc-patches <gcc-patches@gcc.gnu.org>
> Cc: rdapp.gcc@gmail.com; kito.cheng <kito.cheng@gmail.com>; Li, Pan2
> <pan2.li@intel.com>; jeffreyalaw <jeffreyalaw@gmail.com>
> Subject: Re: [PATCH 1/5] RISC-V: Remove float vector eqne pattern
> 
> Hi Demin,
> 
> are you still going to continue with this?
> 
> Regards
>  Robin
Robin Dapp May 16, 2024, 7:21 p.m. UTC | #6 
> Can eqne pattern removal patches be committed firstly?

Please first make sure you test with corner cases, NaNs in
particular.  I'm pretty sure we don't have any test cases for
those.

Regards
 Robin
Jeff Law June 8, 2024, 8:48 p.m. UTC | #7 
On 5/16/24 1:21 PM, Robin Dapp wrote:
>> Can eqne pattern removal patches be committed firstly?
> 
> Please first make sure you test with corner cases, NaNs in
> particular.  I'm pretty sure we don't have any test cases for
> those.
But isn't canonicalization of EQ/NE safe, even for IEEE NaN and +-0.0?

target = (a == b) ? x : y
target = (a != b) ? y : x

Are equivalent, even for IEEE IIRC.

jeff
Jeff Law June 8, 2024, 8:50 p.m. UTC | #8 
On 2/29/24 11:27 PM, demin.han wrote:
> We can unify eqne and other comparison operations.
> 
> Tested on RV32 and RV64
> 
> gcc/ChangeLog:
> 
> 	* config/riscv/riscv-vector-builtins-bases.cc: Remove eqne cond
> 	* config/riscv/vector.md (@pred_eqne<mode>_scalar): Remove patterns
> 	(*pred_eqne<mode>_scalar_merge_tie_mask): Ditto
> 	(*pred_eqne<mode>_scalar): Ditto
> 	(*pred_eqne<mode>_scalar_narrow): Ditto
So I'll tentatively ACK this for the trunk, assuming Robin doesn't 
object before Tuesday's patchwork meeting.

jeff
Jeff Law June 8, 2024, 8:51 p.m. UTC | #9 
On 3/1/24 1:12 AM, Demin Han wrote:
> Hi juzhe,
> 
> I also thought it’s related to commutive firstly.
> 
> Following things make me to do the removal:
> 
> 1.No tests fails in regression
> 
> 2.When I write if (a == 2) and if (2 == a), the results are same
GCC canonicalizes comparisons so that constants appear second.

Jeff
Robin Dapp June 10, 2024, 7:33 a.m. UTC | #10 
> But isn't canonicalization of EQ/NE safe, even for IEEE NaN and +-0.0?
> 
> target = (a == b) ? x : y
> target = (a != b) ? y : x
> 
> Are equivalent, even for IEEE IIRC.

Yes, that should be fine.  My concern was not that we do a
canonicalization but that we might not do it for some of the
vector cases.  In particular when one of the operands is wrapped
in a vec_duplicate and we end up with it first rather than
second.

My general feeling is that the patch is good but I wasn't entirely
sure about all cases (in particular in case we transform something
after expand).  That's why I would have liked to see at least some
small test cases for it along with the patch (for the combinations
we don't test yet).

Regards
 Robin
Jeff Law June 10, 2024, 1:49 p.m. UTC | #11 
On 6/10/24 1:33 AM, Robin Dapp wrote:
>> But isn't canonicalization of EQ/NE safe, even for IEEE NaN and +-0.0?
>>
>> target = (a == b) ? x : y
>> target = (a != b) ? y : x
>>
>> Are equivalent, even for IEEE IIRC.
> 
> Yes, that should be fine.  My concern was not that we do a
> canonicalization but that we might not do it for some of the
> vector cases.  In particular when one of the operands is wrapped
> in a vec_duplicate and we end up with it first rather than
> second.
> 
> My general feeling is that the patch is good but I wasn't entirely
> sure about all cases (in particular in case we transform something
> after expand).  That's why I would have liked to see at least some
> small test cases for it along with the patch (for the combinations
> we don't test yet).
Ah, OK.

Demin, can you some additional test coverage, guided by Robin's concerns 
above?

Thanks,
jeff
Li, Pan2 June 10, 2024, 2:52 p.m. UTC | #12 
Not sure if below float eq implement in sail-riscv is useful or not, but looks like some special handling for nan, as well as snan.

https://github.com/riscv/sail-riscv/blob/master/c_emulator/SoftFloat-3e/source/f32_eq.c

Pan

-----Original Message-----
From: Jeff Law <jeffreyalaw@gmail.com> 
Sent: Monday, June 10, 2024 9:50 PM
To: Robin Dapp <rdapp.gcc@gmail.com>; Demin Han <demin.han@starfivetech.com>; 钟居哲 <juzhe.zhong@rivai.ai>; gcc-patches <gcc-patches@gcc.gnu.org>
Cc: kito.cheng <kito.cheng@gmail.com>; Li, Pan2 <pan2.li@intel.com>
Subject: Re: [PATCH 1/5] RISC-V: Remove float vector eqne pattern



On 6/10/24 1:33 AM, Robin Dapp wrote:
>> But isn't canonicalization of EQ/NE safe, even for IEEE NaN and +-0.0?
>>
>> target = (a == b) ? x : y
>> target = (a != b) ? y : x
>>
>> Are equivalent, even for IEEE IIRC.
> 
> Yes, that should be fine.  My concern was not that we do a
> canonicalization but that we might not do it for some of the
> vector cases.  In particular when one of the operands is wrapped
> in a vec_duplicate and we end up with it first rather than
> second.
> 
> My general feeling is that the patch is good but I wasn't entirely
> sure about all cases (in particular in case we transform something
> after expand).  That's why I would have liked to see at least some
> small test cases for it along with the patch (for the combinations
> we don't test yet).
Ah, OK.

Demin, can you some additional test coverage, guided by Robin's concerns 
above?

Thanks,
jeff
demin.han June 10, 2024, 4:16 p.m. UTC | #13 
Hi，

I‘m on vacation rencently.
I will return in a few days and summit new patch with the test.

Regards，
Demin






________________________________
发件人: Jeff Law <jeffreyalaw@gmail.com>
发送时间: 星期一, 六月 10, 2024 9:49 下午
收件人: Robin Dapp <rdapp.gcc@gmail.com>; Demin Han <demin.han@starfivetech.com>; 钟居哲 <juzhe.zhong@rivai.ai>; gcc-patches <gcc-patches@gcc.gnu.org>
抄送: kito.cheng <kito.cheng@gmail.com>; Li, Pan2 <pan2.li@intel.com>
主题: Re: [PATCH 1/5] RISC-V: Remove float vector eqne pattern



On 6/10/24 1:33 AM, Robin Dapp wrote:
>> But isn't canonicalization of EQ/NE safe, even for IEEE NaN and +-0.0?
>>
>> target = (a == b) ? x : y
>> target = (a != b) ? y : x
>>
>> Are equivalent, even for IEEE IIRC.
>
> Yes, that should be fine.  My concern was not that we do a
> canonicalization but that we might not do it for some of the
> vector cases.  In particular when one of the operands is wrapped
> in a vec_duplicate and we end up with it first rather than
> second.
>
> My general feeling is that the patch is good but I wasn't entirely
> sure about all cases (in particular in case we transform something
> after expand).  That's why I would have liked to see at least some
> small test cases for it along with the patch (for the combinations
> we don't test yet).
Ah, OK.

Demin, can you some additional test coverage, guided by Robin's concerns
above?

Thanks,
jeff
Jeff Law June 10, 2024, 4:23 p.m. UTC | #14 
On 6/10/24 10:16 AM, Demin Han wrote:
> Hi，
> 
> I‘m on vacation rencently.
> I will return in a few days and summit new patch with the test.
No problem.  Enjoy your vacation, this can certainly wait until you return.

jeff
Jeff Law June 10, 2024, 4:30 p.m. UTC | #15 
On 6/10/24 8:52 AM, Li, Pan2 wrote:
> Not sure if below float eq implement in sail-riscv is useful or not, but looks like some special handling for nan, as well as snan.
> 
> https://github.com/riscv/sail-riscv/blob/master/c_emulator/SoftFloat-3e/source/f32_eq.c
Yes, but it's symmetrical, which is what we'd want to see.

jeff
diff mbox series

Patch

diff --git a/gcc/config/riscv/riscv-vector-builtins-bases.cc b/gcc/config/riscv/riscv-vector-builtins-bases.cc
index b6f6e4ff37e..d414721ede8 100644
--- a/gcc/config/riscv/riscv-vector-builtins-bases.cc
+++ b/gcc/config/riscv/riscv-vector-builtins-bases.cc
@@ -1420,10 +1420,6 @@  public:
     switch (e.op_info->op)
       {
 	case OP_TYPE_vf: {
-	  if (CODE == EQ || CODE == NE)
-	    return e.use_compare_insn (CODE, code_for_pred_eqne_scalar (
-					       e.vector_mode ()));
-	  else
 	    return e.use_compare_insn (CODE, code_for_pred_cmp_scalar (
 					       e.vector_mode ()));
 	}
diff --git a/gcc/config/riscv/vector.md b/gcc/config/riscv/vector.md
index ab6e099852d..9210d7c28ad 100644
--- a/gcc/config/riscv/vector.md
+++ b/gcc/config/riscv/vector.md
@@ -7520,92 +7520,6 @@  (define_insn "*pred_cmp<mode>_scalar_narrow"
    (set_attr "mode" "<MODE>")
    (set_attr "spec_restriction" "none,thv,thv,none,none")])
 
-(define_expand "@pred_eqne<mode>_scalar"
-  [(set (match_operand:<VM> 0 "register_operand")
-	(if_then_else:<VM>
-	  (unspec:<VM>
-	    [(match_operand:<VM> 1 "vector_mask_operand")
-	     (match_operand 6 "vector_length_operand")
-	     (match_operand 7 "const_int_operand")
-	     (match_operand 8 "const_int_operand")
-	     (reg:SI VL_REGNUM)
-	     (reg:SI VTYPE_REGNUM)] UNSPEC_VPREDICATE)
-	  (match_operator:<VM> 3 "equality_operator"
-	     [(vec_duplicate:V_VLSF
-	        (match_operand:<VEL> 5 "register_operand"))
-	      (match_operand:V_VLSF 4 "register_operand")])
-	  (match_operand:<VM> 2 "vector_merge_operand")))]
-  "TARGET_VECTOR"
-  {})
-
-(define_insn "*pred_eqne<mode>_scalar_merge_tie_mask"
-  [(set (match_operand:<VM> 0 "register_operand"              "=vm")
-	(if_then_else:<VM>
-	  (unspec:<VM>
-	    [(match_operand:<VM> 1 "register_operand"         "  0")
-	     (match_operand 5 "vector_length_operand"         " rK")
-	     (match_operand 6 "const_int_operand"             "  i")
-	     (match_operand 7 "const_int_operand"             "  i")
-	     (reg:SI VL_REGNUM)
-	     (reg:SI VTYPE_REGNUM)] UNSPEC_VPREDICATE)
-	  (match_operator:<VM> 2 "equality_operator"
-	     [(vec_duplicate:V_VLSF
-	        (match_operand:<VEL> 4 "register_operand"     "  f"))
-	      (match_operand:V_VLSF 3 "register_operand"      " vr")])
-	  (match_dup 1)))]
-  "TARGET_VECTOR"
-  "vmf%B2.vf\t%0,%3,%4,v0.t"
-  [(set_attr "type" "vfcmp")
-   (set_attr "mode" "<MODE>")
-   (set_attr "merge_op_idx" "1")
-   (set_attr "vl_op_idx" "5")
-   (set (attr "ma") (symbol_ref "riscv_vector::get_ma(operands[6])"))
-   (set (attr "avl_type_idx") (const_int 7))])
-
-;; We don't use early-clobber for LMUL <= 1 to get better codegen.
-(define_insn "*pred_eqne<mode>_scalar"
-  [(set (match_operand:<VM> 0 "register_operand"                "=vr,   vr,   &vr,   &vr")
-	(if_then_else:<VM>
-	  (unspec:<VM>
-	    [(match_operand:<VM> 1 "vector_mask_operand"      "vmWc1,vmWc1,vmWc1,vmWc1")
-	     (match_operand 6 "vector_length_operand"         "   rK,   rK,   rK,   rK")
-	     (match_operand 7 "const_int_operand"             "    i,    i,    i,    i")
-	     (match_operand 8 "const_int_operand"             "    i,    i,    i,    i")
-	     (reg:SI VL_REGNUM)
-	     (reg:SI VTYPE_REGNUM)] UNSPEC_VPREDICATE)
-	  (match_operator:<VM> 3 "equality_operator"
-	     [(vec_duplicate:V_VLSF
-	        (match_operand:<VEL> 5 "register_operand"     "    f,    f,    f,    f"))
-	      (match_operand:V_VLSF 4 "register_operand"      "   vr,   vr,   vr,   vr")])
-	  (match_operand:<VM> 2 "vector_merge_operand"        "   vu,    0,    vu,    0")))]
-  "TARGET_VECTOR && riscv_vector::cmp_lmul_le_one (<MODE>mode)"
-  "vmf%B3.vf\t%0,%4,%5%p1"
-  [(set_attr "type" "vfcmp")
-   (set_attr "mode" "<MODE>")
-   (set_attr "spec_restriction" "thv,thv,rvv,rvv")])
-
-;; We use early-clobber for source LMUL > dest LMUL.
-(define_insn "*pred_eqne<mode>_scalar_narrow"
-  [(set (match_operand:<VM> 0 "register_operand"                "=vm,   vr,   vr,  &vr,  &vr")
-	(if_then_else:<VM>
-	  (unspec:<VM>
-	    [(match_operand:<VM> 1 "vector_mask_operand"      "    0,vmWc1,vmWc1,vmWc1,vmWc1")
-	     (match_operand 6 "vector_length_operand"         "   rK,   rK,   rK,   rK,   rK")
-	     (match_operand 7 "const_int_operand"             "    i,    i,    i,    i,    i")
-	     (match_operand 8 "const_int_operand"             "    i,    i,    i,    i,    i")
-	     (reg:SI VL_REGNUM)
-	     (reg:SI VTYPE_REGNUM)] UNSPEC_VPREDICATE)
-	  (match_operator:<VM> 3 "equality_operator"
-	     [(vec_duplicate:V_VLSF
-	        (match_operand:<VEL> 5 "register_operand"     "    f,    f,    f,    f,    f"))
-	      (match_operand:V_VLSF 4 "register_operand"      "   vr,    0,    0,   vr,   vr")])
-	  (match_operand:<VM> 2 "vector_merge_operand"        "   vu,   vu,    0,   vu,    0")))]
-  "TARGET_VECTOR && riscv_vector::cmp_lmul_gt_one (<MODE>mode)"
-  "vmf%B3.vf\t%0,%4,%5%p1"
-  [(set_attr "type" "vfcmp")
-   (set_attr "mode" "<MODE>")
-   (set_attr "spec_restriction" "none,thv,thv,none,none")])
-
 ;; -------------------------------------------------------------------------------
 ;; ---- Predicated floating-point merge
 ;; -------------------------------------------------------------------------------