Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn

  Hi,
  I want to enable "have_cbranchcc4" on rs6000. But not all combinations of
comparison codes and sub CC modes are benefited to generate cbranchcc4 insns
on rs6000. There is an predicate for operand0 of cbranchcc4 to bypass
some combinations. It gets assertion failure in prepare_cmp_insn. I think
we shouldn't suppose that all comparison codes and sub CC modes are supported
and throw an assertion failure in prepare_cmp_insn. It might check the
predicate and go to fail if the predicate can't be satisfied. This patch
changes the behavior of those codes.

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


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

gcc/
	* optabs.cc (prepare_cmp_insn): Go to fail other than assert it when
	predicate check of "cbranchcc4" operand[0] fails.

patch.diff
  

Hi,
   Gentle ping this:
https://gcc.gnu.org/pipermail/gcc-patches/2022-November/607083.html
Thanks
Gui Haochen

在 2022/11/23 10:54, HAO CHEN GUI 写道:
> Hi,
>   I want to enable "have_cbranchcc4" on rs6000. But not all combinations of
> comparison codes and sub CC modes are benefited to generate cbranchcc4 insns
> on rs6000. There is an predicate for operand0 of cbranchcc4 to bypass
> some combinations. It gets assertion failure in prepare_cmp_insn. I think
> we shouldn't suppose that all comparison codes and sub CC modes are supported
> and throw an assertion failure in prepare_cmp_insn. It might check the
> predicate and go to fail if the predicate can't be satisfied. This patch
> changes the behavior of those codes.
> 
>   Bootstrapped and tested on powerpc64-linux BE/LE and x86 with no regressions.
> Is this okay for trunk? Any recommendations? Thanks a lot.
> 
> 
> ChangeLog
> 2022-11-23  Haochen Gui <guihaoc@linux.ibm.com>
> 
> gcc/
> 	* optabs.cc (prepare_cmp_insn): Go to fail other than assert it when
> 	predicate check of "cbranchcc4" operand[0] fails.
> 
> patch.diff
> diff --git a/gcc/optabs.cc b/gcc/optabs.cc
> index 165f8d1fa22..3ec8f6b17ba 100644
> --- a/gcc/optabs.cc
> +++ b/gcc/optabs.cc
> @@ -4484,8 +4484,9 @@ prepare_cmp_insn (rtx x, rtx y, enum rtx_code comparison, rtx size,
>      {
>        enum insn_code icode = optab_handler (cbranch_optab, CCmode);
>        test = gen_rtx_fmt_ee (comparison, VOIDmode, x, y);
> -      gcc_assert (icode != CODE_FOR_nothing
> -                  && insn_operand_matches (icode, 0, test));
> +      gcc_assert (icode != CODE_FOR_nothing);
> +      if (!insn_operand_matches (icode, 0, test))
> +	goto fail;
>        *ptest = test;
>        return;
>      }
  

Add more experts in CC.

on 2022/11/23 10:54, HAO CHEN GUI wrote:
> Hi,
>   I want to enable "have_cbranchcc4" on rs6000. But not all combinations of
> comparison codes and sub CC modes are benefited to generate cbranchcc4 insns
> on rs6000. There is an predicate for operand0 of cbranchcc4 to bypass
> some combinations. It gets assertion failure in prepare_cmp_insn. I think
> we shouldn't suppose that all comparison codes and sub CC modes are supported
> and throw an assertion failure in prepare_cmp_insn. It might check the
> predicate and go to fail if the predicate can't be satisfied. This patch
> changes the behavior of those codes.
> 
>   Bootstrapped and tested on powerpc64-linux BE/LE and x86 with no regressions.
> Is this okay for trunk? Any recommendations? Thanks a lot.
> 
> 
> ChangeLog
> 2022-11-23  Haochen Gui <guihaoc@linux.ibm.com>
> 
> gcc/
> 	* optabs.cc (prepare_cmp_insn): Go to fail other than assert it when
> 	predicate check of "cbranchcc4" operand[0] fails.
> 
> patch.diff
> diff --git a/gcc/optabs.cc b/gcc/optabs.cc
> index 165f8d1fa22..3ec8f6b17ba 100644
> --- a/gcc/optabs.cc
> +++ b/gcc/optabs.cc
> @@ -4484,8 +4484,9 @@ prepare_cmp_insn (rtx x, rtx y, enum rtx_code comparison, rtx size,
>      {
>        enum insn_code icode = optab_handler (cbranch_optab, CCmode);
>        test = gen_rtx_fmt_ee (comparison, VOIDmode, x, y);
> -      gcc_assert (icode != CODE_FOR_nothing
> -                  && insn_operand_matches (icode, 0, test));
> +      gcc_assert (icode != CODE_FOR_nothing);
> +      if (!insn_operand_matches (icode, 0, test))
> +	goto fail;


IMHO, this change looks to accord with the other code in prepare_cmp_insn, which
allows the preparation to fail with NULL_RTX ptest.  Its caller can make its own
decision (ICE due to unexpected, or try other ways) when ptest is null.

If this direction is sensible, maybe we can make it goto fail too if the icode ==
CODE_FOR_nothing, since we already try to relax the restriction.

BR,
Kewen

>        *ptest = test;
>        return;
>      }
  

On Mon, Nov 28, 2022 at 6:33 AM HAO CHEN GUI via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi,
>    Gentle ping this:
> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/607083.html
> Thanks
> Gui Haochen
>
> 在 2022/11/23 10:54, HAO CHEN GUI 写道:
> > Hi,
> >   I want to enable "have_cbranchcc4" on rs6000. But not all combinations of
> > comparison codes and sub CC modes are benefited to generate cbranchcc4 insns
> > on rs6000. There is an predicate for operand0 of cbranchcc4 to bypass
> > some combinations. It gets assertion failure in prepare_cmp_insn. I think
> > we shouldn't suppose that all comparison codes and sub CC modes are supported
> > and throw an assertion failure in prepare_cmp_insn. It might check the
> > predicate and go to fail if the predicate can't be satisfied. This patch
> > changes the behavior of those codes.
> >
> >   Bootstrapped and tested on powerpc64-linux BE/LE and x86 with no regressions.
> > Is this okay for trunk? Any recommendations? Thanks a lot.

Since the function seems to be allowed to fail the patch looks
reasonable - still I wonder
what the "fallback" for a MODE_CC style compare-and-branch is?  There
are callers
of this function that do not seem to expect failure at least, some
suspiciously looking
like MODE_CC candiates.

Richard.

> >
> >
> > ChangeLog
> > 2022-11-23  Haochen Gui <guihaoc@linux.ibm.com>
> >
> > gcc/
> >       * optabs.cc (prepare_cmp_insn): Go to fail other than assert it when
> >       predicate check of "cbranchcc4" operand[0] fails.
> >
> > patch.diff
> > diff --git a/gcc/optabs.cc b/gcc/optabs.cc
> > index 165f8d1fa22..3ec8f6b17ba 100644
> > --- a/gcc/optabs.cc
> > +++ b/gcc/optabs.cc
> > @@ -4484,8 +4484,9 @@ prepare_cmp_insn (rtx x, rtx y, enum rtx_code comparison, rtx size,
> >      {
> >        enum insn_code icode = optab_handler (cbranch_optab, CCmode);
> >        test = gen_rtx_fmt_ee (comparison, VOIDmode, x, y);
> > -      gcc_assert (icode != CODE_FOR_nothing
> > -                  && insn_operand_matches (icode, 0, test));
> > +      gcc_assert (icode != CODE_FOR_nothing);
> > +      if (!insn_operand_matches (icode, 0, test))
> > +     goto fail;
> >        *ptest = test;
> >        return;
> >      }
  

On Mon, Nov 28, 2022 at 09:42:05AM +0100, Richard Biener wrote:
> Since the function seems to be allowed to fail the patch looks
> reasonable - still I wonder
> what the "fallback" for a MODE_CC style compare-and-branch is?  There
> are callers
> of this function that do not seem to expect failure at least, some
> suspiciously looking
> like MODE_CC candiates.

Hi!

cbranchcc4 is *not* a compare-and-branch, like ccbranch<mode>4 for other
modes are.  Instead, it is a conditional branch.  I still think it is a
bad idea to use this same pattern name for a completely different (and
much more basic) concept, it just confuses many things, makes us need
exceptions in most users of cbranch<mode>4 :-(

cbranchcc4 does not do a comparison.  Instead, it uses the result of
some previous comparison in some CC register (or anything else that set
such a register).  We want to use a cbranchcc4 to reuse some earlier
comparison here.  Which is great of course!  But, redoing the
(potentially expensive) computation to prepare the CC for a more
complicated condition is not a good idea.  Also, Power's conditional
branch insns just branch on one of the 32 condition bits (either set or
unset), not on a logical combination of multiple of those bits, as we
need with LTGT, UNLT, UNGT, UNEQ, and LE and GE without fastmath.  So it
is much cleaner (and causes fewer problems later on) if we only allow
those codes we do support.

Example of LTGT:
  fcmpu 0,0,1   # compare f0 <=> f1 to cr0 (exactly one of
                # cr0.lt, cr0.gt, cr0.eq, cr0.un will be set)
  cror 2,0,1    # cr0.eq = cr0.lt | cr0.gt
  beq 0         # branch if cr0.eq is set

So, we want the cbranchcc4 here to just do that last insn, not the last
two insns (or all three as any other cbranch<mode>4 is!)


Segher
  

On Mon, Nov 28, 2022 at 6:58 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Mon, Nov 28, 2022 at 09:42:05AM +0100, Richard Biener wrote:
> > Since the function seems to be allowed to fail the patch looks
> > reasonable - still I wonder
> > what the "fallback" for a MODE_CC style compare-and-branch is?  There
> > are callers
> > of this function that do not seem to expect failure at least, some
> > suspiciously looking
> > like MODE_CC candiates.
>
> Hi!
>
> cbranchcc4 is *not* a compare-and-branch, like ccbranch<mode>4 for other
> modes are.  Instead, it is a conditional branch.  I still think it is a
> bad idea to use this same pattern name for a completely different (and
> much more basic) concept, it just confuses many things, makes us need
> exceptions in most users of cbranch<mode>4 :-(
>
> cbranchcc4 does not do a comparison.  Instead, it uses the result of
> some previous comparison in some CC register (or anything else that set
> such a register).  We want to use a cbranchcc4 to reuse some earlier
> comparison here.  Which is great of course!  But, redoing the
> (potentially expensive) computation to prepare the CC for a more
> complicated condition is not a good idea.  Also, Power's conditional
> branch insns just branch on one of the 32 condition bits (either set or
> unset), not on a logical combination of multiple of those bits, as we
> need with LTGT, UNLT, UNGT, UNEQ, and LE and GE without fastmath.  So it
> is much cleaner (and causes fewer problems later on) if we only allow
> those codes we do support.
>
> Example of LTGT:
>   fcmpu 0,0,1   # compare f0 <=> f1 to cr0 (exactly one of
>                 # cr0.lt, cr0.gt, cr0.eq, cr0.un will be set)
>   cror 2,0,1    # cr0.eq = cr0.lt | cr0.gt
>   beq 0         # branch if cr0.eq is set
>
> So, we want the cbranchcc4 here to just do that last insn, not the last
> two insns (or all three as any other cbranch<mode>4 is!)

Anyhow - my question still stands - what's the fallback for the callers
that do not check for failure?  How are we sure we're not running into
these when relaxing the requirement that a MODE_CC prepare_cmp_insn
must not fail?

Richard.

>
> Segher
  

On Mon, Nov 28, 2022 at 07:46:07PM +0100, Richard Biener wrote:
> Anyhow - my question still stands - what's the fallback for the callers
> that do not check for failure?  How are we sure we're not running into
> these when relaxing the requirement that a MODE_CC prepare_cmp_insn
> must not fail?

This will work the same as with any other define_expand?  If the caller
of gen_blablabla does not check for failure, you end up with a NULL_RTX
in the instruction stream, which will ICE sooner or later.  Not pretty,
sure, but at least it is a reliable ICE :-)


Segher
  

Hi Richard,

在 2022/11/29 2:46, Richard Biener 写道:
> Anyhow - my question still stands - what's the fallback for the callers
> that do not check for failure?  How are we sure we're not running into
> these when relaxing the requirement that a MODE_CC prepare_cmp_insn
> must not fail?

I examed the code and found that currently callers should be fine with
returning a NULL_RTX for MODE_CC processing. The prepare_cmp_insn is called
by following callers.

1 gen_cond_trap which doesn't uses MODE_CC
2 prepare_cmp_insn itself where is after MODE_CC processing, so it never
hits MODE_CC
3 emit_cmp_and_jump_insns which doesn't uses MODE_CC
4 emit_conditional_move which checks the output is null or not
5 emit_conditional_add which checks the output is null or not

Not sure if I missed something. Looking forward to your advice.

Thanks a lot
Gui Haochen
  

On Tue, Nov 29, 2022 at 2:15 AM HAO CHEN GUI <guihaoc@linux.ibm.com> wrote:
>
> Hi Richard,
>
> 在 2022/11/29 2:46, Richard Biener 写道:
> > Anyhow - my question still stands - what's the fallback for the callers
> > that do not check for failure?  How are we sure we're not running into
> > these when relaxing the requirement that a MODE_CC prepare_cmp_insn
> > must not fail?
>
> I examed the code and found that currently callers should be fine with
> returning a NULL_RTX for MODE_CC processing. The prepare_cmp_insn is called
> by following callers.
>
> 1 gen_cond_trap which doesn't uses MODE_CC
> 2 prepare_cmp_insn itself where is after MODE_CC processing, so it never
> hits MODE_CC
> 3 emit_cmp_and_jump_insns which doesn't uses MODE_CC
> 4 emit_conditional_move which checks the output is null or not
> 5 emit_conditional_add which checks the output is null or not

Thanks for checking.

> Not sure if I missed something. Looking forward to your advice.

I'd then say the non-presence of the optab should be handled the same
as a mismatching predicate as the other comment on the patch indicates.

thanks,
Richard.

> Thanks a lot
> Gui Haochen
>