Add a new conversion for conditional ternary set into ifcvt [PR106536]

  Hi,
  There is a new insn on my target, which has a nested if_then_else and
set -1, 0 and 1 according to a comparison.

   [(set (match_operand:SI 0 "gpc_reg_operand" "=r")
         (if_then_else:SI (lt (match_operand:CC 1 "cc_reg_operand" "y")
                              (const_int 0))
                          (const_int -1)
                          (if_then_else (gt (match_dup 1)
                                            (const_int 0))
                                        (const_int 1)
                                        (const_int 0))))]

  In ifcvt pass, it probably contains a comparison, a branch, a setcc
and a constant set.

    8: r122:CC=cmp(r120:DI#0,r121:DI#0)
    9: pc={(r122:CC<0)?L29:pc}

   14: r118:SI=r122:CC>0

   29: L29:
    5: r118:SI=0xffffffffffffffff

  This patch adds the new conversion into ifcvt and convert this kind of
branch into a nested if-then-else insn if the target supports such
pattern.

  HAVE_ternary_conditional_set indicates if the target has such nested
if-then-else insn. It's set in genconfig. noce_try_ternary_cset will be
executed to detect suitable pattern and convert it to the nested
if-then-else insn if HAVE_ternary_conditional_set is set. The hook
TARGET_NOCE_TERNARY_CSET_P detects target specific pattern and output
conditions and setting integers for the nested if-then-else.

  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/
	* doc/tm.texi: Regenerate.
	* doc/tm.texi.in (TARGET_NOCE_TERNARY_CSET_P): Document new hook.
	* genconfig.cc (have_ternary_cset_flag): New.
	(walk_insn_part): Detect nested if-then-else with const_int setting
	and set have_ternary_cset_flag.
	(HAVE_ternary_conditional_set): Define.
	* ifcvt.cc (noce_emit_ternary_cset): New function to emit nested
	if-then-else insns.
	(noce_try_ternary_cset): Detect ternary conditional set and emit the
	insn.
	(noce_process_if_block): Try to do ternary condition set convertion
	when a target supports ternary conditional set insn.
	* target.def (noce_ternary_cset_p): New hook.
	* targhooks.cc (default_noce_ternary_cset_p): New function.
	* targhooks.h (default_noce_ternary_cset_p): New declare.


patch.diff
  

On Wed, Nov 23, 2022 at 8:09 AM HAO CHEN GUI via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi,
>   There is a new insn on my target, which has a nested if_then_else and
> set -1, 0 and 1 according to a comparison.
>
>    [(set (match_operand:SI 0 "gpc_reg_operand" "=r")
>          (if_then_else:SI (lt (match_operand:CC 1 "cc_reg_operand" "y")
>                               (const_int 0))
>                           (const_int -1)
>                           (if_then_else (gt (match_dup 1)
>                                             (const_int 0))
>                                         (const_int 1)
>                                         (const_int 0))))]
>
>   In ifcvt pass, it probably contains a comparison, a branch, a setcc
> and a constant set.
>
>     8: r122:CC=cmp(r120:DI#0,r121:DI#0)
>     9: pc={(r122:CC<0)?L29:pc}
>
>    14: r118:SI=r122:CC>0
>
>    29: L29:
>     5: r118:SI=0xffffffffffffffff
>
>   This patch adds the new conversion into ifcvt and convert this kind of
> branch into a nested if-then-else insn if the target supports such
> pattern.
>
>   HAVE_ternary_conditional_set indicates if the target has such nested
> if-then-else insn. It's set in genconfig. noce_try_ternary_cset will be
> executed to detect suitable pattern and convert it to the nested
> if-then-else insn if HAVE_ternary_conditional_set is set. The hook
> TARGET_NOCE_TERNARY_CSET_P detects target specific pattern and output
> conditions and setting integers for the nested if-then-else.
>
>   Bootstrapped and tested on powerpc64-linux BE/LE and x86 with no
> regressions. Is this okay for trunk? Any recommendations? Thanks a lot.

Wouldn't we usually either add an optab or try to recog a canonical
RTL form instead of adding a new target hook for things like this?

> ChangeLog
> 2022-11-23  Haochen Gui <guihaoc@linux.ibm.com>
>
> gcc/
>         * doc/tm.texi: Regenerate.
>         * doc/tm.texi.in (TARGET_NOCE_TERNARY_CSET_P): Document new hook.
>         * genconfig.cc (have_ternary_cset_flag): New.
>         (walk_insn_part): Detect nested if-then-else with const_int setting
>         and set have_ternary_cset_flag.
>         (HAVE_ternary_conditional_set): Define.
>         * ifcvt.cc (noce_emit_ternary_cset): New function to emit nested
>         if-then-else insns.
>         (noce_try_ternary_cset): Detect ternary conditional set and emit the
>         insn.
>         (noce_process_if_block): Try to do ternary condition set convertion
>         when a target supports ternary conditional set insn.
>         * target.def (noce_ternary_cset_p): New hook.
>         * targhooks.cc (default_noce_ternary_cset_p): New function.
>         * targhooks.h (default_noce_ternary_cset_p): New declare.
>
>
> patch.diff
> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> index 92bda1a7e14..9823eccbe68 100644
> --- a/gcc/doc/tm.texi
> +++ b/gcc/doc/tm.texi
> @@ -7094,6 +7094,15 @@ the @code{POLY_VALUE_MIN}, @code{POLY_VALUE_MAX} and
>  implementation returns the lowest possible value of @var{val}.
>  @end deftypefn
>
> +@deftypefn {Target Hook} bool TARGET_NOCE_TERNARY_CSET_P (struct noce_if_info *@var{if_info}, rtx *@var{outer_cond}, rtx *@var{inner_cond}, int *@var{int1}, int *@var{int2}, int *@var{int3})
> +This hook returns true if the if-then-else-join blocks describled in
> +@code{if_info} can be converted to a ternary conditional set implemented by
> +a nested if-then-else insn.  The @code{int1}, @code{int2} and @code{int3}
> +are three possible results of the nested if-then-else insn.
> +@code{outer_cond} and @code{inner_cond} are the conditions for outer and
> +if-then-else.
> +@end deftypefn
> +
>  @node Scheduling
>  @section Adjusting the Instruction Scheduler
>
> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
> index 112462310b1..1d6f28cc50a 100644
> --- a/gcc/doc/tm.texi.in
> +++ b/gcc/doc/tm.texi.in
> @@ -4631,6 +4631,8 @@ Define this macro if a non-short-circuit operation produced by
>
>  @hook TARGET_ESTIMATED_POLY_VALUE
>
> +@hook TARGET_NOCE_TERNARY_CSET_P
> +
>  @node Scheduling
>  @section Adjusting the Instruction Scheduler
>
> diff --git a/gcc/genconfig.cc b/gcc/genconfig.cc
> index b7c6b48eec6..902c832cf5a 100644
> --- a/gcc/genconfig.cc
> +++ b/gcc/genconfig.cc
> @@ -33,6 +33,7 @@ static int max_recog_operands;  /* Largest operand number seen.  */
>  static int max_dup_operands;    /* Largest number of match_dup in any insn.  */
>  static int max_clobbers_per_insn;
>  static int have_cmove_flag;
> +static int have_ternary_cset_flag;
>  static int have_cond_exec_flag;
>  static int have_lo_sum_flag;
>  static int have_rotate_flag;
> @@ -136,6 +137,12 @@ walk_insn_part (rtx part, int recog_p, int non_pc_set_src)
>           && GET_CODE (XEXP (part, 1)) == MATCH_OPERAND
>           && GET_CODE (XEXP (part, 2)) == MATCH_OPERAND)
>         have_cmove_flag = 1;
> +      else if (recog_p && non_pc_set_src
> +              && GET_CODE (XEXP (part, 1)) == CONST_INT
> +              && GET_CODE (XEXP (part, 2)) == IF_THEN_ELSE
> +              && GET_CODE (XEXP (XEXP (part, 2), 1)) == CONST_INT
> +              && GET_CODE (XEXP (XEXP (part, 2), 2)) == CONST_INT)
> +       have_ternary_cset_flag = 1;
>        break;
>
>      case COND_EXEC:
> @@ -328,6 +335,11 @@ main (int argc, const char **argv)
>    else
>      printf ("#define HAVE_conditional_move 0\n");
>
> +  if (have_ternary_cset_flag)
> +    printf ("#define HAVE_ternary_conditional_set 1\n");
> +  else
> +    printf ("#define HAVE_ternary_conditional_set 0\n");
> +
>    if (have_cond_exec_flag)
>      printf ("#define HAVE_conditional_execution 1\n");
>    else
> diff --git a/gcc/ifcvt.cc b/gcc/ifcvt.cc
> index eb8efb89a89..774dff21719 100644
> --- a/gcc/ifcvt.cc
> +++ b/gcc/ifcvt.cc
> @@ -1830,6 +1830,42 @@ noce_emit_cmove (struct noce_if_info *if_info, rtx x, enum rtx_code code,
>      return NULL_RTX;
>  }
>
> +/*  Emit a nested if-then-else insn.  */
> +
> +static rtx
> +noce_emit_ternary_cset (rtx x, rtx outer_cond, rtx inner_cond,
> +                       int a, int b, int c)
> +{
> +  rtx target;
> +  machine_mode mode;
> +  machine_mode orig_mode = GET_MODE (x);
> +
> +  FOR_EACH_MODE_FROM (mode, orig_mode)
> +    {
> +      rtx inner_if_then_else = gen_rtx_IF_THEN_ELSE (mode, inner_cond,
> +                                                    GEN_INT (b), GEN_INT (c));
> +      rtx outer_if_then_else = gen_rtx_IF_THEN_ELSE (mode, outer_cond,
> +                                                    GEN_INT (a),
> +                                                    inner_if_then_else);
> +      target = (mode == orig_mode) ? x : gen_reg_rtx (mode);
> +      rtx set = gen_rtx_SET (target, outer_if_then_else);
> +      start_sequence ();
> +      rtx_insn *insn = emit_insn (set);
> +
> +      if (recog_memoized (insn) >= 0)
> +       {
> +         rtx_insn *seq = get_insns ();
> +         end_sequence ();
> +         emit_insn (seq);
> +
> +         return target;
> +       }
> +    }
> +
> +  end_sequence ();
> +  return NULL_RTX;
> +}
> +
>  /* Try only simple constants and registers here.  More complex cases
>     are handled in noce_try_cmove_arith after noce_try_store_flag_arith
>     has had a go at it.  */
> @@ -2987,6 +3023,43 @@ noce_try_bitop (struct noce_if_info *if_info)
>    return TRUE;
>  }
>
> +/* Try to find pattern "a < b ? -1 : (a > b ? 1 : 0);" and convert it to
> +   a nested if-then-else insn.  */
> +
> +static int
> +noce_try_ternary_cset (struct noce_if_info *if_info)
> +{
> +  rtx outer_cond = NULL_RTX, inner_cond = NULL_RTX;
> +  int int1 = 0, int2 = 0, int3 = 0;
> +
> +  if (targetm.noce_ternary_cset_p (if_info, &outer_cond, &inner_cond,
> +                                  &int1, &int2, &int3))
> +    {
> +      start_sequence ();
> +      rtx target = noce_emit_ternary_cset (if_info->x, outer_cond, inner_cond,
> +                                          int1, int2, int3);
> +      if (target)
> +       {
> +         rtx_insn *ifcvt_seq;
> +
> +         if (target != if_info->x)
> +           noce_emit_move_insn (if_info->x, target);
> +
> +         ifcvt_seq = end_ifcvt_sequence (if_info);
> +         if (!ifcvt_seq)
> +           return false;
> +
> +         emit_insn_before_setloc (ifcvt_seq, if_info->jump,
> +                                  INSN_LOCATION (if_info->insn_a));
> +         if_info->transform_name = "noce_try_ternary_cset";
> +         return true;
> +       }
> +
> +      end_sequence ();
> +    }
> +
> +  return false;
> +}
>
>  /* Similar to get_condition, only the resulting condition must be
>     valid at JUMP, instead of at EARLIEST.
> @@ -3963,6 +4036,9 @@ noce_process_if_block (struct noce_if_info *if_info)
>    if (HAVE_conditional_move
>        && noce_try_cmove (if_info))
>      goto success;
> +  if (HAVE_ternary_conditional_set
> +      && noce_try_ternary_cset (if_info))
> +    goto success;
>    if (! targetm.have_conditional_execution ())
>      {
>        if (noce_try_addcc (if_info))
> diff --git a/gcc/target.def b/gcc/target.def
> index 2a7fa68f83d..1b983bf852e 100644
> --- a/gcc/target.def
> +++ b/gcc/target.def
> @@ -3907,6 +3907,20 @@ candidate as a replacement for the if-convertible sequence described in\n\
>  bool, (rtx_insn *seq, struct noce_if_info *if_info),
>  default_noce_conversion_profitable_p)
>
> +/* Return true if the if-then-else-join blocks can be converted to a nested\n\
> +if-then-else insn.  */
> +DEFHOOK
> +(noce_ternary_cset_p,
> + "This hook returns true if the if-then-else-join blocks describled in\n\
> +@code{if_info} can be converted to a ternary conditional set implemented by\n\
> +a nested if-then-else insn.  The @code{int1}, @code{int2} and @code{int3}\n\
> +are three possible results of the nested if-then-else insn.\n\
> +@code{outer_cond} and @code{inner_cond} are the conditions for outer and\n\
> +if-then-else.",
> +bool, (struct noce_if_info *if_info, rtx *outer_cond, rtx *inner_cond,
> +       int *int1, int *int2, int *int3),
> +default_noce_ternary_cset_p)
> +
>  /* Return true if new_addr should be preferred over the existing address used by
>     memref in insn.  */
>  DEFHOOK
> diff --git a/gcc/targhooks.cc b/gcc/targhooks.cc
> index b15ae19bcb6..179324e3f0b 100644
> --- a/gcc/targhooks.cc
> +++ b/gcc/targhooks.cc
> @@ -2673,4 +2673,17 @@ default_gcov_type_size (void)
>    return TYPE_PRECISION (long_long_integer_type_node) > 32 ? 64 : 32;
>  }
>
> +/* The default implementation of TARGET_NOCE_TERNARY_CSET_P.  */
> +
> +bool
> +default_noce_ternary_cset_p (struct noce_if_info *if_info ATTRIBUTE_UNUSED,
> +                            rtx *outer_cond ATTRIBUTE_UNUSED,
> +                            rtx *inner_cond ATTRIBUTE_UNUSED,
> +                            int *int1 ATTRIBUTE_UNUSED,
> +                            int *int2 ATTRIBUTE_UNUSED,
> +                            int *int3 ATTRIBUTE_UNUSED)
> +{
> +  return false;
> +}
> +
>  #include "gt-targhooks.h"
> diff --git a/gcc/targhooks.h b/gcc/targhooks.h
> index ecce55ebe79..a58bc8ef6a6 100644
> --- a/gcc/targhooks.h
> +++ b/gcc/targhooks.h
> @@ -296,5 +296,7 @@ extern rtx default_memtag_extract_tag (rtx, rtx);
>  extern rtx default_memtag_untagged_pointer (rtx, rtx);
>
>  extern HOST_WIDE_INT default_gcov_type_size (void);
> +extern bool default_noce_ternary_cset_p (struct noce_if_info *, rtx *, rtx *,
> +                                        int *, int *, int *);
>
>  #endif /* GCC_TARGHOOKS_H */
  

