* [PATCH] riscv: Split off shift patterns for autovectorization.
@ 2023-05-10 15:24 Robin Dapp
2023-05-10 19:19 ` Palmer Dabbelt
0 siblings, 1 reply; 8+ messages in thread
From: Robin Dapp @ 2023-05-10 15:24 UTC (permalink / raw)
To: gcc-patches, juzhe.zhong, Kito Cheng, Michael Collison, palmer,
jeffreyalaw
Cc: rdapp.gcc
Hi,
this patch splits off the shift patterns of the binop patterns.
This is necessary as the scalar shifts require a Pmode operand
as shift count. To this end, a new iterator any_int_binop_no_shift
is introduced. At a later point when the binops are split up
further in commutative and non-commutative patterns (which both
do not include the shift patterns) we might not need this anymore.
Bootstrapped and regtested.
Regards
Robin
--
gcc/ChangeLog:
* config/riscv/autovec.md (<optab><mode>3): Add scalar shift
pattern.
(v<optab><mode>3): Add vector shift pattern.
* config/riscv/vector-iterators.md: New iterator.
---
gcc/config/riscv/autovec.md | 40 +++++++++++++++++++++++++++-
gcc/config/riscv/vector-iterators.md | 4 +++
2 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/gcc/config/riscv/autovec.md b/gcc/config/riscv/autovec.md
index 8347e42bb9c..2da4fc67d51 100644
--- a/gcc/config/riscv/autovec.md
+++ b/gcc/config/riscv/autovec.md
@@ -65,7 +65,7 @@ (define_expand "movmisalign<mode>"
(define_expand "<optab><mode>3"
[(set (match_operand:VI 0 "register_operand")
- (any_int_binop:VI
+ (any_int_binop_no_shift:VI
(match_operand:VI 1 "<binop_rhs1_predicate>")
(match_operand:VI 2 "<binop_rhs2_predicate>")))]
"TARGET_VECTOR"
@@ -91,3 +91,41 @@ (define_expand "<optab><mode>3"
NULL_RTX, <VM>mode);
DONE;
})
+
+;; =========================================================================
+;; == Binary integer shifts by scalar.
+;; =========================================================================
+
+(define_expand "<optab><mode>3"
+ [(set (match_operand:VI 0 "register_operand")
+ (any_shift:VI
+ (match_operand:VI 1 "register_operand")
+ (match_operand:<VEL> 2 "csr_operand")))]
+ "TARGET_VECTOR"
+{
+ if (!CONST_SCALAR_INT_P (operands[2]))
+ operands[2] = gen_lowpart (Pmode, operands[2]);
+ riscv_vector::emit_len_binop (code_for_pred_scalar
+ (<BINOP_TO_UPPERCASE>, <MODE>mode),
+ operands[0], operands[1], operands[2],
+ NULL_RTX, <VM>mode, Pmode);
+ DONE;
+})
+
+;; =========================================================================
+;; == Binary integer shifts by vector.
+;; =========================================================================
+
+(define_expand "v<optab><mode>3"
+ [(set (match_operand:VI 0 "register_operand")
+ (any_shift:VI
+ (match_operand:VI 1 "register_operand")
+ (match_operand:VI 2 "vector_shift_operand")))]
+ "TARGET_VECTOR"
+{
+ riscv_vector::emit_len_binop (code_for_pred
+ (<BINOP_TO_UPPERCASE>, <MODE>mode),
+ operands[0], operands[1], operands[2],
+ NULL_RTX, <VM>mode);
+ DONE;
+})
diff --git a/gcc/config/riscv/vector-iterators.md b/gcc/config/riscv/vector-iterators.md
index 42848627c8c..fdb0bfbe3b1 100644
--- a/gcc/config/riscv/vector-iterators.md
+++ b/gcc/config/riscv/vector-iterators.md
@@ -1429,6 +1429,10 @@ (define_code_iterator any_commutative_binop [plus and ior xor
(define_code_iterator any_non_commutative_binop [minus div udiv mod umod])
+(define_code_iterator any_int_binop_no_shift
+ [plus minus and ior xor smax umax smin umin mult div udiv mod umod
+])
+
(define_code_iterator any_immediate_binop [plus minus and ior xor])
(define_code_iterator any_sat_int_binop [ss_plus ss_minus us_plus us_minus])
--
2.40.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] riscv: Split off shift patterns for autovectorization.
2023-05-10 15:24 [PATCH] riscv: Split off shift patterns for autovectorization Robin Dapp
@ 2023-05-10 19:19 ` Palmer Dabbelt
2023-05-10 23:00 ` 钟居哲
2023-05-11 10:33 ` [PATCH v2] RISC-V: " Robin Dapp
0 siblings, 2 replies; 8+ messages in thread
From: Palmer Dabbelt @ 2023-05-10 19:19 UTC (permalink / raw)
To: rdapp.gcc
Cc: gcc-patches, juzhe.zhong, Kito Cheng, collison, jeffreyalaw, rdapp.gcc
On Wed, 10 May 2023 08:24:50 PDT (-0700), rdapp.gcc@gmail.com wrote:
> Hi,
>
> this patch splits off the shift patterns of the binop patterns.
> This is necessary as the scalar shifts require a Pmode operand
> as shift count. To this end, a new iterator any_int_binop_no_shift
> is introduced. At a later point when the binops are split up
> further in commutative and non-commutative patterns (which both
> do not include the shift patterns) we might not need this anymore.
>
> Bootstrapped and regtested.
>
> Regards
> Robin
>
> --
>
> gcc/ChangeLog:
>
> * config/riscv/autovec.md (<optab><mode>3): Add scalar shift
> pattern.
> (v<optab><mode>3): Add vector shift pattern.
> * config/riscv/vector-iterators.md: New iterator.
> ---
> gcc/config/riscv/autovec.md | 40 +++++++++++++++++++++++++++-
> gcc/config/riscv/vector-iterators.md | 4 +++
> 2 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/config/riscv/autovec.md b/gcc/config/riscv/autovec.md
> index 8347e42bb9c..2da4fc67d51 100644
> --- a/gcc/config/riscv/autovec.md
> +++ b/gcc/config/riscv/autovec.md
> @@ -65,7 +65,7 @@ (define_expand "movmisalign<mode>"
>
> (define_expand "<optab><mode>3"
> [(set (match_operand:VI 0 "register_operand")
> - (any_int_binop:VI
> + (any_int_binop_no_shift:VI
> (match_operand:VI 1 "<binop_rhs1_predicate>")
> (match_operand:VI 2 "<binop_rhs2_predicate>")))]
> "TARGET_VECTOR"
> @@ -91,3 +91,41 @@ (define_expand "<optab><mode>3"
> NULL_RTX, <VM>mode);
> DONE;
> })
> +
> +;; =========================================================================
> +;; == Binary integer shifts by scalar.
> +;; =========================================================================
> +
> +(define_expand "<optab><mode>3"
> + [(set (match_operand:VI 0 "register_operand")
> + (any_shift:VI
> + (match_operand:VI 1 "register_operand")
> + (match_operand:<VEL> 2 "csr_operand")))]
I don't think VEL is _wrong_ here, as it's an integer type that's big
enough to hold the shift amount, but we might get some odd generated
code for the QI and HI flavors as we frequently don't handle the shorter
types well.
"csr_operand" does seem wrong, though, as that just accepts constants.
Maybe "arith_operand" is the way to go? I haven't looked at the
V immediates though.
> + "TARGET_VECTOR"
> +{
> + if (!CONST_SCALAR_INT_P (operands[2]))
> + operands[2] = gen_lowpart (Pmode, operands[2]);
> + riscv_vector::emit_len_binop (code_for_pred_scalar
> + (<BINOP_TO_UPPERCASE>, <MODE>mode),
> + operands[0], operands[1], operands[2],
> + NULL_RTX, <VM>mode, Pmode);
> + DONE;
> +})
> +
> +;; =========================================================================
> +;; == Binary integer shifts by vector.
> +;; =========================================================================
> +
> +(define_expand "v<optab><mode>3"
> + [(set (match_operand:VI 0 "register_operand")
> + (any_shift:VI
> + (match_operand:VI 1 "register_operand")
> + (match_operand:VI 2 "vector_shift_operand")))]
> + "TARGET_VECTOR"
> +{
> + riscv_vector::emit_len_binop (code_for_pred
> + (<BINOP_TO_UPPERCASE>, <MODE>mode),
> + operands[0], operands[1], operands[2],
> + NULL_RTX, <VM>mode);
> + DONE;
> +})
> diff --git a/gcc/config/riscv/vector-iterators.md b/gcc/config/riscv/vector-iterators.md
> index 42848627c8c..fdb0bfbe3b1 100644
> --- a/gcc/config/riscv/vector-iterators.md
> +++ b/gcc/config/riscv/vector-iterators.md
> @@ -1429,6 +1429,10 @@ (define_code_iterator any_commutative_binop [plus and ior xor
>
> (define_code_iterator any_non_commutative_binop [minus div udiv mod umod])
>
> +(define_code_iterator any_int_binop_no_shift
> + [plus minus and ior xor smax umax smin umin mult div udiv mod umod
> +])
> +
> (define_code_iterator any_immediate_binop [plus minus and ior xor])
>
> (define_code_iterator any_sat_int_binop [ss_plus ss_minus us_plus us_minus])
> --
> 2.40.0
It'd be great to have test cases for the patterns we're adding, at least
for some of the stickier ones.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Re: [PATCH] riscv: Split off shift patterns for autovectorization.
2023-05-10 19:19 ` Palmer Dabbelt
@ 2023-05-10 23:00 ` 钟居哲
2023-05-11 10:33 ` [PATCH v2] RISC-V: " Robin Dapp
1 sibling, 0 replies; 8+ messages in thread
From: 钟居哲 @ 2023-05-10 23:00 UTC (permalink / raw)
To: palmer, rdapp.gcc
Cc: gcc-patches, kito.cheng, Michael Collison, Jeff Law, rdapp.gcc
[-- Attachment #1: Type: text/plain, Size: 5071 bytes --]
>> I don't think VEL is _wrong_ here, as it's an integer type that's big
>> enough to hold the shift amount, but we might get some odd generated
>> code for the QI and HI flavors as we frequently don't handle the shorter
>> types well.
This implementation has been proved works well in both my downsteam GCC and "rvv-next".
>> "csr_operand" does seem wrong, though, as that just accepts constants.
>> Maybe "arith_operand" is the way to go? I haven't looked at the
>> V immediates though.
"arith_operand" is not correct which is SMALL_OPERND - 12bit operand.
For shift V immediates should be 0 ~ 31 which perfectly match csr_operand.
juzhe.zhong@rivai.ai
From: Palmer Dabbelt
Date: 2023-05-11 03:19
To: rdapp.gcc
CC: gcc-patches; juzhe.zhong; Kito Cheng; collison; jeffreyalaw; rdapp.gcc
Subject: Re: [PATCH] riscv: Split off shift patterns for autovectorization.
On Wed, 10 May 2023 08:24:50 PDT (-0700), rdapp.gcc@gmail.com wrote:
> Hi,
>
> this patch splits off the shift patterns of the binop patterns.
> This is necessary as the scalar shifts require a Pmode operand
> as shift count. To this end, a new iterator any_int_binop_no_shift
> is introduced. At a later point when the binops are split up
> further in commutative and non-commutative patterns (which both
> do not include the shift patterns) we might not need this anymore.
>
> Bootstrapped and regtested.
>
> Regards
> Robin
>
> --
>
> gcc/ChangeLog:
>
> * config/riscv/autovec.md (<optab><mode>3): Add scalar shift
> pattern.
> (v<optab><mode>3): Add vector shift pattern.
> * config/riscv/vector-iterators.md: New iterator.
> ---
> gcc/config/riscv/autovec.md | 40 +++++++++++++++++++++++++++-
> gcc/config/riscv/vector-iterators.md | 4 +++
> 2 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/config/riscv/autovec.md b/gcc/config/riscv/autovec.md
> index 8347e42bb9c..2da4fc67d51 100644
> --- a/gcc/config/riscv/autovec.md
> +++ b/gcc/config/riscv/autovec.md
> @@ -65,7 +65,7 @@ (define_expand "movmisalign<mode>"
>
> (define_expand "<optab><mode>3"
> [(set (match_operand:VI 0 "register_operand")
> - (any_int_binop:VI
> + (any_int_binop_no_shift:VI
> (match_operand:VI 1 "<binop_rhs1_predicate>")
> (match_operand:VI 2 "<binop_rhs2_predicate>")))]
> "TARGET_VECTOR"
> @@ -91,3 +91,41 @@ (define_expand "<optab><mode>3"
> NULL_RTX, <VM>mode);
> DONE;
> })
> +
> +;; =========================================================================
> +;; == Binary integer shifts by scalar.
> +;; =========================================================================
> +
> +(define_expand "<optab><mode>3"
> + [(set (match_operand:VI 0 "register_operand")
> + (any_shift:VI
> + (match_operand:VI 1 "register_operand")
> + (match_operand:<VEL> 2 "csr_operand")))]
I don't think VEL is _wrong_ here, as it's an integer type that's big
enough to hold the shift amount, but we might get some odd generated
code for the QI and HI flavors as we frequently don't handle the shorter
types well.
"csr_operand" does seem wrong, though, as that just accepts constants.
Maybe "arith_operand" is the way to go? I haven't looked at the
V immediates though.
> + "TARGET_VECTOR"
> +{
> + if (!CONST_SCALAR_INT_P (operands[2]))
> + operands[2] = gen_lowpart (Pmode, operands[2]);
> + riscv_vector::emit_len_binop (code_for_pred_scalar
> + (<BINOP_TO_UPPERCASE>, <MODE>mode),
> + operands[0], operands[1], operands[2],
> + NULL_RTX, <VM>mode, Pmode);
> + DONE;
> +})
> +
> +;; =========================================================================
> +;; == Binary integer shifts by vector.
> +;; =========================================================================
> +
> +(define_expand "v<optab><mode>3"
> + [(set (match_operand:VI 0 "register_operand")
> + (any_shift:VI
> + (match_operand:VI 1 "register_operand")
> + (match_operand:VI 2 "vector_shift_operand")))]
> + "TARGET_VECTOR"
> +{
> + riscv_vector::emit_len_binop (code_for_pred
> + (<BINOP_TO_UPPERCASE>, <MODE>mode),
> + operands[0], operands[1], operands[2],
> + NULL_RTX, <VM>mode);
> + DONE;
> +})
> diff --git a/gcc/config/riscv/vector-iterators.md b/gcc/config/riscv/vector-iterators.md
> index 42848627c8c..fdb0bfbe3b1 100644
> --- a/gcc/config/riscv/vector-iterators.md
> +++ b/gcc/config/riscv/vector-iterators.md
> @@ -1429,6 +1429,10 @@ (define_code_iterator any_commutative_binop [plus and ior xor
>
> (define_code_iterator any_non_commutative_binop [minus div udiv mod umod])
>
> +(define_code_iterator any_int_binop_no_shift
> + [plus minus and ior xor smax umax smin umin mult div udiv mod umod
> +])
> +
> (define_code_iterator any_immediate_binop [plus minus and ior xor])
>
> (define_code_iterator any_sat_int_binop [ss_plus ss_minus us_plus us_minus])
> --
> 2.40.0
It'd be great to have test cases for the patterns we're adding, at least
for some of the stickier ones.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] RISC-V: Split off shift patterns for autovectorization.
2023-05-10 19:19 ` Palmer Dabbelt
2023-05-10 23:00 ` 钟居哲
@ 2023-05-11 10:33 ` Robin Dapp
2023-05-11 10:35 ` juzhe.zhong
2023-05-11 14:21 ` Jeff Law
1 sibling, 2 replies; 8+ messages in thread
From: Robin Dapp @ 2023-05-11 10:33 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: gcc-patches, juzhe.zhong, Kito Cheng, collison, jeffreyalaw, rdapp.gcc
> "csr_operand" does seem wrong, though, as that just accepts constants.
> Maybe "arith_operand" is the way to go? I haven't looked at the
> V immediates though.
I was pondering changing the shift-count operand to QImode everywhere
but that indeed does not help code generation across the board. It can
still work but might require extra patterns here and there.
"csr_operand" accepts 0-31 constants as well as registers which should
be fine here.
No changes from v1 apart from the RISC-V in the subject and a bit of
rebasing and comments.
This patch splits off the shift patterns of the binop patterns.
This is necessary as the scalar shifts require a Pmode operand
as shift count. To this end, a new iterator any_int_binop_no_shift
is introduced. At a later point when the binops are split up
further in commutative and non-commutative patterns (which both
do not include the shift patterns) we might not need this anymore.
gcc/ChangeLog:
* config/riscv/autovec.md (<optab><mode>3): Add scalar shift
pattern.
(v<optab><mode>3): Add vector shift pattern.
* config/riscv/vector-iterators.md: New iterator.
---
gcc/config/riscv/autovec.md | 47 +++++++++++++++++++++++++++-
gcc/config/riscv/vector-iterators.md | 4 +++
2 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/gcc/config/riscv/autovec.md b/gcc/config/riscv/autovec.md
index 58926ed3e67..ac0c939d277 100644
--- a/gcc/config/riscv/autovec.md
+++ b/gcc/config/riscv/autovec.md
@@ -97,7 +97,7 @@ (define_expand "@vec_series<mode>"
(define_expand "<optab><mode>3"
[(set (match_operand:VI 0 "register_operand")
- (any_int_binop:VI
+ (any_int_binop_no_shift:VI
(match_operand:VI 1 "<binop_rhs1_predicate>")
(match_operand:VI 2 "<binop_rhs2_predicate>")))]
"TARGET_VECTOR"
@@ -119,3 +119,48 @@ (define_expand "<optab><mode>3"
NULL, <VM>mode);
DONE;
})
+
+;; -------------------------------------------------------------------------
+;; ---- [INT] Binary shifts by scalar.
+;; -------------------------------------------------------------------------
+;; Includes:
+;; - vsll.vx/vsra.vx/vsrl.vx
+;; - vsll.vi/vsra.vi/vsrl.vi
+;; -------------------------------------------------------------------------
+
+(define_expand "<optab><mode>3"
+ [(set (match_operand:VI 0 "register_operand")
+ (any_shift:VI
+ (match_operand:VI 1 "register_operand")
+ (match_operand:<VEL> 2 "csr_operand")))]
+ "TARGET_VECTOR"
+{
+ if (!CONST_SCALAR_INT_P (operands[2]))
+ operands[2] = gen_lowpart (Pmode, operands[2]);
+ riscv_vector::emit_len_binop (code_for_pred_scalar
+ (<CODE>, <MODE>mode),
+ operands[0], operands[1], operands[2],
+ NULL_RTX, <VM>mode, Pmode);
+ DONE;
+})
+
+;; -------------------------------------------------------------------------
+;; ---- [INT] Binary shifts by scalar.
+;; -------------------------------------------------------------------------
+;; Includes:
+;; - vsll.vv/vsra.vv/vsrl.vv
+;; -------------------------------------------------------------------------
+
+(define_expand "v<optab><mode>3"
+ [(set (match_operand:VI 0 "register_operand")
+ (any_shift:VI
+ (match_operand:VI 1 "register_operand")
+ (match_operand:VI 2 "vector_shift_operand")))]
+ "TARGET_VECTOR"
+{
+ riscv_vector::emit_len_binop (code_for_pred
+ (<CODE>, <MODE>mode),
+ operands[0], operands[1], operands[2],
+ NULL_RTX, <VM>mode);
+ DONE;
+})
diff --git a/gcc/config/riscv/vector-iterators.md b/gcc/config/riscv/vector-iterators.md
index 29c9d77674b..5cf958ba845 100644
--- a/gcc/config/riscv/vector-iterators.md
+++ b/gcc/config/riscv/vector-iterators.md
@@ -1409,6 +1409,10 @@ (define_code_iterator any_commutative_binop [plus and ior xor
(define_code_iterator any_non_commutative_binop [minus div udiv mod umod])
+(define_code_iterator any_int_binop_no_shift
+ [plus minus and ior xor smax umax smin umin mult div udiv mod umod
+])
+
(define_code_iterator any_sat_int_binop [ss_plus ss_minus us_plus us_minus])
(define_code_iterator sat_int_plus_binop [ss_plus us_plus])
(define_code_iterator sat_int_minus_binop [ss_minus us_minus])
--
2.40.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] RISC-V: Split off shift patterns for autovectorization.
2023-05-11 10:33 ` [PATCH v2] RISC-V: " Robin Dapp
@ 2023-05-11 10:35 ` juzhe.zhong
2023-05-11 11:10 ` Kito Cheng
2023-05-11 14:21 ` Jeff Law
1 sibling, 1 reply; 8+ messages in thread
From: juzhe.zhong @ 2023-05-11 10:35 UTC (permalink / raw)
To: Robin Dapp, palmer
Cc: gcc-patches, kito.cheng, collison, jeffreyalaw, Robin Dapp
[-- Attachment #1: Type: text/plain, Size: 4518 bytes --]
LGTM. Plz commit it now. Then I can rebase vec_init patch.
juzhe.zhong@rivai.ai
From: Robin Dapp
Date: 2023-05-11 18:33
To: Palmer Dabbelt
CC: gcc-patches; juzhe.zhong; Kito Cheng; collison; jeffreyalaw; rdapp.gcc
Subject: [PATCH v2] RISC-V: Split off shift patterns for autovectorization.
> "csr_operand" does seem wrong, though, as that just accepts constants.
> Maybe "arith_operand" is the way to go? I haven't looked at the
> V immediates though.
I was pondering changing the shift-count operand to QImode everywhere
but that indeed does not help code generation across the board. It can
still work but might require extra patterns here and there.
"csr_operand" accepts 0-31 constants as well as registers which should
be fine here.
No changes from v1 apart from the RISC-V in the subject and a bit of
rebasing and comments.
This patch splits off the shift patterns of the binop patterns.
This is necessary as the scalar shifts require a Pmode operand
as shift count. To this end, a new iterator any_int_binop_no_shift
is introduced. At a later point when the binops are split up
further in commutative and non-commutative patterns (which both
do not include the shift patterns) we might not need this anymore.
gcc/ChangeLog:
* config/riscv/autovec.md (<optab><mode>3): Add scalar shift
pattern.
(v<optab><mode>3): Add vector shift pattern.
* config/riscv/vector-iterators.md: New iterator.
---
gcc/config/riscv/autovec.md | 47 +++++++++++++++++++++++++++-
gcc/config/riscv/vector-iterators.md | 4 +++
2 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/gcc/config/riscv/autovec.md b/gcc/config/riscv/autovec.md
index 58926ed3e67..ac0c939d277 100644
--- a/gcc/config/riscv/autovec.md
+++ b/gcc/config/riscv/autovec.md
@@ -97,7 +97,7 @@ (define_expand "@vec_series<mode>"
(define_expand "<optab><mode>3"
[(set (match_operand:VI 0 "register_operand")
- (any_int_binop:VI
+ (any_int_binop_no_shift:VI
(match_operand:VI 1 "<binop_rhs1_predicate>")
(match_operand:VI 2 "<binop_rhs2_predicate>")))]
"TARGET_VECTOR"
@@ -119,3 +119,48 @@ (define_expand "<optab><mode>3"
NULL, <VM>mode);
DONE;
})
+
+;; -------------------------------------------------------------------------
+;; ---- [INT] Binary shifts by scalar.
+;; -------------------------------------------------------------------------
+;; Includes:
+;; - vsll.vx/vsra.vx/vsrl.vx
+;; - vsll.vi/vsra.vi/vsrl.vi
+;; -------------------------------------------------------------------------
+
+(define_expand "<optab><mode>3"
+ [(set (match_operand:VI 0 "register_operand")
+ (any_shift:VI
+ (match_operand:VI 1 "register_operand")
+ (match_operand:<VEL> 2 "csr_operand")))]
+ "TARGET_VECTOR"
+{
+ if (!CONST_SCALAR_INT_P (operands[2]))
+ operands[2] = gen_lowpart (Pmode, operands[2]);
+ riscv_vector::emit_len_binop (code_for_pred_scalar
+ (<CODE>, <MODE>mode),
+ operands[0], operands[1], operands[2],
+ NULL_RTX, <VM>mode, Pmode);
+ DONE;
+})
+
+;; -------------------------------------------------------------------------
+;; ---- [INT] Binary shifts by scalar.
+;; -------------------------------------------------------------------------
+;; Includes:
+;; - vsll.vv/vsra.vv/vsrl.vv
+;; -------------------------------------------------------------------------
+
+(define_expand "v<optab><mode>3"
+ [(set (match_operand:VI 0 "register_operand")
+ (any_shift:VI
+ (match_operand:VI 1 "register_operand")
+ (match_operand:VI 2 "vector_shift_operand")))]
+ "TARGET_VECTOR"
+{
+ riscv_vector::emit_len_binop (code_for_pred
+ (<CODE>, <MODE>mode),
+ operands[0], operands[1], operands[2],
+ NULL_RTX, <VM>mode);
+ DONE;
+})
diff --git a/gcc/config/riscv/vector-iterators.md b/gcc/config/riscv/vector-iterators.md
index 29c9d77674b..5cf958ba845 100644
--- a/gcc/config/riscv/vector-iterators.md
+++ b/gcc/config/riscv/vector-iterators.md
@@ -1409,6 +1409,10 @@ (define_code_iterator any_commutative_binop [plus and ior xor
(define_code_iterator any_non_commutative_binop [minus div udiv mod umod])
+(define_code_iterator any_int_binop_no_shift
+ [plus minus and ior xor smax umax smin umin mult div udiv mod umod
+])
+
(define_code_iterator any_sat_int_binop [ss_plus ss_minus us_plus us_minus])
(define_code_iterator sat_int_plus_binop [ss_plus us_plus])
(define_code_iterator sat_int_minus_binop [ss_minus us_minus])
--
2.40.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] RISC-V: Split off shift patterns for autovectorization.
2023-05-11 10:35 ` juzhe.zhong
@ 2023-05-11 11:10 ` Kito Cheng
0 siblings, 0 replies; 8+ messages in thread
From: Kito Cheng @ 2023-05-11 11:10 UTC (permalink / raw)
To: 钟居哲
Cc: Robin Dapp, palmer, gcc-patches, collison, jeffreyalaw
[-- Attachment #1: Type: text/plain, Size: 4873 bytes --]
Thanks, LGTM
juzhe.zhong@rivai.ai <juzhe.zhong@rivai.ai> 於 2023年5月11日 週四 18:37 寫道:
> LGTM. Plz commit it now. Then I can rebase vec_init patch.
>
>
>
> juzhe.zhong@rivai.ai
>
> From: Robin Dapp
> Date: 2023-05-11 18:33
> To: Palmer Dabbelt
> CC: gcc-patches; juzhe.zhong; Kito Cheng; collison; jeffreyalaw; rdapp.gcc
> Subject: [PATCH v2] RISC-V: Split off shift patterns for autovectorization.
> > "csr_operand" does seem wrong, though, as that just accepts constants.
> > Maybe "arith_operand" is the way to go? I haven't looked at the
> > V immediates though.
>
> I was pondering changing the shift-count operand to QImode everywhere
> but that indeed does not help code generation across the board. It can
> still work but might require extra patterns here and there.
>
> "csr_operand" accepts 0-31 constants as well as registers which should
> be fine here.
>
> No changes from v1 apart from the RISC-V in the subject and a bit of
> rebasing and comments.
>
>
> This patch splits off the shift patterns of the binop patterns.
> This is necessary as the scalar shifts require a Pmode operand
> as shift count. To this end, a new iterator any_int_binop_no_shift
> is introduced. At a later point when the binops are split up
> further in commutative and non-commutative patterns (which both
> do not include the shift patterns) we might not need this anymore.
>
> gcc/ChangeLog:
>
> * config/riscv/autovec.md (<optab><mode>3): Add scalar shift
> pattern.
> (v<optab><mode>3): Add vector shift pattern.
> * config/riscv/vector-iterators.md: New iterator.
> ---
> gcc/config/riscv/autovec.md | 47 +++++++++++++++++++++++++++-
> gcc/config/riscv/vector-iterators.md | 4 +++
> 2 files changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/config/riscv/autovec.md b/gcc/config/riscv/autovec.md
> index 58926ed3e67..ac0c939d277 100644
> --- a/gcc/config/riscv/autovec.md
> +++ b/gcc/config/riscv/autovec.md
> @@ -97,7 +97,7 @@ (define_expand "@vec_series<mode>"
> (define_expand "<optab><mode>3"
> [(set (match_operand:VI 0 "register_operand")
> - (any_int_binop:VI
> + (any_int_binop_no_shift:VI
> (match_operand:VI 1 "<binop_rhs1_predicate>")
> (match_operand:VI 2 "<binop_rhs2_predicate>")))]
> "TARGET_VECTOR"
> @@ -119,3 +119,48 @@ (define_expand "<optab><mode>3"
> NULL, <VM>mode);
> DONE;
> })
> +
> +;;
> -------------------------------------------------------------------------
> +;; ---- [INT] Binary shifts by scalar.
> +;;
> -------------------------------------------------------------------------
> +;; Includes:
> +;; - vsll.vx/vsra.vx/vsrl.vx
> +;; - vsll.vi/vsra.vi/vsrl.vi
> +;;
> -------------------------------------------------------------------------
> +
> +(define_expand "<optab><mode>3"
> + [(set (match_operand:VI 0 "register_operand")
> + (any_shift:VI
> + (match_operand:VI 1 "register_operand")
> + (match_operand:<VEL> 2 "csr_operand")))]
> + "TARGET_VECTOR"
> +{
> + if (!CONST_SCALAR_INT_P (operands[2]))
> + operands[2] = gen_lowpart (Pmode, operands[2]);
> + riscv_vector::emit_len_binop (code_for_pred_scalar
> + (<CODE>, <MODE>mode),
> + operands[0], operands[1], operands[2],
> + NULL_RTX, <VM>mode, Pmode);
> + DONE;
> +})
> +
> +;;
> -------------------------------------------------------------------------
> +;; ---- [INT] Binary shifts by scalar.
> +;;
> -------------------------------------------------------------------------
> +;; Includes:
> +;; - vsll.vv/vsra.vv/vsrl.vv
> +;;
> -------------------------------------------------------------------------
> +
> +(define_expand "v<optab><mode>3"
> + [(set (match_operand:VI 0 "register_operand")
> + (any_shift:VI
> + (match_operand:VI 1 "register_operand")
> + (match_operand:VI 2 "vector_shift_operand")))]
> + "TARGET_VECTOR"
> +{
> + riscv_vector::emit_len_binop (code_for_pred
> + (<CODE>, <MODE>mode),
> + operands[0], operands[1], operands[2],
> + NULL_RTX, <VM>mode);
> + DONE;
> +})
> diff --git a/gcc/config/riscv/vector-iterators.md
> b/gcc/config/riscv/vector-iterators.md
> index 29c9d77674b..5cf958ba845 100644
> --- a/gcc/config/riscv/vector-iterators.md
> +++ b/gcc/config/riscv/vector-iterators.md
> @@ -1409,6 +1409,10 @@ (define_code_iterator any_commutative_binop [plus
> and ior xor
> (define_code_iterator any_non_commutative_binop [minus div udiv mod umod])
> +(define_code_iterator any_int_binop_no_shift
> + [plus minus and ior xor smax umax smin umin mult div udiv mod umod
> +])
> +
> (define_code_iterator any_sat_int_binop [ss_plus ss_minus us_plus
> us_minus])
> (define_code_iterator sat_int_plus_binop [ss_plus us_plus])
> (define_code_iterator sat_int_minus_binop [ss_minus us_minus])
> --
> 2.40.0
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] RISC-V: Split off shift patterns for autovectorization.
2023-05-11 10:33 ` [PATCH v2] RISC-V: " Robin Dapp
2023-05-11 10:35 ` juzhe.zhong
@ 2023-05-11 14:21 ` Jeff Law
2023-05-11 17:51 ` Palmer Dabbelt
1 sibling, 1 reply; 8+ messages in thread
From: Jeff Law @ 2023-05-11 14:21 UTC (permalink / raw)
To: Robin Dapp, Palmer Dabbelt; +Cc: gcc-patches, juzhe.zhong, Kito Cheng, collison
On 5/11/23 04:33, Robin Dapp wrote:
>> "csr_operand" does seem wrong, though, as that just accepts constants.
>> Maybe "arith_operand" is the way to go? I haven't looked at the
>> V immediates though.
>
> I was pondering changing the shift-count operand to QImode everywhere
> but that indeed does not help code generation across the board. It can
> still work but might require extra patterns here and there.
Yea. It's a GCC wart and there hasn't ever been a clear best direction
on the mode for the shift count. If you use QImode, as you note you
often end up having to add various patterns to avoid useless conversions
and such.
I suspect QImode isn't ideal on a target like RV where we don't really
have QImode operations. So all we do is force the introduction of
subregs all over the place to force the operand in to QImode. It's
something I'd like to explore, but would obviously require a fair amount
of benchmarking to be able to confidently say which is better.
Jeff
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] RISC-V: Split off shift patterns for autovectorization.
2023-05-11 14:21 ` Jeff Law
@ 2023-05-11 17:51 ` Palmer Dabbelt
0 siblings, 0 replies; 8+ messages in thread
From: Palmer Dabbelt @ 2023-05-11 17:51 UTC (permalink / raw)
To: jeffreyalaw; +Cc: rdapp.gcc, gcc-patches, juzhe.zhong, Kito Cheng, collison
On Thu, 11 May 2023 07:21:30 PDT (-0700), jeffreyalaw@gmail.com wrote:
> On 5/11/23 04:33, Robin Dapp wrote:
>>> "csr_operand" does seem wrong, though, as that just accepts constants.
>>> Maybe "arith_operand" is the way to go? I haven't looked at the
>>> V immediates though.
>>
>> I was pondering changing the shift-count operand to QImode everywhere
>> but that indeed does not help code generation across the board. It can
>> still work but might require extra patterns here and there.
> Yea. It's a GCC wart and there hasn't ever been a clear best direction
> on the mode for the shift count. If you use QImode, as you note you
> often end up having to add various patterns to avoid useless conversions
> and such.
Yes, and I think given that we have so much weirdness for the sub-XLEN
types in the RISC-V port we'd need to have a lot of fairly large
patterns and some truncation-based fallbacks. We've got some of those
for the integer shifts already, though, so maybe it's the way to go?
FWIW, I was trying to suggest X or REG as the shift amount and thought
we'd done it that way for the integer shifts too. I think we can
reason about that with just some tiny code snippits, even if it's not
the right way to go long term (as per below). Probably a minor win,
though, and I don't think it needs to block the patches.
Also: looks like I was wrong and "csr_operand" does the correct thing
here because there's only a 5-bit immediate for the shift amounts. We
should probably name it something else, though, as this has nothing to
do with CSRs...
> I suspect QImode isn't ideal on a target like RV where we don't really
> have QImode operations. So all we do is force the introduction of
> subregs all over the place to force the operand in to QImode. It's
> something I'd like to explore, but would obviously require a fair amount
> of benchmarking to be able to confidently say which is better.
Folks have tried a few times and it's never ended up better. I do think
we're at a local minimum here, though -- ie, explicitly handling the
shorter types would result in better generated code if we got everything
right. Gut feeling is that'd require a meaningful amount of middle-end
work, though, as we're sufficiently different than MIPS here (and
arm64/x86 have many of the ops).
Nobody in Rivos land is looking at this right now, though it's a pretty
common red flag for new people and frequently trips up code gen so that
might change with little notice...
> Jeff
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-05-11 17:51 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-10 15:24 [PATCH] riscv: Split off shift patterns for autovectorization Robin Dapp
2023-05-10 19:19 ` Palmer Dabbelt
2023-05-10 23:00 ` 钟居哲
2023-05-11 10:33 ` [PATCH v2] RISC-V: " Robin Dapp
2023-05-11 10:35 ` juzhe.zhong
2023-05-11 11:10 ` Kito Cheng
2023-05-11 14:21 ` Jeff Law
2023-05-11 17:51 ` Palmer Dabbelt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).