Hi,
Segher Boessenkool <seg...@kernel.crashing.org> writes:
> Hi!
>
> On Thu, Aug 25, 2022 at 08:11:31PM +0800, Jiufu Guo wrote:
>> Segher Boessenkool <seg...@kernel.crashing.org> writes:
>> > You usually can split fine if you cannot create new pseudos, by reusing
>> > existing registers.
>> >
>> > FAIL will cause an ICE: the RTL instruction does match, but will fail
>> > when trying to generate machine code for it.
>> >
>> Previous patch is using "gen_reg_rtx (DImode)" to generate a pseudo for
>> the rotated result to prevent orignal one being changed accidently.
>> So, an 'assert (can_create_pseudo_p ())' would catch it in after RA.
>
> It sounds like you want a define_split, not a define_insn_and_split.
> That is much more stomachable anyway.
>
Thanks for pointing out this!
As you mentioned, since it is only 'combine pass' that can match the
patterns, it would be better to just a define_split. While I tried to
use this way, like:
(define_split
[(set (pc)
(if_then_else (eqne (match_operand:DI 1 "gpc_reg_operand")
(match_operand:DI 2 "const_int_operand"))
(label_ref (match_operand 0))
(pc)))]
"TARGET_POWERPC64 && num_insns_constant (operands[2], DImode) > 1
&& compare_rotate_immediate_p (UINTVAL (operands[2]))"
[(pc)]
But this does not work. With more debugging, it seems that,
"combine_split_insns/split_insns" returns correctly with sequence of
three insns. But after return, only less than two insns can be
handled. Just as the code comment:
If we were combining three insns and the result is a simple SET
with no ASM_OPERANDS that wasn't recognized, try to split it into two
insns.
then, that 'define_split' fail to keep the result.
In the patch, for 'define_insn_and_split', it is handled as the
process:
In 'combine' pass, the new defined insns "rotate_on_cmpdi" is combined
from three instructions;
And then, in the 'split1' pass, it was split into other three insns.
> Anything that creates conditional branches together with compars insns
> belongs before RA, before sched1 even.
>
For this patch, it would run in 'split1' mostly. The good thing is
'split1' is before sched1. :-)
>> To enable this splitter works after RA, we may need to reserve one
>> register (clobber would be ok). Such as below:
>>
>> [(set (pc)
>> (if_then_else (eqne (match_operand:DI 1 "gpc_reg_operand" "r")
>> (match_operand:DI 2 "const_int_operand" "n"))
>> (label_ref (match_operand 0 ""))
>> (pc)))
>> (clobber (match_scratch:DI 3 "=r"))
>> (clobber (match_scratch:CCUNS 4 "=y"))]
>
> Yes, that is one way to do it. Another way is to reuse operand 1. A
> clobber is probably better in this case though :-)
Yes, a clobber would be better -:) For example:
If %3 is used later, it would be not safe to change:
"%3:DI!=0x8642000000000000"==>"%3:DI=%3DI<-15, %3:DI!=0x4321"
>
> If this is only so combine can match things, you certainly want just a
> define_split, and the compare+branch in one pattern is not as bad
> then.
As the above comments, since I failed to use 'define_split', so in
patch, 'define_insn_and_split' is used. :(
BR,
Jeff(Jiufu)
>
>
> Segher
