| Message ID | 20200211184332.0968DD80313@oc3748833570.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] Fix -ffast-math flags handling inconsistencies | expand |
Hi! On Tue, Feb 11, 2020 at 07:43:31PM +0100, Ulrich Weigand wrote: > 1. If a component flag of -ffast-math (or -funsafe-math-optimizations) > is explicitly set (or reset) on the command line, this should override > any implicit change due to -f(no-)fast-math, no matter in which order > the flags come on the command line. This change affects all flags. Ack. > 2. Any component flag modified from its default by -ffast-math should > be reset to the default by -fno-fast-math. This was previously > not done for the following flags: > -fcx-limited-range > -fexcess-precision= Ack. > 3. Once -ffinite-math-only is true, the -f(no-)signaling-nans flag has > no meaning (if we have no NaNs at all, it does not matter whether > there is a difference between quiet and signaling NaNs). Therefore, > it does not make sense for -ffast-math to imply -fno-signaling-nans. > This is also a documentation change. Ack. > 4. -ffast-math is documented to imply -fno-rounding-math, however the > latter setting is the default anyway; therefore it does not make > sense to try to modify it from its default setting. Ack. It might be more robust to *do* set it, so that it still works if the default ever changes. > 5. The __FAST_MATH__ preprocessor macro should be defined if and only > if all the component flags of -ffast-math are set to the value that > is documented as the effect of -ffast-math. The following flags > were currently *not* so tested: > -fcx-limited-range > -fassociative-math > -freciprocal-math > -frounding-math > (Note that we should still *test* for -fno-rounding-math here even > though it is not set as per 4. -ffast-math -frounding-math should > not set the __FAST_MATH__ macro.) > This is also a documentation change. Ack. All looks good to me, but I'm not maintainer of this. This should be mentioned in the release notes I think, it does change behaviour (for the better, but any change is a change). Thanks, Segher
On 2/11/20 11:43 AM, Ulrich Weigand wrote: > Hello, > > this second version of the patch incorporates all changes discussed > in the original mail thread starting here: > https://gcc.gnu.org/ml/gcc-patches/2020-01/msg02097.html > > The patch now implements the following set of changes: > > 1. If a component flag of -ffast-math (or -funsafe-math-optimizations) > is explicitly set (or reset) on the command line, this should override > any implicit change due to -f(no-)fast-math, no matter in which order > the flags come on the command line. This change affects all flags. > > 2. Any component flag modified from its default by -ffast-math should > be reset to the default by -fno-fast-math. This was previously > not done for the following flags: > -fcx-limited-range > -fexcess-precision= > > 3. Once -ffinite-math-only is true, the -f(no-)signaling-nans flag has > no meaning (if we have no NaNs at all, it does not matter whether > there is a difference between quiet and signaling NaNs). Therefore, > it does not make sense for -ffast-math to imply -fno-signaling-nans. > This is also a documentation change. > > 4. -ffast-math is documented to imply -fno-rounding-math, however the > latter setting is the default anyway; therefore it does not make > sense to try to modify it from its default setting. > > 5. The __FAST_MATH__ preprocessor macro should be defined if and only > if all the component flags of -ffast-math are set to the value that > is documented as the effect of -ffast-math. The following flags > were currently *not* so tested: > -fcx-limited-range > -fassociative-math > -freciprocal-math > -frounding-math > (Note that we should still *test* for -fno-rounding-math here even > though it is not set as per 4. -ffast-math -frounding-math should > not set the __FAST_MATH__ macro.) > This is also a documentation change. > > > Tested on s390x-ibm-linux. > > OK for mainline? > > Bye, > Ulrich > > gcc/ChangeLog: > > * doc/invoke.texi (-ffast-math): Remove mention of -fno-signaling-nans. > Clarify conditions when __FAST_MATH__ preprocessor macro is defined. > > * opts.c (common_handle_option): Pass OPTS_SET to set_fast_math_flags > and set_unsafe_math_optimizations_flags. > (set_fast_math_flags): Add OPTS_SET argument, and use it to avoid > setting flags already explicitly set on the command line. In the !set > case, also reset x_flag_cx_limited_range and x_flag_excess_precision. > Never reset x_flag_signaling_nans or x_flag_rounding_math. > (set_unsafe_math_optimizations_flags): Add OPTS_SET argument, and use > it to avoid setting flags already explicitly set on the command line. > (fast_math_flags_set_p): Also test x_flag_cx_limited_range, > x_flag_associative_math, x_flag_reciprocal_math, and > x_flag_rounding_math. It appears this was dropped on the floor. It looks reasonable to me. Please retest and commit. Thanks! Jeff
Hi! On Fri, Nov 20, 2020 at 09:33:48PM -0700, Jeff Law wrote: > On 2/11/20 11:43 AM, Ulrich Weigand wrote: > > 1. If a component flag of -ffast-math (or -funsafe-math-optimizations) > > is explicitly set (or reset) on the command line, this should override > > any implicit change due to -f(no-)fast-math, no matter in which order > > the flags come on the command line. This change affects all flags. > > > > 2. Any component flag modified from its default by -ffast-math should > > be reset to the default by -fno-fast-math. This was previously > > not done for the following flags: > > -fcx-limited-range > > -fexcess-precision= > > > > 3. Once -ffinite-math-only is true, the -f(no-)signaling-nans flag has > > no meaning (if we have no NaNs at all, it does not matter whether > > there is a difference between quiet and signaling NaNs). Therefore, > > it does not make sense for -ffast-math to imply -fno-signaling-nans. > > This is also a documentation change. > > > > 4. -ffast-math is documented to imply -fno-rounding-math, however the > > latter setting is the default anyway; therefore it does not make > > sense to try to modify it from its default setting. > > > > 5. The __FAST_MATH__ preprocessor macro should be defined if and only > > if all the component flags of -ffast-math are set to the value that > > is documented as the effect of -ffast-math. The following flags > > were currently *not* so tested: > > -fcx-limited-range > > -fassociative-math > > -freciprocal-math > > -frounding-math > > (Note that we should still *test* for -fno-rounding-math here even > > though it is not set as per 4. -ffast-math -frounding-math should > > not set the __FAST_MATH__ macro.) > > This is also a documentation change. > It appears this was dropped on the floor. It looks reasonable to me. > Please retest and commit. Thanks! It all makes sense, and is a nice improvement :-) But please mention it in the release notes? No doubt people did use non-sensical flag combinations, and they will be affected. Thanks! Segher
On Fri, Nov 20, 2020 at 09:33:48PM -0700, Jeff Law wrote: > > * doc/invoke.texi (-ffast-math): Remove mention of -fno-signaling-nans. > > Clarify conditions when __FAST_MATH__ preprocessor macro is defined. > > > > * opts.c (common_handle_option): Pass OPTS_SET to set_fast_math_flags > > and set_unsafe_math_optimizations_flags. > > (set_fast_math_flags): Add OPTS_SET argument, and use it to avoid > > setting flags already explicitly set on the command line. In the !set > > case, also reset x_flag_cx_limited_range and x_flag_excess_precision. > > Never reset x_flag_signaling_nans or x_flag_rounding_math. > > (set_unsafe_math_optimizations_flags): Add OPTS_SET argument, and use > > it to avoid setting flags already explicitly set on the command line. > > (fast_math_flags_set_p): Also test x_flag_cx_limited_range, > > x_flag_associative_math, x_flag_reciprocal_math, and > > x_flag_rounding_math. > It appears this was dropped on the floor. It looks reasonable to me. > Please retest and commit. Thanks! This did handle flag_excess_precision incorrectly, causing in particular https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97970 I've reverted for now and will post a modified patch later. Sorry for the breakage! At a first glance, it appears the problem is that -fexcess-precision=fast is already the default, so it does not make sense for -fno-fast-math to "reset" this back to default. Probably the best is for fast-math to simply not touch excess precision at all: - if it is set explicitly on the command line, fast-math should not affect it in any case; - if it is not set explicitly on the command line, the default either with fast-math or no-fast-math is excess-precision=fast, so again it should not be touched. I'll still need to look into the language-specific handling of the excess precision setting to make sure this works for all languages. Bye, Ulrich
Hi Ulrich, On Tue, Nov 24, 2020 at 07:42:47PM +0100, Ulrich Weigand wrote: > This did handle flag_excess_precision incorrectly, causing in particular > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97970 > > I've reverted for now and will post a modified patch later. > Sorry for the breakage! > > At a first glance, it appears the problem is that -fexcess-precision=fast > is already the default, so it does not make sense for -fno-fast-math to > "reset" this back to default. Probably the best is for fast-math to > simply not touch excess precision at all: > - if it is set explicitly on the command line, fast-math should not > affect it in any case; > - if it is not set explicitly on the command line, the default either > with fast-math or no-fast-math is excess-precision=fast, so again > it should not be touched. > > I'll still need to look into the language-specific handling of the > excess precision setting to make sure this works for all languages. At least C uses "standard" (instead of "fast") for the strict ISO C dialects. Segher
On Tue, 24 Nov 2020 at 19:43, Ulrich Weigand via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > On Fri, Nov 20, 2020 at 09:33:48PM -0700, Jeff Law wrote: > > > * doc/invoke.texi (-ffast-math): Remove mention of -fno-signaling-nans. > > > Clarify conditions when __FAST_MATH__ preprocessor macro is defined. > > > > > > * opts.c (common_handle_option): Pass OPTS_SET to set_fast_math_flags > > > and set_unsafe_math_optimizations_flags. > > > (set_fast_math_flags): Add OPTS_SET argument, and use it to avoid > > > setting flags already explicitly set on the command line. In the !set > > > case, also reset x_flag_cx_limited_range and x_flag_excess_precision. > > > Never reset x_flag_signaling_nans or x_flag_rounding_math. > > > (set_unsafe_math_optimizations_flags): Add OPTS_SET argument, and use > > > it to avoid setting flags already explicitly set on the command line. > > > (fast_math_flags_set_p): Also test x_flag_cx_limited_range, > > > x_flag_associative_math, x_flag_reciprocal_math, and > > > x_flag_rounding_math. > > It appears this was dropped on the floor. It looks reasonable to me. > > Please retest and commit. Thanks! > > This did handle flag_excess_precision incorrectly, causing in particular > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97970 > > I've reverted for now and will post a modified patch later. > Sorry for the breakage! > > At a first glance, it appears the problem is that -fexcess-precision=fast > is already the default, so it does not make sense for -fno-fast-math to > "reset" this back to default. Probably the best is for fast-math to > simply not touch excess precision at all: > - if it is set explicitly on the command line, fast-math should not > affect it in any case; > - if it is not set explicitly on the command line, the default either > with fast-math or no-fast-math is excess-precision=fast, so again > it should not be touched. > > I'll still need to look into the language-specific handling of the > excess precision setting to make sure this works for all languages. > Hi Ulrich, The patch you've reverted was also causing a regression on arm/aarch64: FAIL: gcc.dg/ipa/iinline-attr.c scan-ipa-dump inline "hooray[^\\n]*inline copy in test" In case this helps you make tests. Thanks, Christophe > Bye, > Ulrich > > -- > Dr. Ulrich Weigand > GNU/Linux compilers and toolchain > Ulrich.Weigand@de.ibm.com
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 2cd8d7ec5ff..662fded21ea 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -11441,10 +11441,10 @@ is unpredictable. @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}. +@option{-fcx-limited-range} and @option{-fexcess-precision=fast}. -This option causes the preprocessor macro @code{__FAST_MATH__} to be defined. +Whenever all these options listed above are set to those values, +the preprocessor macro @code{__FAST_MATH__} will be defined. This option is not turned on by any @option{-O} option besides @option{-Ofast} since it can result in incorrect output for programs diff --git a/gcc/opts.c b/gcc/opts.c index 7affeb41a96..5a2ae9631ab 100644 --- a/gcc/opts.c +++ b/gcc/opts.c @@ -192,10 +192,12 @@ static void set_debug_level (enum debug_info_type type, int extended, const char *arg, struct gcc_options *opts, struct gcc_options *opts_set, location_t loc); -static void set_fast_math_flags (struct gcc_options *opts, int set); +static void set_fast_math_flags (struct gcc_options *opts, + struct gcc_options *opts_set, int set); static void decode_d_option (const char *arg, struct gcc_options *opts, location_t loc, diagnostic_context *dc); static void set_unsafe_math_optimizations_flags (struct gcc_options *opts, + struct gcc_options *opts_set, int set); static void enable_warning_as_error (const char *arg, int value, unsigned int lang_mask, @@ -2447,11 +2449,11 @@ common_handle_option (struct gcc_options *opts, break; case OPT_ffast_math: - set_fast_math_flags (opts, value); + set_fast_math_flags (opts, opts_set, value); break; case OPT_funsafe_math_optimizations: - set_unsafe_math_optimizations_flags (opts, value); + set_unsafe_math_optimizations_flags (opts, opts_set, value); break; case OPT_ffixed_: @@ -2839,44 +2841,44 @@ set_Wstrict_aliasing (struct gcc_options *opts, int onoff) /* The following routines are useful in setting all the flags that -ffast-math and -fno-fast-math imply. */ static void -set_fast_math_flags (struct gcc_options *opts, int set) +set_fast_math_flags (struct gcc_options *opts, + struct gcc_options *opts_set, int set) { - if (!opts->frontend_set_flag_unsafe_math_optimizations) + if (!opts->frontend_set_flag_unsafe_math_optimizations + && !opts_set->x_flag_unsafe_math_optimizations) { opts->x_flag_unsafe_math_optimizations = set; - set_unsafe_math_optimizations_flags (opts, set); + set_unsafe_math_optimizations_flags (opts, opts_set, set); } if (!opts->frontend_set_flag_finite_math_only) - opts->x_flag_finite_math_only = set; + SET_OPTION_IF_UNSET (opts, opts_set, 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; - } + SET_OPTION_IF_UNSET (opts, opts_set, flag_errno_math, !set); + if (!opts->frontend_set_flag_cx_limited_range) + SET_OPTION_IF_UNSET (opts, opts_set, flag_cx_limited_range, set); + if (!opts->frontend_set_flag_excess_precision) + SET_OPTION_IF_UNSET (opts, opts_set, flag_excess_precision, + set ? EXCESS_PRECISION_FAST + : EXCESS_PRECISION_DEFAULT); + + // -ffast-math should also reset -frounding-math, but since this + // is off by default, there's nothing to do for now. } /* When -funsafe-math-optimizations is set the following flags are set as well. */ static void -set_unsafe_math_optimizations_flags (struct gcc_options *opts, int set) +set_unsafe_math_optimizations_flags (struct gcc_options *opts, + struct gcc_options *opts_set, int set) { if (!opts->frontend_set_flag_trapping_math) - opts->x_flag_trapping_math = !set; + SET_OPTION_IF_UNSET (opts, opts_set, flag_trapping_math, !set); if (!opts->frontend_set_flag_signed_zeros) - opts->x_flag_signed_zeros = !set; + SET_OPTION_IF_UNSET (opts, opts_set, flag_signed_zeros, !set); if (!opts->frontend_set_flag_associative_math) - opts->x_flag_associative_math = set; + SET_OPTION_IF_UNSET (opts, opts_set, flag_associative_math, set); if (!opts->frontend_set_flag_reciprocal_math) - opts->x_flag_reciprocal_math = set; + SET_OPTION_IF_UNSET (opts, opts_set, flag_reciprocal_math, set); } /* Return true iff flags in OPTS are set as if -ffast-math. */ @@ -2884,10 +2886,14 @@ 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_rounding_math + && opts->x_flag_cx_limited_range && opts->x_flag_excess_precision == EXCESS_PRECISION_FAST); }
Hello, this second version of the patch incorporates all changes discussed in the original mail thread starting here: https://gcc.gnu.org/ml/gcc-patches/2020-01/msg02097.html The patch now implements the following set of changes: 1. If a component flag of -ffast-math (or -funsafe-math-optimizations) is explicitly set (or reset) on the command line, this should override any implicit change due to -f(no-)fast-math, no matter in which order the flags come on the command line. This change affects all flags. 2. Any component flag modified from its default by -ffast-math should be reset to the default by -fno-fast-math. This was previously not done for the following flags: -fcx-limited-range -fexcess-precision= 3. Once -ffinite-math-only is true, the -f(no-)signaling-nans flag has no meaning (if we have no NaNs at all, it does not matter whether there is a difference between quiet and signaling NaNs). Therefore, it does not make sense for -ffast-math to imply -fno-signaling-nans. This is also a documentation change. 4. -ffast-math is documented to imply -fno-rounding-math, however the latter setting is the default anyway; therefore it does not make sense to try to modify it from its default setting. 5. The __FAST_MATH__ preprocessor macro should be defined if and only if all the component flags of -ffast-math are set to the value that is documented as the effect of -ffast-math. The following flags were currently *not* so tested: -fcx-limited-range -fassociative-math -freciprocal-math -frounding-math (Note that we should still *test* for -fno-rounding-math here even though it is not set as per 4. -ffast-math -frounding-math should not set the __FAST_MATH__ macro.) This is also a documentation change. Tested on s390x-ibm-linux. OK for mainline? Bye, Ulrich gcc/ChangeLog: * doc/invoke.texi (-ffast-math): Remove mention of -fno-signaling-nans. Clarify conditions when __FAST_MATH__ preprocessor macro is defined. * opts.c (common_handle_option): Pass OPTS_SET to set_fast_math_flags and set_unsafe_math_optimizations_flags. (set_fast_math_flags): Add OPTS_SET argument, and use it to avoid setting flags already explicitly set on the command line. In the !set case, also reset x_flag_cx_limited_range and x_flag_excess_precision. Never reset x_flag_signaling_nans or x_flag_rounding_math. (set_unsafe_math_optimizations_flags): Add OPTS_SET argument, and use it to avoid setting flags already explicitly set on the command line. (fast_math_flags_set_p): Also test x_flag_cx_limited_range, x_flag_associative_math, x_flag_reciprocal_math, and x_flag_rounding_math.