Hi Richard,


在 2022/11/24 4:06, Richard Biener 写道:
> Wouldn't we usually either add an optab or try to recog a canonical
> RTL form instead of adding a new target hook for things like this?

Thanks so much for your comments. Please let me make it clear.

Do you mean we should create an optab for "setb" pattern (the nested
if-then-else insn) and detect candidate insns in ifcvt pass? Then
generate the insn with the new optab?

My concern is that some candidate insns are target specific. For
example, different modes cause additional zero_extend or subreg insns
generated on different targets. So I put the detection process into a
target hook.

Looking forward to your advice.

Thanks again
Gui Haochen
  

On Thu, Nov 24, 2022 at 8:25 AM HAO CHEN GUI <guihaoc@linux.ibm.com> wrote:
>
> Hi Richard,
>
>
> 在 2022/11/24 4:06, Richard Biener 写道:
> > Wouldn't we usually either add an optab or try to recog a canonical
> > RTL form instead of adding a new target hook for things like this?
>
> Thanks so much for your comments. Please let me make it clear.
>
> Do you mean we should create an optab for "setb" pattern (the nested
> if-then-else insn) and detect candidate insns in ifcvt pass? Then
> generate the insn with the new optab?

Yes, that would be one way to do it.  Another way would be to
generate a (to be defined) canonical form of such instruction and
see whether it can be recognized (whether there's a define_insn
for it).

