Re: [PATCH 1/5] RISC-V: Remove float vector eqne pattern
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
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
Hi， I‘m on vacation rencently. I will return in a few days and summit new patch with the test. Regards， Demin 发件人: Jeff Law 发送时间: 星期一, 六月 10, 2024 9:49 下午 收件人: Robin Dapp ; Demin Han ; 钟居哲 ; gcc-patches 抄送: kito.cheng ; Li, Pan2 主题: 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
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 Sent: Monday, June 10, 2024 9:50 PM To: Robin Dapp ; Demin Han ; 钟居哲 ; gcc-patches Cc: kito.cheng ; Li, Pan2 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
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
> 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
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
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_scalar): Remove patterns (*pred_eqne_scalar_merge_tie_mask): Ditto (*pred_eqne_scalar): Ditto (*pred_eqne_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
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
> 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
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@starfivetech.com/ https://patchwork.sourceware.org/project/gcc/patch/20240301062711.207137-2-demin@starfivetech.com/ Regards, Demin From: 钟居哲 Sent: 2024年5月16日 10:02 To: Robin Dapp ; Demin Han ; gcc-patches Cc: rdapp.gcc ; kito.cheng ; Li, Pan2 ; jeffreyalaw 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 Dappmailto:rdapp@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
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
RE: [PATCH 1/5] RISC-V: Remove float vector eqne pattern
Hi Robin, Yes. Can eqne pattern removal patches be committed firstly? Regards, Demin > -Original Message- > From: Robin Dapp > Sent: 2024年5月16日 3:49 > To: Demin Han ; 钟居哲 > ; gcc-patches > Cc: rdapp@gmail.com; kito.cheng ; Li, Pan2 > ; jeffreyalaw > 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
Hi Demin, are you still going to continue with this? Regards Robin
Re: [PATCH 1/5] RISC-V: Remove float vector eqne pattern
> 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
RE: Re:[PATCH 1/5] RISC-V: Remove float vector eqne pattern
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: 钟居哲
Sent: 2024年3月1日 15:24
To: Demin Han ; gcc-patches
Cc: kito.cheng ; Li, Pan2 ;
jeffreyalaw ; Robin Dapp
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"mailto:demin@starfivetech.com>>;
Date: Fri, Mar 1, 2024 02:27 PM
To: "gcc-patches"mailto:gcc-patches@gcc.gnu.org>>;
Cc: "juzhe.zhong"mailto:juzhe.zh...@rivai.ai>>;
"kito.cheng"mailto:kito.ch...@gmail.com>>; "Li,
Pan2"mailto:pan2...@intel.com>>;
"jeffreyalaw"mailto:jeffreya...@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_scalar): Remove patterns
(*pred_eqne_scalar_merge_tie_mask): Ditto
(*pred_eqne_scalar): Ditto
(*pred_eqne_scalar_narrow): Ditto
Signed-off-by: demin.han
mailto:demin@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_scalar_narrow"
(set_attr "mode" "")
(set_attr "spec_restriction" "none,thv,thv,none,none")])
-(define_expand "@pred_eqne_scalar"
- [(set (match_operand: 0 "register_operand")
- (if_then_else:
- (unspec:
- [(match_operand: 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: 3 "equality_operator"
- [(vec_duplicate:V_VLSF
- (match_operand: 5 "register_operand"))
- (match_operand:V_VLSF 4 "register_operand")])
- (match_operand: 2 "vector_merge_operand")))]
- "TARGET_VECTOR"
- {})
-
-(define_insn "*pred_eqne_scalar_merge_tie_mask"
- [(set (match_operand: 0 "register_operand" "=vm")
- (if_then_else:
- (unspec:
- [(match_operand: 1 "register_operand" " 0")
- (match_operand 5 "vector_length_operand" " rK")
- (match_operand 6 "const_int_operand" " i")
- (match_operand 7 "const_int_operand"
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"
[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_scalar): Remove patterns
(*pred_eqne_scalar_merge_tie_mask): Ditto
(*pred_eqne_scalar): Ditto
(*pred_eqne_scalar_narrow): Ditto
Signed-off-by: demin.han
---
.../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_scalar_narrow"
(set_attr "mode" "")
(set_attr "spec_restriction" "none,thv,thv,none,none")])
-(define_expand "@pred_eqne_scalar"
- [(set (match_operand: 0 "register_operand")
- (if_then_else:
- (unspec:
- [(match_operand: 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: 3 "equality_operator"
-[(vec_duplicate:V_VLSF
- (match_operand: 5 "register_operand"))
- (match_operand:V_VLSF 4 "register_operand")])
- (match_operand: 2 "vector_merge_operand")))]
- "TARGET_VECTOR"
- {})
-
-(define_insn "*pred_eqne_scalar_merge_tie_mask"
- [(set (match_operand: 0 "register_operand" "=vm")
- (if_then_else:
- (unspec:
- [(match_operand: 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: 2 "equality_operator"
-[(vec_duplicate:V_VLSF
- (match_operand: 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" "")
- (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_scalar"
- [(set (match_operand: 0 "register_operand""=vr, vr,
&vr, &vr")
- (if_then_else:
- (unspec:
- [(match_operand: 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: 3 "equality_operator"
-[(vec_duplicate:V_VLSF
- (match_operand: 5 "register_operand" "f,f,
f,f"))
- (match_operand:V_VLSF 4 "register_operand" " vr, vr,
vr, vr")])
- (match_operand: 2 "vector_merge_operand"" vu,0,
vu,0")))]
- "TARGET_VECTOR && riscv_vector::cmp_lmul_le_one (mode)"
- "vmf%B3.vf\t%0,%4,%5%p1"
- [(set_attr "type" "vfcmp")
- (set_attr "mode" "")
- (set_attr "spec_restriction" "thv,thv,rvv,rvv")])
-
-;; We use early-clobber for source LMUL > dest LMUL.
-(define_insn "*pred_eqne_scalar_narrow"
- [(set (match_operand: 0 "register_operand""=vm, vr,
vr, &vr, &vr")
- (if_then_else:
- (unspec:
- [(match_operand: 1 "vector_mask_operand" "
0,vmWc1,vmWc1,vmWc1,vmWc1")
-(match_operand 6 "vector_length_operand" " rK, rK,
rK, rK, rK")
-(match_
