rs6000: Simplify *rotl<mode>3_insert_4 by removing DImode

  Hi,

define_insn *rotl<mode>3_insert_4 use mode iterator GPR which
consists of SImode and conditional DImode, but the condition
of this define_insn requires the mode should be SImode.  By
further checking, it's found that the rldimi instruction can
not be used for this pattern since the required mask can not
be represented correctly.  We can have the fixed mask end 31
with rlwimi, but can not have the fixed mask end with rldimi
as it has to be (63 - SH) always.

So this patch simplifies the define_insn to use SImode only.

Tested on powerpc64-linux-gnu P8 and powerpc64le-linux-gnu P9
and P10.

Is it ok for trunk?

BR,
Kewen
-----
gcc/ChangeLog:

	* config/rs6000/rs6000.md (*rotl<mode>3_insert_4): Replace mode
	iterator GPR with SImode, adjust the condition and output template,
	rename to ...
	(*rotlsi3_insert_4): ... this.
---
 gcc/config/rs6000/rs6000.md | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

--
2.32.0
  

Hi!

On Mon, Jun 27, 2022 at 05:35:03PM +0800, Kewen.Lin wrote:
> -(define_insn "*rotl<mode>3_insert_4"
> -  [(set (match_operand:GPR 0 "gpc_reg_operand" "=r")
> -	(ior:GPR (and:GPR (match_operand:GPR 3 "gpc_reg_operand" "0")
> -			  (match_operand:GPR 4 "const_int_operand" "n"))
> -		 (lshiftrt:GPR (match_operand:GPR 1 "gpc_reg_operand" "r")
> +; It's unable to extend this define_insn for DImode, because the
> +; corresponding hardware instruction for DImode is rldimi, which only has
> +; four operands and the end of mask is always (63 - SH) rather than 63
> +; that is required for this pattern.
> +(define_insn "*rotlsi3_insert_4"
> +  [(set (match_operand:SI 0 "gpc_reg_operand" "=r")
> +	(ior:SI (and:SI (match_operand:SI 3 "gpc_reg_operand" "0")
> +			  (match_operand:SI 4 "const_int_operand" "n"))
> +		 (lshiftrt:SI (match_operand:SI 1 "gpc_reg_operand" "r")
>  			       (match_operand:SI 2 "const_int_operand" "n"))))]

(Broken indentation here, on all of the last three lines differently
wrong:

  [(set (match_operand:SI 0 "gpc_reg_operand" "=r")
	(ior:SI (and:SI (match_operand:SI 3 "gpc_reg_operand" "0")
			(match_operand:SI 4 "const_int_operand" "n"))
		(lshiftrt:SI (match_operand:SI 1 "gpc_reg_operand" "r")
			     (match_operand:SI 2 "const_int_operand" "n"))))]

is better.)

> -  "<MODE>mode == SImode &&
> -   GET_MODE_PRECISION (<MODE>mode)
> -   == INTVAL (operands[2]) + exact_log2 (-UINTVAL (operands[4]))"
> -{
> -  operands[2] = GEN_INT (GET_MODE_PRECISION (<MODE>mode)
> -			 - INTVAL (operands[2]));
> -  if (<MODE>mode == SImode)
> -    return "rlwimi %0,%1,%h2,32-%h2,31";
> -  else
> -    return "rldimi %0,%1,%H2,64-%H2";
> -}
> +  "INTVAL (operands[2]) + exact_log2 (-UINTVAL (operands[4])) == 32"
> +  "rlwimi %0,%1,32-%h2,%h2,31"
>    [(set_attr "type" "insert")])

I was going to say this is not an improvement at all, because now you
need that big comment.  There are many tricky details here, more
important ones than the comment talks about.  It is better to show such
things in the code instead.

But it already does show that :-)  The patch is okay with that four-line
comment deleted (and the indentation fixed).  Thanks!


Segher
  

Hi Segher!

on 2022/6/28 00:02, Segher Boessenkool wrote:
> Hi!
> 
> On Mon, Jun 27, 2022 at 05:35:03PM +0800, Kewen.Lin wrote:
>> -(define_insn "*rotl<mode>3_insert_4"
>> -  [(set (match_operand:GPR 0 "gpc_reg_operand" "=r")
>> -	(ior:GPR (and:GPR (match_operand:GPR 3 "gpc_reg_operand" "0")
>> -			  (match_operand:GPR 4 "const_int_operand" "n"))
>> -		 (lshiftrt:GPR (match_operand:GPR 1 "gpc_reg_operand" "r")
>> +; It's unable to extend this define_insn for DImode, because the
>> +; corresponding hardware instruction for DImode is rldimi, which only has
>> +; four operands and the end of mask is always (63 - SH) rather than 63
>> +; that is required for this pattern.
>> +(define_insn "*rotlsi3_insert_4"
>> +  [(set (match_operand:SI 0 "gpc_reg_operand" "=r")
>> +	(ior:SI (and:SI (match_operand:SI 3 "gpc_reg_operand" "0")
>> +			  (match_operand:SI 4 "const_int_operand" "n"))
>> +		 (lshiftrt:SI (match_operand:SI 1 "gpc_reg_operand" "r")
>>  			       (match_operand:SI 2 "const_int_operand" "n"))))]
> 
> (Broken indentation here, on all of the last three lines differently
> wrong:
> 
>   [(set (match_operand:SI 0 "gpc_reg_operand" "=r")
> 	(ior:SI (and:SI (match_operand:SI 3 "gpc_reg_operand" "0")
> 			(match_operand:SI 4 "const_int_operand" "n"))
> 		(lshiftrt:SI (match_operand:SI 1 "gpc_reg_operand" "r")
> 			     (match_operand:SI 2 "const_int_operand" "n"))))]
> 
> is better.)
> 

Thanks for catching, I forgot to re-indent after substituting. :(

>> -  "<MODE>mode == SImode &&
>> -   GET_MODE_PRECISION (<MODE>mode)
>> -   == INTVAL (operands[2]) + exact_log2 (-UINTVAL (operands[4]))"
>> -{
>> -  operands[2] = GEN_INT (GET_MODE_PRECISION (<MODE>mode)
>> -			 - INTVAL (operands[2]));
>> -  if (<MODE>mode == SImode)
>> -    return "rlwimi %0,%1,%h2,32-%h2,31";
>> -  else
>> -    return "rldimi %0,%1,%H2,64-%H2";
>> -}
>> +  "INTVAL (operands[2]) + exact_log2 (-UINTVAL (operands[4])) == 32"
>> +  "rlwimi %0,%1,32-%h2,%h2,31"
>>    [(set_attr "type" "insert")])
> 
> I was going to say this is not an improvement at all, because now you
> need that big comment.  There are many tricky details here, more
> important ones than the comment talks about.  It is better to show such
> things in the code instead.
> 
> But it already does show that :-)  The patch is okay with that four-line
> comment deleted (and the indentation fixed).  Thanks!
> 

Thanks!  It's adjusted as your comments and committed in r13-1313.

BR,
Kewen