Note that were just things that came into my mind here, I'm not too
familiar with how we handle such situations but at least I'm not
aware of dozens of target hooks to handle instruction availability.

Richard.

> My concern is that some candidate insns are target specific. For
> example, different modes cause additional zero_extend or subreg insns
> generated on different targets. So I put the detection process into a
> target hook.
>
> Looking forward to your advice.
>
> Thanks again
> Gui Haochen
  

On Wed, 23 Nov 2022, HAO CHEN GUI via Gcc-patches wrote:

> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> index 92bda1a7e14..9823eccbe68 100644
> --- a/gcc/doc/tm.texi
> +++ b/gcc/doc/tm.texi
> @@ -7094,6 +7094,15 @@ the @code{POLY_VALUE_MIN}, @code{POLY_VALUE_MAX} and
>  implementation returns the lowest possible value of @var{val}.
>  @end deftypefn
> 
> +@deftypefn {Target Hook} bool TARGET_NOCE_TERNARY_CSET_P (struct noce_if_info *@var{if_info}, rtx *@var{outer_cond}, rtx *@var{inner_cond}, int *@var{int1}, int *@var{int2}, int *@var{int3})
> +This hook returns true if the if-then-else-join blocks describled in

Random typo spotted: "described"

Also, IMHO needs more explanation (in tm.texi preferably) why 
this doesn't happen as part of general "combine" machinery.

