machmode: Introduce GET_MODE_NEXT_MODE with previous GET_MODE_WIDER_MODE meaning, add new GET_MODE_WIDER_MODE

  On Wed, Oct 05, 2022 at 04:02:25PM -0400, Jason Merrill wrote:
> > > > @@ -5716,7 +5716,13 @@ emit_store_flag_1 (rtx target, enum rtx_
> > > >        {
> > > >         machine_mode optab_mode = mclass == MODE_CC ? CCmode : compare_mode;
> > > >         icode = optab_handler (cstore_optab, optab_mode);
> > > > -     if (icode != CODE_FOR_nothing)
> > > > +     if (icode != CODE_FOR_nothing
> > > > +	 /* Don't consider [BH]Fmode as usable wider mode, as neither is
> > > > +	    a subset or superset of the other.  */
> > > > +	 && (compare_mode == mode
> > > > +	     || !SCALAR_FLOAT_MODE_P (compare_mode)
> > > > +	     || maybe_ne (GET_MODE_PRECISION (compare_mode),
> > > > +			  GET_MODE_PRECISION (mode))))
> > > 
> > > Why do you need to do this here (and in prepare_cmp_insn, and similarly in
> > > can_compare_p)?  Shouldn't get_wider skip over modes that are not actually
> > > wider?
> > 
> > I'm afraid too many places rely on all modes of a certain class to be
> > visible when walking from "narrowest" to "widest" mode, say
> > FOR_EACH_MODE_IN_CLASS/FOR_EACH_MODE/FOR_EACH_MODE_UNTIL/FOR_EACH_WIDER_MODE
> > etc. wouldn't work at all if GET_MODE_WIDER_MODE (BFmode) == SFmode
> > && GET_MODE_WIDER_MODE (HFmode) == SFmode.
> 
> Yes, it seems they need to change now that their assumptions have been
> violated.  I suppose FOR_EACH_MODE_IN_CLASS would need to change to not use
> get_wider, and users of FOR_EACH_MODE/FOR_EACH_MODE_UNTIL need to decide
> whether they want an iteration that uses get_wider (likely with a new name)
> or not.

Here is a patch which does that.
Passes bootstrap/regtest on x86_64-linux and i686-linux.

Though I admit I didn't go carefully through all 24 GET_MODE_WIDER_MODE
uses, 54 FOR_EACH_MODE_IN_CLASS uses, 3 FOR_EACH_MODE uses, 24
FOR_EACH_MODE_FROM, 6 FOR_EACH_MODE_UNTIL and 15 FOR_EACH_WIDER_MODE uses.
It is more important to go through the GET_MODE_WIDER_MODE and
FOR_EACH_WIDER_MODE uses because the patch changes behavior for those,
the rest keep their previous meaning and so can be changed incrementally
if the other meaning is desirable to them (I've of course changed the 3
spots I had to change in the previous BFmode patch and whatever triggered
during the bootstraps).

Thoughts on this?

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.



	Jakub
  

Jakub Jelinek <jakub@redhat.com> writes:
> On Wed, Oct 05, 2022 at 04:02:25PM -0400, Jason Merrill wrote:
>> > > > @@ -5716,7 +5716,13 @@ emit_store_flag_1 (rtx target, enum rtx_
>> > > >        {
>> > > >         machine_mode optab_mode = mclass == MODE_CC ? CCmode : compare_mode;
>> > > >         icode = optab_handler (cstore_optab, optab_mode);
>> > > > -     if (icode != CODE_FOR_nothing)
>> > > > +     if (icode != CODE_FOR_nothing
>> > > > +	 /* Don't consider [BH]Fmode as usable wider mode, as neither is
>> > > > +	    a subset or superset of the other.  */
>> > > > +	 && (compare_mode == mode
>> > > > +	     || !SCALAR_FLOAT_MODE_P (compare_mode)
>> > > > +	     || maybe_ne (GET_MODE_PRECISION (compare_mode),
>> > > > +			  GET_MODE_PRECISION (mode))))
>> > > 
>> > > Why do you need to do this here (and in prepare_cmp_insn, and similarly in
>> > > can_compare_p)?  Shouldn't get_wider skip over modes that are not actually
>> > > wider?
>> > 
>> > I'm afraid too many places rely on all modes of a certain class to be
>> > visible when walking from "narrowest" to "widest" mode, say
>> > FOR_EACH_MODE_IN_CLASS/FOR_EACH_MODE/FOR_EACH_MODE_UNTIL/FOR_EACH_WIDER_MODE
>> > etc. wouldn't work at all if GET_MODE_WIDER_MODE (BFmode) == SFmode
>> > && GET_MODE_WIDER_MODE (HFmode) == SFmode.
>> 
>> Yes, it seems they need to change now that their assumptions have been
>> violated.  I suppose FOR_EACH_MODE_IN_CLASS would need to change to not use
>> get_wider, and users of FOR_EACH_MODE/FOR_EACH_MODE_UNTIL need to decide
>> whether they want an iteration that uses get_wider (likely with a new name)
>> or not.
>
> Here is a patch which does that.
> Passes bootstrap/regtest on x86_64-linux and i686-linux.
>
> Though I admit I didn't go carefully through all 24 GET_MODE_WIDER_MODE
> uses, 54 FOR_EACH_MODE_IN_CLASS uses, 3 FOR_EACH_MODE uses, 24
> FOR_EACH_MODE_FROM, 6 FOR_EACH_MODE_UNTIL and 15 FOR_EACH_WIDER_MODE uses.
> It is more important to go through the GET_MODE_WIDER_MODE and
> FOR_EACH_WIDER_MODE uses because the patch changes behavior for those,
> the rest keep their previous meaning and so can be changed incrementally
> if the other meaning is desirable to them (I've of course changed the 3
> spots I had to change in the previous BFmode patch and whatever triggered
> during the bootstraps).
>
> Thoughts on this?

Looks good to me, just some minor comments below.

> 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.
>
> --- gcc/genmodes.cc.jj	2022-05-23 21:44:48.080857253 +0200
> +++ gcc/genmodes.cc	2022-10-11 22:35:39.680286764 +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-01-18 00:18:02.823743394 +0100
> +++ gcc/machmode.h	2022-10-11 22:35:39.680286764 +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,21 @@ 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).  */

In addition to the comment you added below, I think it would be good to
give an FP example here as well, with HF and BF both included.

> +
> +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).

And then the same example here, but with BF removed.

How robust is the mechanism that guarantees HF comes before BF,
and so is the mode that appears in the (new) wider list?

> +   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 +1113,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 wider mode in the same class.

s/wider //

> +     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 +1155,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.  */

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.

>    template<typename T>
> @@ -1146,20 +1187,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)
> @@ -1170,6 +1218,14 @@ namespace mode_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,

Maybe s/increasing/non-decreasing/?  And maybe
s/first mode wider than/next such mode after/.

Thanks,
Richard

> +   or don't iterate at all if there is no wider 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) \
>    for ((ITERATOR) = (START), mode_iterator::get_wider (&(ITERATOR)); \
> --- gcc/expmed.cc.jj	2022-10-05 21:22:56.191918383 +0200
> +++ gcc/expmed.cc	2022-10-11 22:35:39.682286736 +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-05 21:22:56.217918032 +0200
> +++ gcc/optabs.cc	2022-10-11 23:20:08.216037640 +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 22:29:41.407322933 +0200
> +++ gcc/config/i386/i386-expand.cc	2022-10-11 23:22:55.579761522 +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
  

> Though I admit I didn't go carefully through all 24 GET_MODE_WIDER_MODE
> uses, 54 FOR_EACH_MODE_IN_CLASS uses, 3 FOR_EACH_MODE uses, 24
> FOR_EACH_MODE_FROM, 6 FOR_EACH_MODE_UNTIL and 15 FOR_EACH_WIDER_MODE uses.
> It is more important to go through the GET_MODE_WIDER_MODE and
> FOR_EACH_WIDER_MODE uses because the patch changes behavior for those,
> the rest keep their previous meaning and so can be changed incrementally
> if the other meaning is desirable to them (I've of course changed the 3
> spots I had to change in the previous BFmode patch and whatever triggered
> during the bootstraps).
> 
> Thoughts on this?

Can't we declare that one is wider than the other, for example BFmode since it 
has got a larger range?  Though I guess this would mean special-casing them in 
genmodes.cc as they are presumably strictly identical except for the format.
  

On Wed, Oct 12, 2022 at 12:37:39PM +0200, Eric Botcazou wrote:
> > Though I admit I didn't go carefully through all 24 GET_MODE_WIDER_MODE
> > uses, 54 FOR_EACH_MODE_IN_CLASS uses, 3 FOR_EACH_MODE uses, 24
> > FOR_EACH_MODE_FROM, 6 FOR_EACH_MODE_UNTIL and 15 FOR_EACH_WIDER_MODE uses.
> > It is more important to go through the GET_MODE_WIDER_MODE and
> > FOR_EACH_WIDER_MODE uses because the patch changes behavior for those,
> > the rest keep their previous meaning and so can be changed incrementally
> > if the other meaning is desirable to them (I've of course changed the 3
> > spots I had to change in the previous BFmode patch and whatever triggered
> > during the bootstraps).
> > 
> > Thoughts on this?
> 
> Can't we declare that one is wider than the other, for example BFmode since it 
> has got a larger range?  Though I guess this would mean special-casing them in 
> genmodes.cc as they are presumably strictly identical except for the format.

That doesn't work, one of the modes has larger range, the other has larger
floating point precision.  So, neither of the modes is a subset or superset
of the other.  If we don't handle a particular optab for one of these modes
and allow widening, for both of these modes we want to try SFmode next
(which is a true superset of both modes, it has the same range as BFmode
but higher floating point precision than both HFmode and BFmode).

The only way to work around this widening problem would be always make sure
that whenever we implement any optab for HFmode, we also implement the same
optab for BFmode under exact same conditions and vice versa, even if those
optabs just do by hand whatever the generic code would do if the optab
didn't exist.  But that is way too limiting.

It is true that on PowerPC we have a similar situation for the widest
floating point modes, TFmode/IFmode/KFmode and the backend has the ugly hack
of pretending they have different GET_MODE_PRECISION, but as those are the
widest modes and are implemented in hardware or in software emulation, they
have in the backends the same optabs implemented.  While for HFmode/BFmode,
very few optabs are actually implemented directly and the usual intended use
is performing most arithmetics in SFmode.  Even on PowerPC,
ibm_extended_format and ieee_quad_format are neither subset nor superset of
each other, the latter has larger range and in most cases higher floating
point precision, but the former for certain values can have even 10 times
higher floating point precision.

	Jakub