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

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

[=?gb18030?B?1tO+09Xc?=]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb18030", Size: 289 bytes --]

Would you minding sending this patch again£¿
I can not find the patch now.








 ----------Reply to Message----------
 On Thu, May 16, 2024 03:48 AM Robin Dapp<rdapp.gcc@gmail.com&gt; wrote:

  Hi Demin,

are you still going to continue with this?

Regards
&nbsp;Robin

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

[Demin Han]

[-- Attachment #1: Type: text/plain, Size: 1011 bytes --]

Hi Juzhe,

There are two eqne pattern removal patches, one for float, another for integer.

https://patchwork.sourceware.org/project/gcc/patch/20240301062711.207137-5-demin.han@starfivetech.com/

https://patchwork.sourceware.org/project/gcc/patch/20240301062711.207137-2-demin.han@starfivetech.com/


Regards,
Demin
From: 钟居哲 <juzhe.zhong@rivai.ai>
Sent: 2024年5月16日 10:02
To: Robin Dapp <rdapp.gcc@gmail.com>; Demin Han <demin.han@starfivetech.com>; gcc-patches <gcc-patches@gcc.gnu.org>
Cc: rdapp.gcc <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

Would you minding sending this patch again？
I can not find the patch now.



----------Reply to Message----------
On Thu, May 16, 2024 03:48 AM Robin Dapp<rdapp.gcc@gmail.com<mailto:rdapp.gcc@gmail.com>> wrote:
Hi Demin,

are you still going to continue with this?

Regards
 Robin

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

[Jeff Law]

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

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

[Jeff Law]

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

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

[Demin Han]

[-- Attachment #1: Type: text/plain, Size: 1456 bytes --]

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

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

[Li, Pan2]

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

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

[Jeff Law]

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

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

[Robin Dapp]

> 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

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

[Jeff Law]

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

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

[Jeff Law]

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

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

[Jeff Law]

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

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

[Robin Dapp]

> 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

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

[Demin Han]

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

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

[Robin Dapp]

Hi Demin,

are you still going to continue with this?

Regards
 Robin

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

[Robin Dapp]

> 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

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

[demin.han]

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(-)

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