Hi,
Segher Boessenkool <seg...@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
