Re: [PATCH V4] rs6000: Optimize cmp on rotated 16bits constant

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

Hi,

Segher Boessenkool <segher@kernel.crashing.org> writes:

> Hi!
>
> On Thu, Aug 25, 2022 at 08:11:31PM +0800, Jiufu Guo wrote:
>> Segher Boessenkool <segher@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