[PATCH] Fix -ffast-math flags handling inconsistencies

[PATCH] Fix -ffast-math flags handling inconsistencies

[Ulrich Weigand]

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

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.
 }
 
 /* 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

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

[Richard Biener]

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
>

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

[Ulrich Weigand]

Richard Biener wrote:
> On Fri, Jan 31, 2020 at 6:01 PM Ulrich Weigand <uweigand@de.ibm.com> wrote:
> > 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?

Agreed.  This should probably be worded something along the lines of:
"Whenever all of those options are set to these values as listed above,
the preprocessor macro @code{__FAST_MATH__} will be defined."

> > -      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;
[...]
> > +  // -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.

Ah, it seems I was confused about the intended semantics here.

I thought that a *more specific* option like -fsignalling-nans was always
intended to override a more generic option like -ffast-math, no matter
whether it comes before or after it on the command line.

Is it instead the case that the intended behavior is the generic option
overrides the specific option if the generic option comes later on the
command line?

In that case, I agree that those should be left in.

However, then I think it would be good to add tests for 
!flag_signaling_nans / !flag_rounding_math to fast_math_flags_set_p,
because with these options on, we aren't really in fast-math territory
any more ...

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

I agree it would probably be the best if to use a -fcomplex-method=...
flag, handled analogously to -fexcess-precision in that it defaults to
an explicit DEFAULT value; unless this is overridden by an explicit
command line flag, the language front-end can then choose what DEFAULT
means for this particular language.

However, I'd prefer to not include that change into this patch as well :-)
This patch only changes the behavior related to -fcx-limited-range in one
very special case (-Ofast -fno-fast-math), where it makes it strictly
better.  It should not affect the (existing) interaction with
-fcx-fortran-rules at all -- so anything to improve this can be done
in a separate patch, I think.

> You're tapping into a mine-field ;)

That's for sure :-)

Thanks,
Ulrich


-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

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

[Joseph Myers]

On Fri, 7 Feb 2020, Ulrich Weigand wrote:

> I thought that a *more specific* option like -fsignalling-nans was always
> intended to override a more generic option like -ffast-math, no matter
> whether it comes before or after it on the command line.

Yes, that's correct.  (There are cases where it's ambiguous which option 
is more specific - see what I said in 
<https://gcc.gnu.org/ml/gcc/2016-03/msg00245.html> about -Werror= - but I 
don't think -ffast-math involves such cases.)

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Ulrich Weigand]

Joseph Myers wrote:
> On Fri, 7 Feb 2020, Ulrich Weigand wrote:
> 
> > I thought that a *more specific* option like -fsignalling-nans was always
> > intended to override a more generic option like -ffast-math, no matter
> > whether it comes before or after it on the command line.
> 
> Yes, that's correct.  (There are cases where it's ambiguous which option 
> is more specific - see what I said in 
> <https://gcc.gnu.org/ml/gcc/2016-03/msg00245.html> about -Werror= - but I 
> don't think -ffast-math involves such cases.)

I see.  However, this does not appear to be implemented in current code.
GCC (without my patch) behaves like this (using -fno-finite-math-only
as an example instead of -fsignaling-nans, given the separate point
about the latter option raised by Segher):

gcc -dD -E -x c /dev/null | grep FINITE
#define __FINITE_MATH_ONLY__ 0
gcc -dD -E -x c /dev/null -ffast-math | grep FINITE
#define __FINITE_MATH_ONLY__ 1
gcc -dD -E -x c /dev/null -ffast-math -fno-finite-math-only | grep FINITE
#define __FINITE_MATH_ONLY__ 0
gcc -dD -E -x c /dev/null -fno-finite-math-only -ffast-math | grep FINITE
#define __FINITE_MATH_ONLY__ 1

Given the above rule, in the last case __FINITE_MATH_ONLY__ should
also be 0, right?

I'm not sure how this can be implemented in the current option handling
framework.  The -ffast-math handling case would have to check whether
or not there is any explicit use of -f(no-)finite-math-only; is there
any infrastructure to readily perform such a test?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

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

[Joseph Myers]

On Mon, 10 Feb 2020, Ulrich Weigand wrote:

> Given the above rule, in the last case __FINITE_MATH_ONLY__ should
> also be 0, right?

Yes.

> I'm not sure how this can be implemented in the current option handling
> framework.  The -ffast-math handling case would have to check whether
> or not there is any explicit use of -f(no-)finite-math-only; is there
> any infrastructure to readily perform such a test?

Pass opts_set as well as opts to set_fast_math_flags, and use it for 
checking whether an option was explicitly specified (whether enabled or 
disabled) on the command line.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Ulrich Weigand]

Joseph Myers wrote:
> On Mon, 10 Feb 2020, Ulrich Weigand wrote:
> > I'm not sure how this can be implemented in the current option handling
> > framework.  The -ffast-math handling case would have to check whether
> > or not there is any explicit use of -f(no-)finite-math-only; is there
> > any infrastructure to readily perform such a test?
> 
> Pass opts_set as well as opts to set_fast_math_flags, and use it for 
> checking whether an option was explicitly specified (whether enabled or 
> disabled) on the command line.

Ah, I see.  I'll update the patch accordingly.  Thanks!

I guess I still need to check for the opts->frontend_set_... flags
as well -- those are not reflected in opts_set, right?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

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

[Segher Boessenkool]

On Fri, Feb 07, 2020 at 05:47:32PM +0100, Ulrich Weigand wrote:
> > but what happens to -fsignalling-nans -ffast-math then?  Better leave those
> > in I'd say.
> 
> Ah, it seems I was confused about the intended semantics here.
> 
> I thought that a *more specific* option like -fsignalling-nans was always
> intended to override a more generic option like -ffast-math, no matter
> whether it comes before or after it on the command line.

-fsignaling-nans is an independent option.  Signaling NaNs don't do much
at all when you also have -ffinite-math-only, of course, which says you
don't have *any* NaNs.


Segher

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

[Ulrich Weigand]

Segher Boessenkool wrote:
> On Fri, Feb 07, 2020 at 05:47:32PM +0100, Ulrich Weigand wrote:
> > > but what happens to -fsignalling-nans -ffast-math then?  Better leave those
> > > in I'd say.
> > 
> > Ah, it seems I was confused about the intended semantics here.
> > 
> > I thought that a *more specific* option like -fsignalling-nans was always
> > intended to override a more generic option like -ffast-math, no matter
> > whether it comes before or after it on the command line.
> 
> -fsignaling-nans is an independent option.  Signaling NaNs don't do much
> at all when you also have -ffinite-math-only, of course, which says you
> don't have *any* NaNs.

That's a good point, thanks for pointing this out!  I agree that
-fsignaling-nans is not a good example then; it really should be
independent of -ffast-math.

However, there's still one other option that has a similar issue:
-frounding-math.  This *is* properly related to -ffast-math, in
that -ffast-math really ought to imply -fno-rounding-math, but the
latter is (currently) also the default in GCC.

Bye,
Ulrich


-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com