Re: [PATCH] RISC-V: Support combine cond extend and reduce sum to cond widen reduce sum
Hi Lehua,
> +(define_expand "vcond_mask_"
> + [(set (match_operand:V_VLS 0 "register_operand")
> +(if_then_else:V_VLS
> + (match_operand: 3 "register_operand")
> + (match_operand:V_VLS 1 "nonmemory_operand")
> + (match_operand:V_VLS 2 "vector_register_or_const_0_operand")))]
> + "TARGET_VECTOR"
Would it hurt to allow any nonmemory operand here and just force the
"unsupported" constants into a register?
> + {
> +if (satisfies_constraint_Wc0 (operands[2]))
> + {
> +rtx reg = gen_reg_rtx (mode);
> +emit_insn (gen_vec_duplicate_const_0 (reg, operands[2]));
Can't we emit a move_insn directly without going through the new pattern?
Or will that be optimized away in between? And the new pattern isn't
actually a duplicate but a move anyway so maybe a force_reg (operands[2]) would
already do? Probably initial values other than 0 don't work out of the box?
In any case it wouldn't hurt to describe the "design decisions" (i.e. why
we need one thing and not another) so it's easier to follow the patterns
in the future.
Regards
Robin
Re: [PATCH] RISC-V: Support combine cond extend and reduce sum to cond widen reduce sum
Hi Robin,
+(define_expand "vcond_mask_"
+ [(set (match_operand:V_VLS 0 "register_operand")
+(if_then_else:V_VLS
+ (match_operand: 3 "register_operand")
+ (match_operand:V_VLS 1 "nonmemory_operand")
+ (match_operand:V_VLS 2 "vector_register_or_const_0_operand")))]
+ "TARGET_VECTOR"
Would it hurt to allow any nonmemory operand here and just force the
"unsupported" constants into a register?
Are you talking about why operand 2 doesn't use nonmemory_operand
predicate? If so, I think this is because our vmerge.v[vxi]m insns only
supports that operand 1 is a scalar and operand 2 must be a vector register.
+ {
+if (satisfies_constraint_Wc0 (operands[2]))
+ {
+rtx reg = gen_reg_rtx (mode);
+emit_insn (gen_vec_duplicate_const_0 (reg, operands[2]));
Can't we emit a move_insn directly without going through the new pattern?
Or will that be optimized away in between?
Currently, the move_insn of moving vector const 0 to a vector register
will produce the bellow rtl from vector.md, I hope keep the simple
pattern before split1 pass:
(set (reg:RVVM2HI 149)
(if_then_else:RVVM2HI (unspec:RVVMF8BI [
(const_vector:RVVMF8BI repeat [
(const_int 1 [0x1])
])
(reg:DI 146)
(const_int 2 [0x2]) repeated x2
(const_int 1 [0x1])
(reg:SI 66 vl)
(reg:SI 67 vtype)
] UNSPEC_VPREDICATE)
(const_vector:RVVM2HI repeat [
(const_int 0 [0])
])
(unspec:RVVM2HI [
(reg:SI 0 zero)
] UNSPEC_VUNDEF)))
And the new pattern isn't
actually a duplicate but a move anyway so maybe a force_reg (operands[2]) would
already do? Probably initial values other than 0 don't work out of the box?
Yes, the name vec_duplicate_const_0 is missleading, maybe change it to
mov_vec_const_0 is better.
In any case it wouldn't hurt to describe the "design decisions" (i.e. why
we need one thing and not another) so it's easier to follow the patterns
in the future.
Yes, I should have described the design in more detail. Before this
patch, if I wanted to implement the current combine optimization, I
would need to create a combine pattern that combines the following three
instructions:
(set (reg:RVVM2HI 149)
(if_then_else:RVVM2HI (unspec:RVVMF8BI [
(const_vector:RVVMF8BI repeat [
(const_int 1 [0x1])
])
(reg:DI 146)
(const_int 2 [0x2]) repeated x2
(const_int 1 [0x1])
(reg:SI 66 vl)
(reg:SI 67 vtype)
] UNSPEC_VPREDICATE)
(const_vector:RVVM2HI repeat [
(const_int 0 [0])
])
(unspec:RVVM2HI [
(reg:SI 0 zero)
] UNSPEC_VUNDEF)))
(set (reg:RVVM2HI 138 [ vect__ifc__38.17 ])
(if_then_else:RVVM2HI (reg:RVVMF8BI 135 [ mask__18.11 ])
(sign_extend:RVVM2HI (reg:RVVM1QI 136 [ vect__6.14 ]))
(reg:RVVM2HI 149)))
(set (reg:HI 151)
(unspec:HI [
(reg:RVVM2HI 138 [ vect__ifc__38.17 ])
] UNSPEC_REDUC_SUM))
into one insn:
(set (reg:SF 139 [ ])
(unspec:SF [
(if_then_else:RVVM2SF (reg:RVVMF16BI 135 [ mask__11.32 ])
(float_extend:RVVM2SF (reg:RVVM1HF 136 [ vect__7.35 ]))
(if_then_else:RVVM2SF (unspec:RVVMF16BI [
(const_vector:RVVMF16BI repeat [
(const_int 1 [0x1])
])
(reg:DI 143)
(const_int 2 [0x2]) repeated x2
(const_int 1 [0x1])
(reg:SI 66 vl)
(reg:SI 67 vtype)
] UNSPEC_VPREDICATE)
(const_vector:RVVM2SF repeat [
(const_double:SF 0.0 [0x0.0p+0])
])
(unspec:RVVM2SF [
(reg:SI 0 zero)
] UNSPEC_VUNDEF)))
] UNSPEC_REDUC_SUM_UNORDERED))
But I think the combine pattern is ugly because the
move_vector_const_0_to_vector_registers has many extra operands we don't
care about. So I want to introduce a simple pattern of it. I have two
options here, one is to modify the generic mov pattern, changing
define_expand to define_insn_and_split to keep pattern simple. The
reason why I did not choose this one is that mov pattern uses a lot of
places, including reload pass. The impact is relatively big.
The other option (this patch chooses) is to s
Re: [PATCH] RISC-V: Support combine cond extend and reduce sum to cond widen reduce sum
Hi Lehua,
>> Would it hurt to allow any nonmemory operand here and just force the
>> "unsupported" constants into a register?
>
> Are you talking about why operand 2 doesn't use nonmemory_operand
> predicate? If so, I think this is because our vmerge.v[vxi]m insns
> only supports that operand 1 is a scalar and operand 2 must be a
> vector register.
My question was rather:
Why doesn't something like
(define_insn_and_split "vcond_mask_"
[(set (match_operand:V_VLS 0 "register_operand")
(if_then_else:V_VLS
(match_operand: 3 "register_operand")
(match_operand:V_VLS 1 "nonmemory_operand")
(match_operand:V_VLS 2 "nonmemory_operand")))]
"TARGET_VECTOR && can_create_pseudo_p ()"
"#"
"&& 1"
[(const_int 0)]
{
/* The order of vcond_mask is opposite to pred_merge. */
if (REG_P (operands[2]))
operands[2] = force_reg (mode, operands[2]);
std::swap (operands[1], operands[2]);
riscv_vector::emit_vlmax_insn (code_for_pred_merge (mode),
riscv_vector::MERGE_OP, operands);
DONE;
}
[(set_attr "type" "vector")]
)
suffice? You could disallow operands[2] != 0 if needed.
Regards
Robin
Re: [PATCH] RISC-V: Support combine cond extend and reduce sum to cond widen reduce sum
Hi Robin,
Would it hurt to allow any nonmemory operand here and just force the
"unsupported" constants into a register?
Are you talking about why operand 2 doesn't use nonmemory_operand
predicate? If so, I think this is because our vmerge.v[vxi]m insns
only supports that operand 1 is a scalar and operand 2 must be a
vector register.
My question was rather:
Why doesn't something like
(define_insn_and_split "vcond_mask_"
[(set (match_operand:V_VLS 0 "register_operand")
(if_then_else:V_VLS
(match_operand: 3 "register_operand")
(match_operand:V_VLS 1 "nonmemory_operand")
(match_operand:V_VLS 2 "nonmemory_operand")))]
"TARGET_VECTOR && can_create_pseudo_p ()"
"#"
"&& 1"
[(const_int 0)]
{
/* The order of vcond_mask is opposite to pred_merge. */
if (REG_P (operands[2]))
operands[2] = force_reg (mode, operands[2]);
std::swap (operands[1], operands[2]);
riscv_vector::emit_vlmax_insn (code_for_pred_merge (mode),
riscv_vector::MERGE_OP, operands);
DONE;
}
[(set_attr "type" "vector")]
)
suffice? You could disallow operands[2] != 0 if needed.
I think I understand what you're saying. The reason for not doing this
is because simply allowing the operand 2 of vcond_mask to be vec_const_0
would cause the other combine patterns to fail because they require the
operand 2 of vcond_mask to be a register. Like the following existing
combine patterns (operand 2 as the instruction merge operand, is not
allowed to be non-register):
(define_insn_and_split "*cond_"
[(set (match_operand:VF 0 "register_operand")
(if_then_else:VF
(match_operand: 1 "register_operand")
(any_float_unop:VF
(match_operand:VF 2 "register_operand"))
(match_operand:VF 3 "register_operand")))]
"TARGET_VECTOR && can_create_pseudo_p ()"
"#"
"&& 1"
[(const_int 0)]
{
insn_code icode = code_for_pred (, mode);
rtx ops[] = {operands[0], operands[1], operands[2], operands[3],
gen_int_mode (GET_MODE_NUNITS (mode), Pmode)};
riscv_vector::expand_cond_len_unop (icode, ops);
DONE;
}
[(set_attr "type" "vector")])
My current method is still to keep the operand 2 of vcond_mask as a
register, but the pattern of mov_vec_const_0 is simplified, so that the
corresponding combine pattern can be more simple. That's the only reason
I split the vcond_mask into three patterns.
--
Best,
Lehua
Re: [PATCH] RISC-V: Support combine cond extend and reduce sum to cond widen reduce sum
Hi Lehua, thanks for the explanation. > My current method is still to keep the operand 2 of vcond_mask as a > register, but the pattern of mov_vec_const_0 is simplified, so that > the corresponding combine pattern can be more simple. That's the only > reason I split the vcond_mask into three patterns. My "problem" with the separate split it that it really sticks out and everybody seeing it would wonder why we need it. It's not that bad of course but it appears as if we messed up somewhere else. I checked and I don't see additional FAILs with the vmask pattern that additionally allows a const0 operand (that is forced into a register) and a force_reg in abs:VF. Would you mind re-checking if we can avoid the extra "vec_duplicate_const_0" by changing the other affected patterns in a similar manner? I really didn't verify in-depth so if we needed to add a force_reg to every pattern we might need to reconsider. Still, I'd be unsure if I preferred the "vec_dup_const_0" over additional force_regs ;) Regards Robin
Re: [PATCH] RISC-V: Support combine cond extend and reduce sum to cond widen reduce sum
On 2023/9/20 6:02, Robin Dapp wrote:
Hi Lehua,
thanks for the explanation.
My current method is still to keep the operand 2 of vcond_mask as a
register, but the pattern of mov_vec_const_0 is simplified, so that
the corresponding combine pattern can be more simple. That's the only
reason I split the vcond_mask into three patterns.
My "problem" with the separate split it that it really sticks out
and everybody seeing it would wonder why we need it. It's not that
bad of course but it appears as if we messed up somewhere else.
Can not agree more.
I checked and I don't see additional FAILs with the vmask pattern
that additionally allows a const0 operand (that is forced into a register)
and a force_reg in abs:VF.
Would you mind re-checking if we can avoid the extra
"vec_duplicate_const_0" by changing the other affected patterns
in a similar manner? I really didn't verify in-depth so if we needed
to add a force_reg to every pattern we might need to reconsider.
Still, I'd be unsure if I preferred the "vec_dup_const_0" over
additional force_regs ;)
I think that's a little weird, too. I prefer to add a single
define_insn_and_split move_const_0 pattern like following diff, How do
you feel about that?
diff --git a/gcc/config/riscv/riscv-v.cc b/gcc/config/riscv/riscv-v.cc
index f4dab9fceb8..c0ab96ae8ab 100644
--- a/gcc/config/riscv/riscv-v.cc
+++ b/gcc/config/riscv/riscv-v.cc
@@ -973,7 +973,11 @@ expand_const_vector (rtx target, rtx src)
rtx tmp = register_operand (target, mode) ? target : gen_reg_rtx
(mode);
/* Element in range -16 ~ 15 integer or 0.0 floating-point,
we use vmv.v.i instruction. */
- if (satisfies_constraint_vi (src) || satisfies_constraint_Wc0 (src))
+ /* For const int or float 0, we keep the simple pattern before split1
+pass. */
+ if (can_create_pseudo_p () && satisfies_constraint_Wc0 (src))
+ emit_insn (gen_mov_vec_const_0 (mode, tmp, src));
+ else if (satisfies_constraint_vi (src))
{
rtx ops[] = {tmp, src};
emit_vlmax_insn (code_for_pred_mov (mode), UNARY_OP, ops);
diff --git a/gcc/config/riscv/vector.md b/gcc/config/riscv/vector.md
index f66ffebba24..b4973125d04 100644
--- a/gcc/config/riscv/vector.md
+++ b/gcc/config/riscv/vector.md
@@ -1640,6 +1640,19 @@
"TARGET_VECTOR"
{})
+(define_insn_and_split "@mov_vec_const_0"
+ [(set (match_operand:V_VLS 0 "register_operand")
+(match_operand:V_VLS 1 "vector_const_0_operand"))]
+ "TARGET_VECTOR && can_create_pseudo_p ()"
+ "#"
+ "&& 1"
+ [(const_int 0)]
+ {
+emit_move_insn (operands[0], operands[1]);
+DONE;
+ }
+ [(set_attr "type" "vector")])
+
;; vle.v/vse.v,vmv.v.v
(define_insn_and_split "*pred_mov"
[(set (match_operand:V_VLS 0 "nonimmediate_operand""=vr,
vr,vd, m,vr,vr")
--
Best,
Lehua
Re: [PATCH] RISC-V: Support combine cond extend and reduce sum to cond widen reduce sum
Hi Robin,
I have posted a V2 patch to implement the method I mentioned. I wonder
if that makes it a little easier to understand?
V2 patch:
https://gcc.gnu.org/pipermail/gcc-patches/2023-September/630985.html
On 2023/9/20 12:02, Lehua Ding wrote:
On 2023/9/20 6:02, Robin Dapp wrote:
Hi Lehua,
thanks for the explanation.
My current method is still to keep the operand 2 of vcond_mask as a
register, but the pattern of mov_vec_const_0 is simplified, so that
the corresponding combine pattern can be more simple. That's the only
reason I split the vcond_mask into three patterns.
My "problem" with the separate split it that it really sticks out
and everybody seeing it would wonder why we need it. It's not that
bad of course but it appears as if we messed up somewhere else.
Can not agree more.
I checked and I don't see additional FAILs with the vmask pattern
that additionally allows a const0 operand (that is forced into a
register)
and a force_reg in abs:VF.
Would you mind re-checking if we can avoid the extra
"vec_duplicate_const_0" by changing the other affected patterns
in a similar manner? I really didn't verify in-depth so if we needed
to add a force_reg to every pattern we might need to reconsider.
Still, I'd be unsure if I preferred the "vec_dup_const_0" over
additional force_regs ;)
I think that's a little weird, too. I prefer to add a single
define_insn_and_split move_const_0 pattern like following diff, How do
you feel about that?
diff --git a/gcc/config/riscv/riscv-v.cc b/gcc/config/riscv/riscv-v.cc
index f4dab9fceb8..c0ab96ae8ab 100644
--- a/gcc/config/riscv/riscv-v.cc
+++ b/gcc/config/riscv/riscv-v.cc
@@ -973,7 +973,11 @@ expand_const_vector (rtx target, rtx src)
rtx tmp = register_operand (target, mode) ? target : gen_reg_rtx
(mode);
/* Element in range -16 ~ 15 integer or 0.0 floating-point,
we use vmv.v.i instruction. */
- if (satisfies_constraint_vi (src) || satisfies_constraint_Wc0 (src))
+ /* For const int or float 0, we keep the simple pattern before
split1
+ pass. */
+ if (can_create_pseudo_p () && satisfies_constraint_Wc0 (src))
+ emit_insn (gen_mov_vec_const_0 (mode, tmp, src));
+ else if (satisfies_constraint_vi (src))
{
rtx ops[] = {tmp, src};
emit_vlmax_insn (code_for_pred_mov (mode), UNARY_OP, ops);
diff --git a/gcc/config/riscv/vector.md b/gcc/config/riscv/vector.md
index f66ffebba24..b4973125d04 100644
--- a/gcc/config/riscv/vector.md
+++ b/gcc/config/riscv/vector.md
@@ -1640,6 +1640,19 @@
"TARGET_VECTOR"
{})
+(define_insn_and_split "@mov_vec_const_0"
+ [(set (match_operand:V_VLS 0 "register_operand")
+ (match_operand:V_VLS 1 "vector_const_0_operand"))]
+ "TARGET_VECTOR && can_create_pseudo_p ()"
+ "#"
+ "&& 1"
+ [(const_int 0)]
+ {
+ emit_move_insn (operands[0], operands[1]);
+ DONE;
+ }
+ [(set_attr "type" "vector")])
+
;; vle.v/vse.v,vmv.v.v
(define_insn_and_split "*pred_mov"
[(set (match_operand:V_VLS 0 "nonimmediate_operand" "=vr,
vr, vd, m, vr, vr")
--
Best,
Lehua
