Re: [PATCH] machmode, v2: Introduce GET_MODE_NEXT_MODE with previous GET_MODE_WIDER_MODE meaning, add new GET_MODE_WIDER_MODE

[Richard Sandiford <richard.sandiford@arm.com>]

Jakub Jelinek <jakub@redhat.com> writes:
> On Wed, Oct 12, 2022 at 11:15:40AM +0100, Richard Sandiford wrote:
>> Looks good to me, just some minor comments below.
>
> Here is an updated patch.
>
>> How robust is the mechanism that guarantees HF comes before BF,
>> and so is the mode that appears in the (new) wider list?
>
> genmodes.cc seems to have cmp_modes which does a lot of different
> comparisons to make sure it is a total order.
> I think the BFmode vs. HFmode ordering is about the last case:
>   if (m->counter < n->counter)
>     return -1;
>   else
>     return 1;
> there because everything else is equal and ->counter is about which
> mode is declared first in *-modes.def.
> And my code for the new mode_wider in genmodes.cc always uses VOIDmode
> for !CLASS_HAS_WIDER_MODES_P classes and for CLASS_HAS_WIDER_MODES_P
> classes provides a subset of the total ordering, in the already computed
> ->wider chain it skips modes with the same size/precision.

OK, I guess that's good enough.

>
>> > -  /* Set mode iterator *ITER to the next widest mode in the same class.
>> > +  /* Set mode iterator *ITER to the next wider mode in the same class.
>> >       Such a mode is known to exist.  */
>> 
>> I'll take your word for it that this is correct. ;-)  I would say
>> "next widest", but it's very likely that I'm wrong.
>
> I'm not a native english speaker, but to me next with superlative would be
> if we have the widest mode, next widest would be the one whose only
> wider mode is the widest mode.
>
> Everything else changed.
>
> 2022-10-12  Jakub Jelinek  <jakub@redhat.com>
>
> 	* genmodes.cc (emit_mode_wider): Emit previous content of
> 	mode_wider array into mode_next array and for mode_wider
> 	emit always VOIDmode for !CLASS_HAS_WIDER_MODES_P classes,
> 	otherwise skip through modes with the same precision.
> 	* machmode.h (mode_next): Declare.
> 	(GET_MODE_NEXT_MODE): New inline function.
> 	(mode_iterator::get_next, mode_iterator::get_known_next): New
> 	function templates.
> 	(FOR_EACH_MODE_IN_CLASS): Use get_next instead of get_wider.
> 	(FOR_EACH_MODE): Use get_known_next instead of get_known_wider.
> 	(FOR_EACH_MODE_FROM): Use get_next instead of get_wider.
> 	(FOR_EACH_WIDER_MODE_FROM): Define.
> 	(FOR_EACH_NEXT_MODE): Define.
> 	* expmed.cc (emit_store_flag_1): Use FOR_EACH_WIDER_MODE_FROM
> 	instead of FOR_EACH_MODE_FROM.
> 	* optabs.cc (prepare_cmp_insn): Likewise.  Remove redundant
> 	!CLASS_HAS_WIDER_MODES_P check.
> 	(prepare_float_lib_cmp): Use FOR_EACH_WIDER_MODE_FROM instead of
> 	FOR_EACH_MODE_FROM.
> 	* config/i386/i386-expand.cc (get_mode_wider_vector): Use
> 	GET_MODE_NEXT_MODE instead of GET_MODE_WIDER_MODE.

LGTM, but please give others 24 hours to object.

Thanks,
Richard