brgds, H-P
  

Hi Nilsson,

在 2022/12/2 10:49, Hans-Peter Nilsson 写道:
> On Wed, 23 Nov 2022, HAO CHEN GUI via Gcc-patches wrote:
> 
>> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
>> index 92bda1a7e14..9823eccbe68 100644
>> --- a/gcc/doc/tm.texi
>> +++ b/gcc/doc/tm.texi
>> @@ -7094,6 +7094,15 @@ the @code{POLY_VALUE_MIN}, @code{POLY_VALUE_MAX} and
>>  implementation returns the lowest possible value of @var{val}.
>>  @end deftypefn
>>
>> +@deftypefn {Target Hook} bool TARGET_NOCE_TERNARY_CSET_P (struct noce_if_info *@var{if_info}, rtx *@var{outer_cond}, rtx *@var{inner_cond}, int *@var{int1}, int *@var{int2}, int *@var{int3})
>> +This hook returns true if the if-then-else-join blocks describled in
> 
> Random typo spotted: "described"
> 
> Also, IMHO needs more explanation (in tm.texi preferably) why 
> this doesn't happen as part of general "combine" machinery.

Thanks for your comments. Combine can't take it as the insns are not in same
block. Also combine has the limitation on the number of insns. I will add
those comments.

Thanks
Gui Haochen

