Re: RISC-V: Remove masking third operand of rotate instructions

[Jeff Law <jeffreyalaw@gmail.com>]

On 5/10/23 09:50, Jivan Hakobyan via Gcc-patches wrote:
> Subject:
> RISC-V: Remove masking third operand of rotate instructions
> From:
> Jivan Hakobyan via Gcc-patches <gcc-patches@gcc.gnu.org>
> Date:
> 5/10/23, 09:50
> 
> To:
> gcc-patches@gcc.gnu.org
> 
> 
> Rotate instructions do not need to mask the third operand.
> For example  RV64 the following code:
> 
> unsigned long foo1(unsigned long rs1, unsigned long rs2)
> {
>      long shamt = rs2 & (64 - 1);
>      return (rs1 << shamt) | (rs1 >> ((64 - shamt) & (64 - 1)));
> }
> 
> Compiles to:
> foo1:
>          andi    a1,a1,63
>          rol     a0,a0,a1
>          ret
> 
> This patch removes unnecessary masking.
> Besides, I have merged masking insns for shifts that were written before.
> 
> 
> gcc/ChangeLog:
>          * config/riscv/riscv.md: Merged
>          * config/riscv/bitmanip.md: New insns
>          * config/riscv/iterators.md: New iterator and optab items
>          * config/riscv/predicates.md: New predicates
Usually we try to mention the patterns that got changed.  So something 
like this

	* config/riscv/riscv.md (*<optab><mode>3_mask):  New pattern,
	combined from....
	(*<optab>si3_mask, *<optab>di3_mask): Here.

Similarly for the other patterns in riscv.md that you combined.

For the bitmanip ChangeLog it might look like

	* config/riscv/bitmanip.md (*<bitmanip_optab><mode>3_mask): New
	pattern.
	(*<bitmanip_optab>si3_sext_mask): Likewise.


	* config/riscv/iterators.md (shiftm1): Generalize to handle more
	masking constants.
	(bitmanip_rotate): New iterator.
	(bitmanip_optab): Add rotates.

	* config/riscv/predicates.md (const_si_mask_operand): Renamed
	from const31_operand.  Generalize to handle more mask constants.
	(const_di_mask_operand): Similarly.


> 
> 
> -- With the best regards Jivan Hakobyan
> 
> 
> rotate_mask.patch
> 
> diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
> index a27fc3e34a1..0fd0cbdeb04 100644
> --- a/gcc/config/riscv/bitmanip.md
> +++ b/gcc/config/riscv/bitmanip.md
> @@ -351,6 +351,42 @@
>     "rolw\t%0,%1,%2"
>     [(set_attr "type" "bitmanip")])
>   
> +(define_insn_and_split "*<bitmanip_optab><mode>3_mask"
> +  [(set (match_operand:X     0 "register_operand" "= r")
> +        (bitmanip_rotate:X
> +            (match_operand:X 1 "register_operand" "  r")
> +            (match_operator 4 "subreg_lowpart_operator"
> +             [(and:X
> +               (match_operand:X 2 "register_operand"  "r")
> +               (match_operand 3 "<X:shiftm1>" "<X:shiftm1p>"))])))]
> +  "TARGET_ZBB || TARGET_ZBKB"
> +  "#"
> +  "&& 1"
> +  [(set (match_dup 0)
> +        (bitmanip_rotate:X (match_dup 1)
> +                           (match_dup 2)))]
> +  "operands[2] = gen_lowpart (QImode, operands[2]);"
> +  [(set_attr "type" "bitmanip")
> +   (set_attr "mode" "<X:MODE>")])
It's worth noting that by using a subreg_lowpart_operator in this manner 
we can match any narrowing subreg lowpart rather than being restricted 
to QImode.  Clever use of iterators for the predicate and constraints on 
operand 3 as well.






> diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
> index c508ee3ad89..777d9468efa 100644
> --- a/gcc/config/riscv/riscv.md
> +++ b/gcc/config/riscv/riscv.md
> @@ -2010,44 +2010,23 @@
>     [(set_attr "type" "shift")
>      (set_attr "mode" "SI")])
>   
> -(define_insn_and_split "*<optab>si3_mask"
> -  [(set (match_operand:SI     0 "register_operand" "= r")
> -	(any_shift:SI
> -	    (match_operand:SI 1 "register_operand" "  r")
> +(define_insn_and_split "*<optab><mode>3_mask"
> +  [(set (match_operand:X     0 "register_operand" "= r")
> +	(any_shift:X
> +	    (match_operand:X 1 "register_operand" "  r")
>   	    (match_operator 4 "subreg_lowpart_operator"
>   	     [(and:SI
>   	       (match_operand:SI 2 "register_operand"  "r")
> -	       (match_operand 3 "const_int_operand"))])))]
Shouldn't the mode of operand 2 change to "X" as well?





> -(define_insn_and_split "*<optab>di3_mask_1"
> -  [(set (match_operand:DI     0 "register_operand" "= r")
> -	(any_shift:DI
> -	    (match_operand:DI 1 "register_operand" "  r")
> +(define_insn_and_split "*<optab><mode>3_mask_1"
> +  [(set (match_operand:GPR     0 "register_operand" "= r")
> +	(any_shift:GPR
> +	    (match_operand:GPR 1 "register_operand" "  r")
>   	    (match_operator 4 "subreg_lowpart_operator"
>   	     [(and:DI
>   	       (match_operand:DI 2 "register_operand"  "r")
> -	       (match_operand 3 "const_int_operand"))])))]
Presumably we use GPR here because for TARGET_64BIT we can match both 
the 32bit and 64bit opcodes?  I was wondering if we should do the same 
for the rotate patterns -- use the GPR iterator rather than the X 
iterator to match rol[w] and ror[w].

Overall it looks really good.  Both in terms of improving code 
generation for the rotates and cleaning up the shift patterns a bit too. 
  Just a couple questions/cleanups and an improved ChangeLog and this 
should be good to go.

jeff