Re: [rs6000, patch] Enable have_cbranchcc4 on rs6000

[David Edelsohn <dje.gcc@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3404 bytes --]

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));
> +}
>