Re: [PATCH] Fix -ffast-math flags handling inconsistencies

[Richard Biener <richard.guenther@gmail.com>]

On Fri, Jan 31, 2020 at 6:01 PM Ulrich Weigand <uweigand@de.ibm.com> wrote:
>
> Hello,
>
> we've noticed some inconsistencies in how the component flags of -ffast-math
> are handled, see the discussion on the GCC list starting here:
> https://gcc.gnu.org/ml/gcc/2020-01/msg00365.html
>
> This patch fixes those inconsistencies.  Specifically, there are the
> following changes:
>
> 1. Some component flags for -ffast-math are *set* with -ffast-math
>    (changing the default), but are not reset with -fno-fast-math,
>    causing the latter to have surprising results.  (Note that since
>    "-ffast-math -fno-fast-math" is short-cut by the driver, you'll
>    only see the surprising results with "-Ofast -fno-fast-math".)
>    This is fixed here by both setting and resetting the flags.
>
>    This affects the following flags
>       -fcx-limited-range
>       -fexcess-precision=
>
> 2. Some component flags for -ffast-math are changed from their default,
>    but are *not* included in the fast_math_flags_set_p test, causing
>    __FAST_MATH__ to remain predefined even when the full set of fast
>    math options is not actually in effect.  This is fixed here by
>    adding those flags into the fast_math_flags_set_p test.
>
>    This affects the following flags:
>      -fcx-limited-range
>      -fassociative-math
>      -freciprocal-math
>
> 3. For some math flags, set_fast_math_flags has code that sets their
>    values only to what is already their default.  The overall effect
>    of this code is a complete no-op.  This patch removes that dead code.
>
>    This affects the following flags:
>      -frounding-math
>      -fsignaling-nans
>
>
> The overall effect of this patch is that now all component flags of
> -ffast-math are treated exactly equivalently:
>   * they are set (changed from their default) with -ffast-math
>   * they are reset to their default with -fno-fast-math
>   * __FAST_MATH__ is only defined if the value of the flag matches
>     what -ffast-math would have set it to

The last part is not obviously correct to me since it doesn't match
documentation which says

@item -ffast-math
@opindex ffast-math
Sets the options @option{-fno-math-errno}, @option{-funsafe-math-optimizations},
@option{-ffinite-math-only}, @option{-fno-rounding-math},
@option{-fno-signaling-nans}, @option{-fcx-limited-range} and
@option{-fexcess-precision=fast}.

This option causes the preprocessor macro @code{__FAST_MATH__} to be defined.

to me this reads as -ffast-math -fexcess-precision=standard defines
__FAST_MATH__.
The only relevant part to define __FAST_MATH__ is specifying -ffast-math, other
options are not relevant (which of course is contradicted by
implementation - where
I didn't actually follow its history in that respect).  So can you
adjust documentation
as to when exactly __FAST_MATH__ is defined?

Also...

> Tested on s390x-ibm-linux.
>
> OK for mainline?
>
> Bye,
> Ulrich
>
> ChangeLog:
>
>         * opts.c (set_fast_math_flags): In the !set case, also reset
>         x_flag_cx_limited_range and x_flag_excess_precision.  Remove dead
>         code resetting x_flag_signaling_nans and x_flag_rounding_math.
>         (fast_math_flags_set_p): Also test x_flag_cx_limited_range,
>         x_flag_associative_math, and x_flag_reciprocal_math.
>
> diff --git a/gcc/opts.c b/gcc/opts.c
> index 7affeb4..4452793 100644
> --- a/gcc/opts.c
> +++ b/gcc/opts.c
> @@ -2850,18 +2850,14 @@ set_fast_math_flags (struct gcc_options *opts, int set)
>      opts->x_flag_finite_math_only = set;
>    if (!opts->frontend_set_flag_errno_math)
>      opts->x_flag_errno_math = !set;
> -  if (set)
> -    {
> -      if (opts->frontend_set_flag_excess_precision == EXCESS_PRECISION_DEFAULT)
> -       opts->x_flag_excess_precision
> -         = set ? EXCESS_PRECISION_FAST : EXCESS_PRECISION_DEFAULT;
> -      if (!opts->frontend_set_flag_signaling_nans)
> -       opts->x_flag_signaling_nans = 0;
> -      if (!opts->frontend_set_flag_rounding_math)
> -       opts->x_flag_rounding_math = 0;
> -      if (!opts->frontend_set_flag_cx_limited_range)
> -       opts->x_flag_cx_limited_range = 1;
> -    }
> +  if (!opts->frontend_set_flag_cx_limited_range)
> +    opts->x_flag_cx_limited_range = set;
> +  if (!opts->frontend_set_flag_excess_precision)
> +    opts->x_flag_excess_precision
> +      = set ? EXCESS_PRECISION_FAST : EXCESS_PRECISION_DEFAULT;
> +
> +  // -ffast-math should also reset -fsignaling-nans and -frounding-math,
> +  // but since those are off by default, there's nothing to do for now.
...

but what happens to -fsignalling-nans -ffast-math then?  Better leave those
in I'd say.

Note frontends come into play with what is considered -ffast-math
and -fno-fast-math but below flags are tested irrespectively of that
interpretation.

Note there's -fcx-fortran-rules similar to -fcx-limited-range but not tested
above.  The canonical middle-end "flag" to look at is flag_complex_method.
Somehow -fcx-fortran-rules doesn't come into play at all above but it
affects -fcx-limited-range in another inconsistent way in that
-fcx-limited-range -fcx-fortran-rules and -fcx-fortran-rules -fcx-limited-range
behave the same (-fcx-fortran-rules takes precedence...).  I guess
-fcomplex-method=ENUM should be exposed and -fcx-* made
appropriate aliases here.

You're tapping into a mine-field ;)

Richard.

>  }
>
>  /* When -funsafe-math-optimizations is set the following
> @@ -2884,10 +2880,13 @@ bool
>  fast_math_flags_set_p (const struct gcc_options *opts)
>  {
>    return (!opts->x_flag_trapping_math
> +         && !opts->x_flag_signed_zeros
> +         && opts->x_flag_associative_math
> +         && opts->x_flag_reciprocal_math
>           && opts->x_flag_unsafe_math_optimizations
>           && opts->x_flag_finite_math_only
> -         && !opts->x_flag_signed_zeros
>           && !opts->x_flag_errno_math
> +         && opts->x_flag_cx_limited_range
>           && opts->x_flag_excess_precision == EXCESS_PRECISION_FAST);
>  }
>
> --
>   Dr. Ulrich Weigand
>   GNU/Linux compilers and toolchain
>   Ulrich.Weigand@de.ibm.com
>