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

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

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

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

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