Re: [7/67] Add scalar_float_mode

[Richard Sandiford <richard dot sandiford at arm dot com>]

David Malcolm <dmalcolm@redhat.com> writes:
> On Fri, 2016-12-09 at 12:56 +0000, Richard Sandiford wrote:
>> This patch adds a scalar_float_mode class, which wraps a mode enum
>> that is known to satisfy SCALAR_FLOAT_MODE_P.  Things like "SFmode"
>> now give a scalar_float_mode object instead of a machine_mode.
>> This in turn needs a change to the real.h format_helper, so that
>> it can accept both machine_modes and scalar_float_modes.
>
> [...]
>
>> diff --git a/gcc/machmode.h b/gcc/machmode.h
>> index 44a8ad4..996f991 100644
>> --- a/gcc/machmode.h
>> +++ b/gcc/machmode.h
>> @@ -29,6 +29,16 @@ extern const unsigned short
>> mode_unit_precision[NUM_MACHINE_MODES];
>>  extern const unsigned char mode_wider[NUM_MACHINE_MODES];
>>  extern const unsigned char mode_2xwider[NUM_MACHINE_MODES];
>>  
>> +/* Allow direct conversion of enums to specific mode classes only
>> +   when USE_ENUM_MODES is defined.  This is only intended for use
>> +   by gencondmd, so that it can tell more easily when .md conditions
>> +   are always false.  */
>> +#ifdef USE_ENUM_MODES
>> +#define PROTECT_ENUM_CONVERSION public
>> +#else
>> +#define PROTECT_ENUM_CONVERSION protected
>> +#endif
>> +
>>  /* Get the name of mode MODE as a string.  */
>>  
>>  extern const char * const mode_name[NUM_MACHINE_MODES];
>> @@ -237,6 +247,85 @@ opt_mode<T>::exists (U *mode) const
>>    return false;
>>  }
>>  
>> +/* Return true if mode M has type T.  */
>> +
>> +template<typename T>
>> +inline bool
>> +is_a (machine_mode_enum m)
>> +{
>> +  return T::includes_p (m);
>> +}
>> +
>> +/* Assert that mode M has type T, and return it in that form.  */
>> +
>> +template<typename T>
>> +inline T
>> +as_a (machine_mode_enum m)
>> +{
>> +  gcc_checking_assert (T::includes_p (m));
>> +  return T::from_int (m);
>> +}
>> +
>> +/* Convert M to an opt_mode<T>.  */
>> +
>> +template<typename T>
>> +inline opt_mode<T>
>> +dyn_cast (machine_mode_enum m)
>> +{
>> +  if (T::includes_p (m))
>> +    return T::from_int (m);
>> +  return opt_mode<T> ();
>> +}
>> +
>> +/* Return true if mode M has type T, storing it as a T in *RESULT
>> +   if so.  */
>> +
>> +template<typename T, typename U>
>> +inline bool
>> +is_a (machine_mode_enum m, U *result)
>> +{
>> +  if (T::includes_p (m))
>> +    {
>> +      *result = T::from_int (m);
>> +      return true;
>> +    }
>> +  return false;
>> +}
>> +
>> +/* Represents a machine mode that is known to be a
>> SCALAR_FLOAT_MODE_P.  */
>> +class scalar_float_mode
>
> Should this be a subclass of machine_mode?  Or does this lead to name
> clashes? (e.g. for "from_int")
>
> (Similar questions for the other more specialized wrapper classes; it
> looks like you have a set of independent wrapper classes around the
> machine_mode_enum, without using inheritance, with a trip through the
> enum needed to convert between wrappers)

Making it a subclass would be the natural thing to do if the class
actually defined the mode (and if we for example used pointers to
the definitions to represent the mode).  With wrappers it's kind-of
the "wrong way round" though: having scalar_int_mode a subclass of
machine_mode would allow things like:

  void foo (machine_mode *x) { *x = SFmode; }
  void bar () { scalar_int_mode y; foo (&y); ... }

and so isn't type-safe.

>> +{
>> +public:
>> +  ALWAYS_INLINE scalar_float_mode () {}
>> +  ALWAYS_INLINE operator machine_mode_enum () const { return m_mode;
>> }
>> +
>> +  static bool includes_p (machine_mode_enum);
>> +  static scalar_float_mode from_int (int i);
>> +
>> +PROTECT_ENUM_CONVERSION:
>> +  ALWAYS_INLINE scalar_float_mode (machine_mode_enum m) : m_mode (m)
>> {}
>
> Should this ctor have some kind of:
>   gcc_checking_assert (SCALAR_FLOAT_MODE_P (m));
> or somesuch?  (or does that slow down the debug build too much?)

The idea was to have the asserts in as_a <> calls instead, which is
the point at which forcible conversion occurs.  These routines are for
cases where we've already done the check or know due to static type
checking that a check isn't needed.

>> +
>> +protected:
>> +  machine_mode_enum m_mode;
>> +};
>> +
>> +/* Return true if M is a scalar_float_mode.  */
>> +
>> +inline bool
>> +scalar_float_mode::includes_p (machine_mode_enum m)
>> +{
>> +  return SCALAR_FLOAT_MODE_P (m);
>> +}
>> +
>> +/* Return M as a scalar_float_mode.  This function should only be
>> used by
>> +   utility functions; general code should use as_a<T> instead.  */
>> +
>> +ALWAYS_INLINE scalar_float_mode
>> +scalar_float_mode::from_int (int i)
>> +{
>> +  return machine_mode_enum (i);
>> +}
>
> ...or put an assertion in here?  (I *think* it's the only public
> function that can (implicitly) call the ctor without checking the
> invariant)

Same idea here: this should only be called when no check is needed,
and like you say shouldn't have an assert for speed reasons.  The
number of callers to from_int is small and so it's relatively easy
to check that the calls are safe.

> Hope this is constructive

Yeah, definitely :-)

Thanks,
Richard