fast_math_flags_set_p vs. set_fast_math_flags inconsistency?

fast_math_flags_set_p vs. set_fast_math_flags inconsistency?

[Ulrich Weigand]

Hello,

the -ffast-math command line option sets a bunch of other flags
internally, as implemented in set_fast_math_flags.  It is possible
to selectively override those flags on the command line as well.

I'm now wondering under what circumstances the __FAST_MATH__ macro
should still be defined.  This is currently implemented in the
fast_math_flags_set_p routine, which checks the status of *some*
(but not all!) of the flags implied by -ffast-math.

This has the effect that e.g. after

  -ffast-math -fno-finite-math-only

the __FAST_MATH__ macro is no longer predefined, but after

  -ffast-math -fno-associative-math

the __FAST_MATH__ macro still *is* predefined, even though both
-ffinite-math-only and -fassociative-math are implied by -ffast-math.

Is this deliberate?  (If so, is it documented somewhere?)

Or is this just a bug and fast_math_flags_set_p ought to check
all flags implied by -ffast-math?

Bye,
Ulrich

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

Re: fast_math_flags_set_p vs. set_fast_math_flags inconsistency?

[Joseph Myers]

On Mon, 20 Jan 2020, Ulrich Weigand wrote:

> This has the effect that e.g. after
> 
>   -ffast-math -fno-finite-math-only
> 
> the __FAST_MATH__ macro is no longer predefined, but after
> 
>   -ffast-math -fno-associative-math
> 
> the __FAST_MATH__ macro still *is* predefined, even though both
> -ffinite-math-only and -fassociative-math are implied by -ffast-math.
> 
> Is this deliberate?  (If so, is it documented somewhere?)

I'd expect both to cause it to be undefined.  My guess would be that some 
past patch, after fast_math_flags_set_p was added, added more flags to 
-ffast-math but accidentally failed to update fast_math_flags_set_p; you'd 
need to look at past patches adding flags to -ffast-math to confirm if 
it's accidental.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: fast_math_flags_set_p vs. set_fast_math_flags inconsistency?

[Ulrich Weigand]

Joseph Myers wrote:
> On Mon, 20 Jan 2020, Ulrich Weigand wrote:
> 
> > This has the effect that e.g. after
> > 
> >   -ffast-math -fno-finite-math-only
> > 
> > the __FAST_MATH__ macro is no longer predefined, but after
> > 
> >   -ffast-math -fno-associative-math
> > 
> > the __FAST_MATH__ macro still *is* predefined, even though both
> > -ffinite-math-only and -fassociative-math are implied by -ffast-math.
> > 
> > Is this deliberate?  (If so, is it documented somewhere?)
> 
> I'd expect both to cause it to be undefined.  My guess would be that some 
> past patch, after fast_math_flags_set_p was added, added more flags to 
> -ffast-math but accidentally failed to update fast_math_flags_set_p; you'd 
> need to look at past patches adding flags to -ffast-math to confirm if 
> it's accidental.

It looks like there's multiple cases here.  For the two flags
-fassociative-math and -freciprocal-math, it seems to have happened just as
you describe: they were created (split out of -funsafe-math-optimizations)
in commit a1a826110720eda37c73f829daa4ee243ee953f5, which however did not
update fast_math_flags_set_p.

For the other three flags, -fsignaling-nans, -frounding-math, and
-fcx-limited-range, the story appears to be a bit different: from the
very beginning, those were treated somewhat differently: these are
only modified as a result of -ffast-math, not -fno-fast-math (like
the other flags derived from -ffast-math), and they are not considered
by fast_math_flags_set_p.  (I'm not sure if there is any particular
reason why these should be treated differently here, though.)

Finally, there is one "mixed" flag, -fexcess-precision, which is handled
like the above three in that its default is only modified as a result of
-ffast-math, not -fno-fast-math; but nevertheless this flag *is* checked
in fast_math_flags_set_p.

Do you think there is something that ought to be fixed here?

Bye,
Ulrich

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

Re: fast_math_flags_set_p vs. set_fast_math_flags inconsistency?

[Joseph Myers]

On Tue, 21 Jan 2020, Ulrich Weigand wrote:

> It looks like there's multiple cases here.  For the two flags
> -fassociative-math and -freciprocal-math, it seems to have happened just as
> you describe: they were created (split out of -funsafe-math-optimizations)
> in commit a1a826110720eda37c73f829daa4ee243ee953f5, which however did not
> update fast_math_flags_set_p.

