Re: [rs6000, patch] Enable have_cbranchcc4 on rs6000

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi guys,

On Tue, Nov 15, 2022 at 10:04:04PM -0500, David Edelsohn wrote:
> On Tue, Nov 15, 2022 at 9:32 PM HAO CHEN GUI <guihaoc@linux.ibm.com> wrote:
> >   The patch enables have_cbrnachcc4 which is a flag in ifcvt.cc to
> > indicate if branch by CC bits is invalid or not. As rs6000 already has
> > "*cbranch" insn which does branching according to CC bits, the flag
> > should be enabled and relevant branches can be optimized out. The test
> > case illustrates the optimization.

cbranch<mode>4 normally compares the <mode>mode operands 2 and 3, and
jumps to operands[0] if operands[1] (a relation of operands 2 and 3) is
true.  See emit_cmp_and_jump_insns for example.  But cbranchcc4 does
not do any comparison; instead, it branches based on the result of a
previous comparison, stored in operands[2], and operands[3] has to be
const_int 0.

This really should never have been done this way, because now almost
everywhere we need exception code for MODE_CC things here (and always
do only the exceptional case on targets that have no use for actual
compare-and-branch patterns).

Another appearance of this (last stage 4!) was PR104335.  We never
followed up on that, probably should have, sigh.

So, cbranch<mode>4 now means "compare and branch", except when <mode> is
cc, in which case it means "conditional branch", and operands[3] is
required to be const_int 0 in that case, and if not we ICE, or worse, do
the wrong thing.  Woohoo.

> >   "*cbranch" is an anonymous insn which can't be generated directly.

It can be, rs6000_emit_cbranch (which does mean "conditional branch"
btw).  All RTL can be created out of thin air.

But you mean "can't be generated by name"?  Yup.

> > So changing "const_int 0" to the third operand predicated by
> > "zero_constant" won't cause ICEs as orginal patterns still can be matched.

It never is valid to have an actual comparison of anything MODE_CC with
anything else, only MODE_INT, MODE_FLOAT, MODE_DECIMAL_FLOAT have a
(total) order.  The (const_int 0) in such MODE_CC RTL is only notational
convenience, it does not mean integer 0 or anything else, it is a
placeholder so things can look like a relation.

> > -(define_insn "*cbranch"
> > +(define_insn "cbranchcc4"
> >    [(set (pc)
> >         (if_then_else (match_operator 1 "branch_comparison_operator"
> >                                       [(match_operand 2 "cc_reg_operand" "y")
> > -                                      (const_int 0)])
> > +                                      (match_operand 3 "zero_constant")])
> >                       (label_ref (match_operand 0))
> >                       (pc)))]
> >    ""

> Shouldn't cbranchcc4 be a separate pattern?  This pattern has an existing
> purpose and an expected ordering of operands.

It still has the same operands, only operands[3] is added.  Nothing can
create the old pattern by name (because its name is starred), so the
expander won't ever access operands[3] while there are only 3 operands
passed -- simply because the expander cannot be called in any way :-)

> cbranchcc4 is passed the comparison operator as operand 0.  Other ports
> ignore the second comparison operator and use (const_int 0).  Operand 3 is
> the label, which seems to be the reason that you needed to change it to
> match_operand 3.

operands[0] is the dest, operands[1] is the relation, operands[2] is the
cc reg, operands[3] is const_int 0.  The dest is last in the pattern, to
make it more confusing to read :-)

> It's great to add cbranchcc4 to the Power port where it definitely was an
> omission, but adapting *cbranch for that purpose is the wrong approach.
> The changes to the pattern are incorrect because they are covering up a
> difference in ordering of the operands.  One can argue that the named
> pattern only enables the functionality in ifcvt and the pattern otherwise
> is used in its previous role.  But this is a Frankenstein monster
> approach.

I agree with that!  But the generic code adopted that, and other ports
followed it as well, so who are we to disdain this nice performance
improvement?

> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/powerpc/cbranchcc4.c
> > @@ -0,0 +1,8 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -fdump-rtl-ce1" } */
> > +/* { dg-final {scan-rtl-dump "noce_try_store_flag_constants" "ce1" } } */

(space after { please?  For consistency.)

> > +int test (unsigned int a, unsigned int b)
> > +{
> > +    return (a < b ? 0 : (a > b ? 2 : 1));
> > +}

Please add a comment what this test wants to test for?  Something like
/* If-conversion should <something here>.  */
maybe?


I would approve this patch, given that the compiler will immediately ICE
if something not a zero is passed as operands[3] (although it would be
nice to have a zero_int_operand that would aonly allow const_int).
David, do you agree given all of the above?

Thanks,


Segher