> 
> brgds, H-P
  

Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> On Thu, Nov 24, 2022 at 8:25 AM HAO CHEN GUI <guihaoc@linux.ibm.com> wrote:
>>
>> Hi Richard,
>>
>>
>> 在 2022/11/24 4:06, Richard Biener 写道:
>> > Wouldn't we usually either add an optab or try to recog a canonical
>> > RTL form instead of adding a new target hook for things like this?
>>
>> Thanks so much for your comments. Please let me make it clear.
>>
>> Do you mean we should create an optab for "setb" pattern (the nested
>> if-then-else insn) and detect candidate insns in ifcvt pass? Then
>> generate the insn with the new optab?
>
> Yes, that would be one way to do it.  Another way would be to
> generate a (to be defined) canonical form of such instruction and
> see whether it can be recognized (whether there's a define_insn
> for it).
>
> Note that were just things that came into my mind here, I'm not too
> familiar with how we handle such situations but at least I'm not
> aware of dozens of target hooks to handle instruction availability.

Yeah, this was my reaction too.  The patch does use recog for the
conditional set itself, which is good, but I'm not sure from the patch
what the target hook is supposed to do in preparation for the recog.

I think it'd be better to avoid having too many hooks that take
noce_if_info parameters.  It's really just a bunch of internal
pass state, rather than something that was designed to be a public
interface.