So that's a bug.

> For the other three flags, -fsignaling-nans, -frounding-math, and
> -fcx-limited-range, the story appears to be a bit different: from the

The first two of those are disabled by default as well as disabled by 
-ffast-math, so it seems right that -fno-fast-math does nothing with them 
and that they aren't checked by fast_math_flags_set_p.

The last one is disabled by default but enabled by -ffast-math.  So it 
would seem appropriate to handle it like other such options, disable it 
with -fno-fast-math, and check it in fast_math_flags_set_p.

> Finally, there is one "mixed" flag, -fexcess-precision, which is handled
> like the above three in that its default is only modified as a result of
> -ffast-math, not -fno-fast-math; but nevertheless this flag *is* checked
> in fast_math_flags_set_p.

That one's trickier because the default depends on whether a C standards 
conformance mode is specified.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: fast_math_flags_set_p vs. set_fast_math_flags inconsistency?

[Ulrich Weigand]

Joseph Myers wrote:
> On Tue, 21 Jan 2020, Ulrich Weigand wrote:
> 
> > It looks like there's multiple cases here.  For the two flags
> > -fassociative-math and -freciprocal-math, it seems to have happened just as
> > you describe: they were created (split out of -funsafe-math-optimizations)
> > in commit a1a826110720eda37c73f829daa4ee243ee953f5, which however did not
> > update fast_math_flags_set_p.
> 
> So that's a bug.

OK, agreed.

> > For the other three flags, -fsignaling-nans, -frounding-math, and
> > -fcx-limited-range, the story appears to be a bit different: from the
> 
> The first two of those are disabled by default as well as disabled by 
> -ffast-math, so it seems right that -fno-fast-math does nothing with them 
> and that they aren't checked by fast_math_flags_set_p.

I see.  I guess that makes me wonder what -fno-fast-math *ever* does
(except canceling a -ffast-math earlier on the command line).  Looking
at the current code, -fno-fast-math (just like -ffast-math) only ever
sets flags whose default is not overridden on the command line, but
then it always sets them to their default value!

Am I missing something here?  If that's the intent, it might be cleaner
to write set_fast_math_flags as just one big
  if (set)
    {
    }

> The last one is disabled by default but enabled by -ffast-math.  So it 
> would seem appropriate to handle it like other such options, disable it 
> with -fno-fast-math, and check it in fast_math_flags_set_p.

OK.

> > Finally, there is one "mixed" flag, -fexcess-precision, which is handled
> > like the above three in that its default is only modified as a result of
> > -ffast-math, not -fno-fast-math; but nevertheless this flag *is* checked
> > in fast_math_flags_set_p.
> 
> That one's trickier because the default depends on whether a C standards 
> conformance mode is specified.

This also makes sense if we consider the semantics of -fno-fast-math to
just leave all component flags at their default, as above ...