> --- gcc/genmodes.cc.jj	2022-10-12 10:15:21.444381490 +0200
> +++ gcc/genmodes.cc	2022-10-12 12:28:02.414528652 +0200
> @@ -1527,7 +1527,7 @@ emit_mode_wider (void)
>    int c;
>    struct mode_data *m;
>  
> -  print_decl ("unsigned char", "mode_wider", "NUM_MACHINE_MODES");
> +  print_decl ("unsigned char", "mode_next", "NUM_MACHINE_MODES");
>  
>    for_all_modes (c, m)
>      tagged_printf ("E_%smode",
> @@ -1535,6 +1535,37 @@ emit_mode_wider (void)
>  		   m->name);
>  
>    print_closer ();
> +  print_decl ("unsigned char", "mode_wider", "NUM_MACHINE_MODES");
> +
> +  for_all_modes (c, m)
> +    {
> +      struct mode_data *m2 = 0;
> +
> +      if (m->cl == MODE_INT
> +	  || m->cl == MODE_PARTIAL_INT
> +	  || m->cl == MODE_FLOAT
> +	  || m->cl == MODE_DECIMAL_FLOAT
> +	  || m->cl == MODE_COMPLEX_FLOAT
> +	  || m->cl == MODE_FRACT
> +	  || m->cl == MODE_UFRACT
> +	  || m->cl == MODE_ACCUM
> +	  || m->cl == MODE_UACCUM)
> +	for (m2 = m->wider; m2 && m2 != void_mode; m2 = m2->wider)
> +	  {
> +	    if (m2->bytesize == m->bytesize
> +		&& m2->precision == m->precision)
> +	      continue;
> +	    break;
> +	  }
> +
> +      if (m2 == void_mode)
> +	m2 = 0;
> +      tagged_printf ("E_%smode",
> +		     m2 ? m2->name : void_mode->name,
> +		     m->name);
> +    }
> +
> +  print_closer ();
>    print_decl ("unsigned char", "mode_2xwider", "NUM_MACHINE_MODES");
>  
>    for_all_modes (c, m)
> --- gcc/machmode.h.jj	2022-10-12 10:15:21.491380846 +0200
> +++ gcc/machmode.h	2022-10-12 12:33:36.795975117 +0200
> @@ -28,6 +28,7 @@ extern const unsigned char mode_inner[NU
>  extern CONST_MODE_NUNITS poly_uint16_pod mode_nunits[NUM_MACHINE_MODES];
>  extern CONST_MODE_UNIT_SIZE unsigned char mode_unit_size[NUM_MACHINE_MODES];
>  extern const unsigned short mode_unit_precision[NUM_MACHINE_MODES];
> +extern const unsigned char mode_next[NUM_MACHINE_MODES];
>  extern const unsigned char mode_wider[NUM_MACHINE_MODES];
>  extern const unsigned char mode_2xwider[NUM_MACHINE_MODES];
>  
> @@ -760,7 +761,23 @@ GET_MODE_NUNITS (const T &mode)
>  }
>  #endif
>  
> -/* Get the next wider natural mode (eg, QI -> HI -> SI -> DI -> TI).  */
> +/* Get the next natural mode (not narrower, eg, QI -> HI -> SI -> DI -> TI
> +   or HF -> BF -> SF -> DF -> XF -> TF).  */
> +
> +template<typename T>
> +ALWAYS_INLINE opt_mode<T>
> +GET_MODE_NEXT_MODE (const T &m)
> +{
> +  return typename opt_mode<T>::from_int (mode_next[m]);
> +}
> +
> +/* Get the next wider mode (eg, QI -> HI -> SI -> DI -> TI
> +   or { HF, BF } -> SF -> DF -> XF -> TF).
> +   This is similar to GET_MODE_NEXT_MODE, but while GET_MODE_NEXT_MODE
> +   can include mode that have the same precision (e.g.
> +   GET_MODE_NEXT_MODE (HFmode) can be BFmode even when both have the same
> +   precision), this one will skip those.  And always VOIDmode for
> +   modes whose class is !CLASS_HAS_WIDER_MODES_P.  */
>  
>  template<typename T>
>  ALWAYS_INLINE opt_mode<T>
> @@ -1098,7 +1115,33 @@ namespace mode_iterator
>      return *iter != E_VOIDmode;
>    }
>  
> -  /* Set mode iterator *ITER to the next widest mode in the same class,
> +  /* Set mode iterator *ITER to the next mode in the same class,
> +     if any.  */
> +
> +  template<typename T>
> +  inline void
> +  get_next (opt_mode<T> *iter)
> +  {
> +    *iter = GET_MODE_NEXT_MODE (iter->require ());
> +  }
> +
> +  inline void
> +  get_next (machine_mode *iter)
> +  {
> +    *iter = GET_MODE_NEXT_MODE (*iter).else_void ();
> +  }
> +
> +  /* Set mode iterator *ITER to the next mode in the same class.
> +     Such a mode is known to exist.  */
> +
> +  template<typename T>
> +  inline void
> +  get_known_next (T *iter)
> +  {
> +    *iter = GET_MODE_NEXT_MODE (*iter).require ();
> +  }
> +
> +  /* Set mode iterator *ITER to the next wider mode in the same class,
>       if any.  */
>  
>    template<typename T>
> @@ -1114,7 +1157,7 @@ namespace mode_iterator
>      *iter = GET_MODE_WIDER_MODE (*iter).else_void ();
>    }
>  
> -  /* Set mode iterator *ITER to the next widest mode in the same class.
> +  /* Set mode iterator *ITER to the next wider mode in the same class.
>       Such a mode is known to exist.  */
>  
>    template<typename T>
> @@ -1146,20 +1189,27 @@ namespace mode_iterator
>  #define FOR_EACH_MODE_IN_CLASS(ITERATOR, CLASS)  \
>    for (mode_iterator::start (&(ITERATOR), CLASS); \
>         mode_iterator::iterate_p (&(ITERATOR)); \
> -       mode_iterator::get_wider (&(ITERATOR)))
> +       mode_iterator::get_next (&(ITERATOR)))
>  
>  /* Make ITERATOR iterate over all the modes in the range [START, END),
>     in order of increasing width.  */
>  #define FOR_EACH_MODE(ITERATOR, START, END) \
>    for ((ITERATOR) = (START); \
>         (ITERATOR) != (END); \
> -       mode_iterator::get_known_wider (&(ITERATOR)))
> +       mode_iterator::get_known_next (&(ITERATOR)))
>  
> -/* Make ITERATOR iterate over START and all wider modes in the same
> +/* Make ITERATOR iterate over START and all non-narrower modes in the same
>     class, in order of increasing width.  */
>  #define FOR_EACH_MODE_FROM(ITERATOR, START) \
>    for ((ITERATOR) = (START); \
>         mode_iterator::iterate_p (&(ITERATOR)); \
> +       mode_iterator::get_next (&(ITERATOR)))
> +
> +/* Make ITERATOR iterate over START and all wider modes in the same
> +   class, in order of strictly increasing width.  */
> +#define FOR_EACH_WIDER_MODE_FROM(ITERATOR, START) \
> +  for ((ITERATOR) = (START); \
> +       mode_iterator::iterate_p (&(ITERATOR)); \
>         mode_iterator::get_wider (&(ITERATOR)))
>  
>  /* Make ITERATOR iterate over modes in the range [NARROWEST, END)
> @@ -1169,6 +1219,14 @@ namespace mode_iterator
>    FOR_EACH_MODE (ITERATOR, get_narrowest_mode (END), END)
>  
>  /* Make ITERATOR iterate over modes in the same class as MODE, in order
> +   of non-decreasing width.  Start at next such mode after START,
> +   or don't iterate at all if there is no such mode.  */
> +#define FOR_EACH_NEXT_MODE(ITERATOR, START) \
> +  for ((ITERATOR) = (START), mode_iterator::get_next (&(ITERATOR)); \
> +       mode_iterator::iterate_p (&(ITERATOR)); \
> +       mode_iterator::get_next (&(ITERATOR)))
> +
> +/* Make ITERATOR iterate over modes in the same class as MODE, in order
>     of increasing width.  Start at the first mode wider than START,
>     or don't iterate at all if there is no wider mode.  */
>  #define FOR_EACH_WIDER_MODE(ITERATOR, START) \
> --- gcc/expmed.cc.jj	2022-10-12 10:15:21.335382983 +0200
> +++ gcc/expmed.cc	2022-10-12 12:28:02.417528612 +0200
> @@ -5712,7 +5712,7 @@ emit_store_flag_1 (rtx target, enum rtx_
>  
>    /* Next try expanding this via the backend's cstore<mode>4.  */
>    mclass = GET_MODE_CLASS (mode);
> -  FOR_EACH_MODE_FROM (compare_mode, mode)
> +  FOR_EACH_WIDER_MODE_FROM (compare_mode, mode)
>      {
>       machine_mode optab_mode = mclass == MODE_CC ? CCmode : compare_mode;
>       icode = optab_handler (cstore_optab, optab_mode);
> --- gcc/optabs.cc.jj	2022-10-12 10:15:21.542380147 +0200
> +++ gcc/optabs.cc	2022-10-12 12:28:02.418528598 +0200
> @@ -4384,7 +4384,6 @@ prepare_cmp_insn (rtx x, rtx y, enum rtx
>    machine_mode mode = *pmode;
>    rtx libfunc, test;
>    machine_mode cmp_mode;
> -  enum mode_class mclass;
>  
>    /* The other methods are not needed.  */
>    gcc_assert (methods == OPTAB_DIRECT || methods == OPTAB_WIDEN
> @@ -4490,9 +4489,8 @@ prepare_cmp_insn (rtx x, rtx y, enum rtx
>        return;
>      }
>  
> -  mclass = GET_MODE_CLASS (mode);
>    test = gen_rtx_fmt_ee (comparison, VOIDmode, x, y);
> -  FOR_EACH_MODE_FROM (cmp_mode, mode)
> +  FOR_EACH_WIDER_MODE_FROM (cmp_mode, mode)
>      {
>        enum insn_code icode;
>        icode = optab_handler (cbranch_optab, cmp_mode);
> @@ -4515,7 +4513,7 @@ prepare_cmp_insn (rtx x, rtx y, enum rtx
>  	  delete_insns_since (last);
>  	}
>  
> -      if (methods == OPTAB_DIRECT || !CLASS_HAS_WIDER_MODES_P (mclass))
> +      if (methods == OPTAB_DIRECT)
>  	break;
>      }
>  
> @@ -4711,7 +4709,7 @@ prepare_float_lib_cmp (rtx x, rtx y, enu
>    bool reversed_p = false;
>    scalar_int_mode cmp_mode = targetm.libgcc_cmp_return_mode ();
>  
> -  FOR_EACH_MODE_FROM (mode, orig_mode)
> +  FOR_EACH_WIDER_MODE_FROM (mode, orig_mode)
>      {
>        if (code_to_optab (comparison)
>  	  && (libfunc = optab_libfunc (code_to_optab (comparison), mode)))
> --- gcc/config/i386/i386-expand.cc.jj	2022-09-26 18:47:26.892350579 +0200
> +++ gcc/config/i386/i386-expand.cc	2022-10-12 12:28:02.421528557 +0200
> @@ -14941,7 +14941,7 @@ static machine_mode
>  get_mode_wider_vector (machine_mode o)
>  {
>    /* ??? Rely on the ordering that genmodes.cc gives to vectors.  */
> -  machine_mode n = GET_MODE_WIDER_MODE (o).require ();
> +  machine_mode n = GET_MODE_NEXT_MODE (o).require ();
>    gcc_assert (GET_MODE_NUNITS (o) == GET_MODE_NUNITS (n) * 2);
>    gcc_assert (GET_MODE_SIZE (o) == GET_MODE_SIZE (n));
>    return n;
>
>
> 	Jakub