Currently we have one hook that takes noce_if_info parameters,
for costing.  That's still one more than I'd like, but at least there
are no correctness concerns and, if someone changes noce_if_info
in future, just rebuilding cc1 for the affected targets should be
good enough as far as testing goes.  If we start using hooks for code
generation too, we're effectively distributing the ifcvt pass across
targets, which makes the pass harder to maintain.

Thanks,
Richard
  

Hi!

On Thu, Nov 24, 2022 at 11:24:15AM +0100, Richard Biener wrote:
> On Thu, Nov 24, 2022 at 8:25 AM HAO CHEN GUI <guihaoc@linux.ibm.com> wrote:
> > 在 2022/11/24 4:06, Richard Biener 写道:
> > > Wouldn't we usually either add an optab or try to recog a canonical
> > > RTL form instead of adding a new target hook for things like this?
> >
> > Thanks so much for your comments. Please let me make it clear.
> >
> > Do you mean we should create an optab for "setb" pattern (the nested
> > if-then-else insn) and detect candidate insns in ifcvt pass? Then
> > generate the insn with the new optab?
> 
> Yes, that would be one way to do it.  Another way would be to
> generate a (to be defined) canonical form of such instruction and
> see whether it can be recognized (whether there's a define_insn
> for it).

But these insns are most useful when they come up "naturally", so we
really have to recognise many formulations of it separately.

The machine insn  setb x,BF  returns:
  { -1,  if BF.0 is set
  { 1,   if BF.1 is set
  { 0,   otherwise
This "otherwise" includes when a floating point comparison returned
"unordered" (BF.3, set if an operand was NaN), and BF.2 ("equal").  If
the comparison was only three-way (integer or fast-math) it is natural
to test for equality first (given we always have exactly one of the
first three BF bits set in that case!)

> Note that were just things that came into my mind here, I'm not too
> familiar with how we handle such situations but at least I'm not
> aware of dozens of target hooks to handle instruction availability.

In similar cases I never could up with anything that worked better than
recognising all possible patterns, unfortunately.  We can do a predicate
for that though, there is no need to write it out all over the place :-)


Segher