Re: [PATCH] rs6000: Enable generate const through pli+pli+rldimi

[Jiufu Guo <guojiufu@linux.ibm.com>]

Hi,

Segher Boessenkool <segher@kernel.crashing.org> writes:
> Hi!
>
> On Wed, Aug 10, 2022 at 03:11:23PM +0800, Jiufu Guo wrote:
>> As mentioned in PR106550, since pli could support 34bits immediate, we could
>> use less instructions(3insn would be ok) to build 64bits constant with pli.
>> 
>> For example, for constant 0x020805006106003, we could generate it with:
>> asm code1:
>> pli 9,101736451 (0x6106003)
>> sldi 9,9,32
>> paddi 9,9, 2130000 (0x0208050)
>> 
>> or asm code2:
>> pli 10, 2130000
>> pli 9, 101736451
>> rldimi 9, 10, 32, 0
>> 
>> If there is only one register can be used, then the asm code1 is ok. Otherwise
>> asm code2 may be better.
>
> It is significantly better yes.  That code with sldi is perhaps what we
> have to do after reload, but all those three insns are sequential,
> expensive.

>
>> This patch re-enable the constant building(splitter) before RA by updating the
>> constrains from int_reg_operand_not_pseudo to gpc_reg_operand.  And then, we
>> could use two different pseduo for two pli(s), and asm code2 can be generated.
>
>> This patch also could generate asm code1 if hard register is allocated for the
>> constant.
>
>> +  else if (TARGET_PREFIXED)
>> +    {
>> +      /* pli 9,high32 + pli 10,low32 + rldimi 9,10,32,0.  */
>> +      if (can_create_pseudo_p ())
>> +	{
>> +	  temp = gen_reg_rtx (DImode);
>> +	  rtx temp1 = gen_reg_rtx (DImode);
>> +	  emit_move_insn (copy_rtx (temp), GEN_INT ((ud4 << 16) | ud3));
>> +	  emit_move_insn (copy_rtx (temp1), GEN_INT ((ud2 << 16) | ud1));
>> +
>> +	  rtx one = gen_rtx_AND (DImode, temp1, GEN_INT (0xffffffff));
>
> Why do you meed to mask the value here?  That is a nop, no?
As you mentioned, this is not needed if using gen_rotldi3_insert_3.
>
>> +	  rtx two = gen_rtx_ASHIFT (DImode, temp, GEN_INT (32));
>> +	  emit_move_insn (dest, gen_rtx_IOR (DImode, one, two));
>
> But you can call gen_rotldi3_insert_3 explicitly, a better idea if this
> code can run late (so we cannot rely on other optimisations to clean
> things up).
Thanks! Using gen_rotldi3_insert_3 would indicate the rotate behavior
explicitly.

emit_insn (gen_rotldi3_insert_3 (dest, temp, GEN_INT (32), temp1,
                                GEN_INT (0xffffffff)));

>
>> --- a/gcc/config/rs6000/rs6000.md
>> +++ b/gcc/config/rs6000/rs6000.md
>> @@ -9659,7 +9659,7 @@ (define_split
>>  ;; When non-easy constants can go in the TOC, this should use
>>  ;; easy_fp_constant predicate.
>>  (define_split
>> -  [(set (match_operand:DI 0 "int_reg_operand_not_pseudo")
>> +  [(set (match_operand:DI 0 "gpc_reg_operand")
>>  	(match_operand:DI 1 "const_int_operand"))]
>>    "TARGET_POWERPC64 && num_insns_constant (operands[1], DImode) > 1"
>>    [(set (match_dup 0) (match_dup 2))
>
> This is a huge change.  Do you have some indication that it helps /
> hurts / is neutral?  Some reasoning why it is a good idea?
Thanks for this concern! I would do more check/test for this.

The 'int_reg_operand_not_pseudo' cause this splitter only work after RA.
Using 'int_reg_operand_not_pseudo', the code sequence "pli+sldi+paddi"
can be used.

>
> I am not against it, but some more rationale would be good :-)
>
> Btw, this splitter uses operands[2] and [3] in the replacement, and
> neither of those exists.  The replacement never is used of course.
> Instead, rs6000_emit_set_const is called always.  It would be less
> misleading if the replacement text was just "(pc)" or such.

Right, "(pc)" would be better to avoid misleading.

BR,
Jeff(Jiufu)
>
>
> Segher