(As an aside, the current code is even more confusing as it has a dead
condition:

  if (set)
    {
      if (opts->frontend_set_flag_excess_precision == EXCESS_PRECISION_DEFAULT)
        opts->x_flag_excess_precision
          = set ? EXCESS_PRECISION_FAST : EXCESS_PRECISION_DEFAULT;

The second test of "set" must always be true here, so this will never actually
actively set the flag to EXCESS_PRECISION_DEFAULT.)

Bye,
Ulrich

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

Re: fast_math_flags_set_p vs. set_fast_math_flags inconsistency?

[Joseph Myers]

On Mon, 27 Jan 2020, Ulrich Weigand wrote:

> I see.  I guess that makes me wonder what -fno-fast-math *ever* does
> (except canceling a -ffast-math earlier on the command line).  Looking
> at the current code, -fno-fast-math (just like -ffast-math) only ever
> sets flags whose default is not overridden on the command line, but
> then it always sets them to their default value!

As a general principle, more specific flags take precedence over less 
specific ones, regardless of the command-line order.  So it's correct for 
-ffast-math and -fno-fast-math not to do anything with a flag that was 
explicitly overridden by the user (modulo any issues where a particular 
combination of flags is unsupported by GCC, as with the 
"%<-fassociative-math%> disabled; other options take precedence" case in 
toplev.c).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: fast_math_flags_set_p vs. set_fast_math_flags inconsistency?

[Ulrich Weigand]

Joseph Myers wrote:
> On Mon, 27 Jan 2020, Ulrich Weigand wrote:
> > I see.  I guess that makes me wonder what -fno-fast-math *ever* does
> > (except canceling a -ffast-math earlier on the command line).  Looking
> > at the current code, -fno-fast-math (just like -ffast-math) only ever
> > sets flags whose default is not overridden on the command line, but
> > then it always sets them to their default value!
> 
> As a general principle, more specific flags take precedence over less 
> specific ones, regardless of the command-line order.  So it's correct for 
> -ffast-math and -fno-fast-math not to do anything with a flag that was 
> explicitly overridden by the user (modulo any issues where a particular 
> combination of flags is unsupported by GCC, as with the 
> "%<-fassociative-math%> disabled; other options take precedence" case in 
> toplev.c).

Sure, I agree with the intended behavior as you describe.  I was just 
confused under what circumstances set_fast_math_flags with set == 0
ever does anything.  However, I've now understood that this is required
e.g. for the -Ofast -fno-fast-math case (this seems to be the only case
where set_fast_math_flags is called twice, first with set == 1 and then
with set == 0).

And indeed, in this case we see the effect of not resetting the
flag_cx_limited_range (on x86_64):

$ gcc -E -dD -x c /dev/null -std=c99 | grep GCC_IEC
#define __GCC_IEC_559 2
#define __GCC_IEC_559_COMPLEX 2
$ gcc -E -dD -x c /dev/null -std=c99 -Ofast | grep GCC_IEC
#define __GCC_IEC_559 0
#define __GCC_IEC_559_COMPLEX 0
$ gcc -E -dD -x c /dev/null -std=c99 -Ofast -fno-fast-math | grep GCC_IEC
#define __GCC_IEC_559 2
#define __GCC_IEC_559_COMPLEX 0

Note how __GCC_IEC_559_COMPLEX is not properly reset.

Similarly, we see the effect of not resetting flag_excess_precision
(only on i386):

$ gcc -E -dD -x c /dev/null -m32 -std=c99 | grep GCC_IEC
#define __GCC_IEC_559 2
#define __GCC_IEC_559_COMPLEX 2
$ gcc -E -dD -x c /dev/null -m32 -std=c99 -Ofast | grep GCC_IEC
#define __GCC_IEC_559 0
#define __GCC_IEC_559_COMPLEX 0
$ gcc -E -dD -x c /dev/null -m32 -std=c99 -Ofast -fno-fast-math | grep GCC_IEC
#define __GCC_IEC_559 0
#define __GCC_IEC_559_COMPLEX 0

Note how __GCC_IEC_559 is not properly reset either.

So it seems to me that indeed both of these should be reset
in the !set case of set_fast_math_flags.

In addition, we've already agreed that these three flags should be
checked in fast_math_flags_set_p, where they are currently missing:

  flag_associative_math
  flag_reciprocal_math
  flag_cx_limited_range

Finally, this remaining piece of code in set_fast_math_flags:

 if (set)
   {
     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;
   }

seems always a no-op since it only ever sets the flags to their
default value when they still must have that same default value.

For clarity, I'd suggest to either remove this code or replace
it with a comment.


The following patch (not yet fully tested) implements the above changes.
Note that this now brings the set of flags set and reset by
set_fast_math_flags completely in sync with the set of flags
tested in fast_math_flags_set_p.

Does this make sense to you?

Thanks,
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: fast_math_flags_set_p vs. set_fast_math_flags inconsistency?

[Joseph Myers]

On Tue, 28 Jan 2020, Ulrich Weigand wrote:

> The following patch (not yet fully tested) implements the above changes.
> Note that this now brings the set of flags set and reset by
> set_fast_math_flags completely in sync with the set of flags
> tested in fast_math_flags_set_p.
> 
> Does this make sense to you?

It makes sense, but this is not a review of the patch.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: fast_math_flags_set_p vs. set_fast_math_flags inconsistency?

[Ulrich Weigand]

Joseph Myers wrote:
> On Tue, 28 Jan 2020, Ulrich Weigand wrote:
> 
> > The following patch (not yet fully tested) implements the above changes.
> > Note that this now brings the set of flags set and reset by
> > set_fast_math_flags completely in sync with the set of flags
> > tested in fast_math_flags_set_p.
> > 
> > Does this make sense to you?
> 
> It makes sense, but this is not a review of the patch.

Understood, thanks for having a look!  I'll go ahead and submit the
patch for review as usual then.

Thanks,
Ulrich

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