[rs6000] Enable have_cbranchcc4 on rs6000

Hi,
  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" is an anonymous insn which can't be generated directly.
So changing "const_int 0" to the third operand predicated by
"zero_constant" won't cause ICEs as orginal patterns still can be matched.

  Bootstrapped and tested on powerpc64-linux BE and LE with no regressions.
Is this okay for trunk? Any recommendations? Thanks a lot.


ChangeLog
2022-11-16  Haochen Gui <guihaoc@linux.ibm.com>

gcc/
	* config/rs6000/rs6000.md (*cbranch): Rename to...
	(cbranchcc4): ...this, and set const_int 0 to the third operand.

gcc/testsuite/
	* gcc.target/powerpc/cbranchcc4.c: New.


patch.diff

On Tue, Nov 15, 2022 at 9:32 PM HAO CHEN GUI <guihaoc@linux.ibm.com> wrote:

> Hi,
>   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" is an anonymous insn which can't be generated directly.
> So changing "const_int 0" to the third operand predicated by
> "zero_constant" won't cause ICEs as orginal patterns still can be matched.
>
>   Bootstrapped and tested on powerpc64-linux BE and LE with no regressions.
> Is this okay for trunk? Any recommendations? Thanks a lot.
>
>
> ChangeLog
> 2022-11-16  Haochen Gui <guihaoc@linux.ibm.com>
>
> gcc/
>         * config/rs6000/rs6000.md (*cbranch): Rename to...
>         (cbranchcc4): ...this, and set const_int 0 to the third operand.
>
> gcc/testsuite/
>         * gcc.target/powerpc/cbranchcc4.c: New.
>
>
> patch.diff
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index e9e5cd1e54d..ee171f21f6a 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -13067,11 +13067,11 @@ (define_insn_and_split "*<code><mode>_cc"
>  ;; Conditional branches.
>  ;; These either are a single bc insn, or a bc around a b.
>
> -(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.

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.

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.  You're trying to twist the existing pattern so that it triggers
as cbranchcc4, but creating a pattern that messes up its arguments and only
works because the new, named pattern never is called.  This is too ugly.
Please fix.

Thanks, David

diff --git a/gcc/testsuite/gcc.target/powerpc/cbranchcc4.c
> b/gcc/testsuite/gcc.target/powerpc/cbranchcc4.c
> new file mode 100644
> index 00000000000..1751d274bbf
> --- /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" } } */
> +
> +int test (unsigned int a, unsigned int b)
> +{
> +    return (a < b ? 0 : (a > b ? 2 : 1));
> +}
>

Hi David,
  I found definition of the operands in 'cbranch'. The argumnets matters.
I will create a new expand pattern for cbranchcc4. Thanks a lot for your
comments.

'cbranchmode4’
Conditional branch instruction combined with a compare instruction.
Operand 0 is a comparison operator. Operand 1 and operand 2 are the
first and second operands of the comparison, respectively. Operand 3
is the code_label to jump to.

Gui Haochen
Thanks

在 2022/11/16 11:04, David Edelsohn 写道:
> 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.  You're trying to twist the existing pattern so that it triggers as cbranchcc4, but creating a pattern that messes up its arguments and only works because the new, named pattern never is called.  This is too ugly.  Please fix.

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

On Wed, Nov 16, 2022 at 06:06:19AM -0600, Segher Boessenkool wrote:
> 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.

Apparently the operand order is different?  Where is that documented at
all anyway?  Grr.  Please adjust my comments to taste :-/


Segher

Hao

cbranchcc4 is a named pattern and requires a specific operand ordering.  If
you change *cbranch to cbranchcc4, you must change the order of the
operands, not a quick and dirty hack to *cbranch.  Also, you should change
*cbranch_2insn and *creturn as well so that all of the patterns are
consistent.

See for example the aarch64.md implementation.  and the documentation in
Standard Names

https://gcc.gnu.org/onlinedocs/gccint/Standard-Names.html

which mentions cbranch<mode>4 and, briefly, cbranchcc4.

You seemed to want to make the minimal change so that the pattern would
work with ifcvt without considering the impact on the existing pattern and
without understanding what a named pattern with specific operands really
means.  You changed the pattern predicate so that the operands in the wrong
positions would match the pattern.

Thanks, David

On Wed, Nov 16, 2022 at 12:56 AM HAO CHEN GUI <guihaoc@linux.ibm.com> wrote:

> Hi David,
>   I found definition of the operands in 'cbranch'. The argumnets matters.
> I will create a new expand pattern for cbranchcc4. Thanks a lot for your
> comments.
>
> 'cbranchmode4’
> Conditional branch instruction combined with a compare instruction.
> Operand 0 is a comparison operator. Operand 1 and operand 2 are the
> first and second operands of the comparison, respectively. Operand 3
> is the code_label to jump to.
>
> Gui Haochen
> Thanks
>
> 在 2022/11/16 11:04, David Edelsohn 写道:
> > 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.  You're trying to twist the existing pattern so that it triggers
> as cbranchcc4, but creating a pattern that messes up its arguments and only
> works because the new, named pattern never is called.  This is too ugly.
> Please fix.
>