On Wed, Aug 24, 2022 at 03:48:49PM +0800, Jiufu Guo wrote: > Segher Boessenkool <seg...@kernel.crashing.org> writes: > >> + "TARGET_POWERPC64 && !reload_completed && can_create_pseudo_p () > > > > reload_completed in splitters is almost always wrong. It isn't any > > better if it is in the insn condition of a define_insn_and_split :-) > > > Thanks, 'can_create_pseudo_p' would be ok for this patch. > Or just FAIL, if !can_create_pseudo_p()?
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. > >> + && num_insns_constant (operands[2], DImode) > 1 > >> + && (rotate_from_leading_zeros_const (~UINTVAL (operands[2]), 49) > 0 > >> + || rotate_from_leading_zeros_const (UINTVAL (operands[2]), 48) > > >> 0)" > > There must be a better way to describe this. > Will update this. I'm thinking to replace this with a meaning function, > maybe 'compare_rotate_immediate_p'. Thanks! > > Why is this doing a conditional branch at all? Unpredictable > > conditional branches are extremely costly. > This optimization needs to check whether the comparison code is ne/eq or > not. To get the comparison code, we need to check the parent insn of > the 'cmp' insn. This is why conditional branch patterns in used here. > > This patch should not change the information (about prediction) of the > branch insn. I'm thinking of updating the patch to keep the 'note info > REG_BR_PROB' for the branch instruction. Ah, good. Explain a bit about that? In a code comment or in the commit message, whichever works best here. Thanks! Segher
