Re: [PATCH] Eliminate MODE_HAS_... macros

[Richard Guenther <rguenther at suse dot de>]

On Sun, 10 Aug 2008, Ulrich Weigand wrote:

> Hello,
> 
> we currently have a certain amount of redundancy between information in the
> real_format data structure and the MODE_HAS_... target macros.  For example,
> the information provided by MODE_HAS_NANS is already available by looking at
> the has_nans member of the appropriate real_format structure.
> 
> This patch eliminates that redundancy by removing the MODE_HAS_... macros,
> and introducing REAL_MODE_FORMAT_HAS_... macros that look at the real_format
> members.  (One piece of information, whether the format supports any sign
> dependent rounding mode, was missing from real_format.  This patch adds that
> as another member.)
> 
> Note that there is only one target (SPU) that changes the default definition
> of the MODE_HAS_... macros, and the the default definition only looks at
> TARGET_FLOAT_FORMAT (which is only special on VAX), and macros that are
> nowhere defined (LARGEST_EXPONENT_IS_NORMAL, ROUND_TOWARDS_ZERO).  This has
> the effect all four MODE_HAS_... macros were always true on all non-VAX,
> non-SPU targets, and always false on all VAX targets.
> 
> This agrees with the current settings of the real_format has_nans, has_inf
> and has_signed_zeros members, and the new has_sign_dependent_rounding
> member introduced by the patch.  Overall, this patch should therefore have
> no effect on the behaviour of GCC on any target except SPU.
> 
> On the SPU, this patch actually fixes three problems:
> 
> - MODE_HAS_SIGN_DEPENDENT_ROUNDING was always false; it should be false
>   only for SFmode and true for DFmode (which does have round to pos/neg
>   infinity rounding modes).
> 
> - The MODE_HAS_ macros defined in the SPU back end did not expect to be
>   called for vector / complex float values, but they were.  The new
>   mechanism in the patch below inherits properties of the base floating
>   point format for the corresponding vector / complex types.
> 
> - The ROUND_TOWARDS_ZERO macro influences the behaviour of the FPBIT
>   routines.  On the SPU, single-precision routines should round towards
>   zero, while double-precision routines should round to nearest; this
>   cannot be specified with the current single-valued ROUND_TOWARDS_ZERO.
>   However, on inspection it seems that no single-precision FPBIT routine
>   is ever actually *used* by the SPU back-end, only two double-precision
>   routines are (__fixdfsi and __unorddf2).  Therefore, removing the
>   definition of ROUND_TOWARDS_ZERO actually fixes a problem.
> 
> Tested on spu-elf, s390-ibm-linux and s390x-ibm-linux with no regressions.
> OK for mainline?

Now with a closer look ...

> 	* real.h (struct real_format): New member has_sign_dependent_rounding.
> 	* real.c (ieee_single_format, mips_single_format, motorola_single_format,
>         spu_single_format, ieee_double_format, mips_double_format,
>         motorola_double_format, ieee_extended_motorola_format,
>         ieee_extended_intel_96_format, ieee_extended_intel_128_format,
>         ieee_extended_intel_96_round_53_format, ibm_extended_format,
>         mips_extended_format, ieee_quad_format, mips_quad_format,
>         vax_f_format, vax_d_format, vax_g_format): Initialize it.
>         * config/pdp11/pdp11.c (pdp11_f_format, pdp11_d_format): Likewise.
> 
> 	* defaults.h (MODE_HAS_NANS, MODE_HAS_INFINITIES,
> 	MODE_HAS_SIGNED_ZEROS, MODE_HAS_SIGN_DEPENDENT_ROUNDING): Remove.
> 	* config/spu/spu.h (MODE_HAS_NANS, MODE_HAS_INFINITIES,
> 	MODE_HAS_SIGN_DEPENDENT_ROUNDING): Remove.
> 	(ROUND_TOWARDS_ZERO): Likewise.
>
> 	* real.h (REAL_MODE_FORMAT_HAS_NANS): New macro.
> 	(REAL_MODE_FORMAT_HAS_INFINITIES, REAL_MODE_FORMAT_HAS_SIGNED_ZEROS,
> 	REAL_MODE_FORMAT_HAS_SIGN_DEPENDENT_ROUNDING): Likewise.

These macros are really long to spell (they take away half of the
80 columns allowed) -- as you are still passing a mode to them, not
a struct real_format I'd like you to shorten them to at least
REAL_MODE_HAS_... or even better just keep the old names MODE_HAS_...
adding an appropriate check for FLOAT_MODE_P.

I know not renaming the macros may be more error-prone, but as you have
done it once with renaming I'm sure that won't break anything.

It will also considerably shrink the patch ;)

> 	* flags.h: Include "real.h".
> 	(HONOR_NANS): Use REAL_MODE_FORMAT_HAS_xxx instead of MODE_HAS_xxx.
> 	(HONOR_INFINITIES, HONOR_SIGNED_ZEROS, HONOR_SIGN_DEPENDENT_ROUNDING):
> 	Likewise.
> 	* builtins.c (fold_builtin_inf): Likewise.
> 	* c-cppbuiltins.c (builtin_define_float_constants): Likewise.
> 	* c-lex.c (interpret_float): Likewise.
> 	* fold-const.c (const_binop, fold_binary): Likewise.
> 	* simplify-rtx.c (simplify_const_binary_operation): Likewise.
> 
> 	* machmode.h (GET_MODE_INNER): Cast result to enum machine_mode.
> 	* real.h (REAL_MODE_FORMAT): Protect MODE against macro expansion.
> 
> 	* doc/tm.texi (Storage Layout): Remove documentation of
> 	MODE_HAS_NANS, MODE_HAS_INFINITIES, MODE_HAS_SIGNED_ZEROS,
> 	MODE_HAS_SIGN_DEPENDENT_ROUNDING.  Update documentation of
> 	ROUND_TOWARDS_ZERO and LARGEST_EXPONENT_IS_NORMAL to clarify
> 	they only apply to libgcc2.a.


>  #include "coretypes.h"
>  #include "options.h"
> +#include "real.h"
>  
>  enum debug_info_type
>  {
> @@ -282,7 +283,9 @@ extern bool warn_disallowed_functions;
>     disabled for modes with NaNs.  The user can ask for them to be
>     done anyway using the -funsafe-math-optimizations switch.  */
>  #define HONOR_NANS(MODE) \
> -  (MODE_HAS_NANS (MODE) && !flag_finite_math_only)
> +  (!flag_finite_math_only && FLOAT_MODE_P (MODE) \

the FLOAT_MODE_P checks would become unneccessary here.  And ...

> +   && REAL_MODE_FORMAT_HAS_NANS \
> +       (SCALAR_FLOAT_MODE_P (MODE)? (MODE) : GET_MODE_INNER (MODE)))

... the handling of non-scalar float modes should be folded into the
MODE_HAS_... macros IMHO.

The middle-end parts are ok with the suggested changes.

Thanks,
Richard.