[Fortran] Enable -fwrapv for -std=legacy
Checks
Commit Message
Hello world, here's the patch that was discussed.
Regression-tested. OK for trunk?
Since this appeared only in gcc13, I see no need for a backport.
I will also document this in the changes file.
Best regards
Thomas
Set -frapv if -std=legacy is set.
Fortran legacy codes sometimes contain linear congruential
seudorandom number generators. These generators implicitly depend
on wrapping behavior on integer overflow, which is illegal Fortran,
but the best they could to at the time.
A gcc13 change exposed this in rnflow, part of the Polyhedron
benchmark, with -O3. Rather than "regress" on such code, this patch
enables -fwrapv if -std=legacy is enabled. This allows the benchmark
to run successfully, and presumably lots of other code as well.
gcc/fortran/ChangeLog:
PR fortran/109075
* options.cc (gfc_handle_option): If -std=legacy is set,
also set -frwapv.
* invoke.texi: Document the change.
Comments
On Fri, Mar 10, 2023 at 06:54:10PM +0100, Thomas Koenig via Gcc-patches wrote:
> Hello world, here's the patch that was discussed.
>
> Regression-tested. OK for trunk?
>
> Since this appeared only in gcc13, I see no need for a backport.
> I will also document this in the changes file.
>
> Best regards
>
> Thomas
>
> Set -frapv if -std=legacy is set.
s/frapv/fwrapv/
> Fortran legacy codes sometimes contain linear congruential
> seudorandom number generators. These generators implicitly depend
> on wrapping behavior on integer overflow, which is illegal Fortran,
> but the best they could to at the time.
>
> A gcc13 change exposed this in rnflow, part of the Polyhedron
> benchmark, with -O3. Rather than "regress" on such code, this patch
> enables -fwrapv if -std=legacy is enabled. This allows the benchmark
> to run successfully, and presumably lots of other code as well.
I think it certainly shouldn't overwrite it, it can adjust the default,
but if user asks for -std=legacy -fno-wrapv or
-fno-wrapv -std=legacy, it should honor that.
> gcc/fortran/ChangeLog:
>
> PR fortran/109075
> * options.cc (gfc_handle_option): If -std=legacy is set,
s/ / /
> also set -frwapv.
s/rwapv/wrapv/
> * invoke.texi: Document the change.
> --- a/gcc/fortran/options.cc
> +++ b/gcc/fortran/options.cc
> @@ -797,6 +797,8 @@ gfc_handle_option (size_t scode, const char *arg, HOST_WIDE_INT value,
> case OPT_std_legacy:
> set_default_std_flags ();
> gfc_option.warn_std = 0;
> + /* -std=legacy implies -fwapv, but the user can override it. */
> + flag_wrapv = 1;
> break;
>
> case OPT_fshort_enums:
So, I think it should be done later, after option processing, say
in gfc_post_options if it is possible to determine if -std=legacy
has been specified (and not say -std=legacy -std=f2018 etc.),
and using SET_OPTION_IF_UNSET.
Jakub
> Am 10.03.2023 um 18:54 schrieb Thomas Koenig via Fortran <fortran@gcc.gnu.org>:
>
> Hello world, here's the patch that was discussed.
>
> Regression-tested. OK for trunk?
>
> Since this appeared only in gcc13, I see no need for a backport.
> I will also document this in the changes file.
The „problem“ is latent forever, I’m not sure it’s good to amend the kitchen-sink std=legacy option with -fwrapv since that has quite some negative effects on optimization.
Richard
> Best regards
>
> Thomas
>
> Set -frapv if -std=legacy is set.
>
> Fortran legacy codes sometimes contain linear congruential
> seudorandom number generators. These generators implicitly depend
> on wrapping behavior on integer overflow, which is illegal Fortran,
> but the best they could to at the time.
>
> A gcc13 change exposed this in rnflow, part of the Polyhedron
> benchmark, with -O3. Rather than "regress" on such code, this patch
> enables -fwrapv if -std=legacy is enabled. This allows the benchmark
> to run successfully, and presumably lots of other code as well.
>
> gcc/fortran/ChangeLog:
>
> PR fortran/109075
> * options.cc (gfc_handle_option): If -std=legacy is set,
> also set -frwapv.
> * invoke.texi: Document the change.
> <p1.diff>
On Fri, Mar 10, 2023 at 07:01:29PM +0100, Richard Biener via Fortran wrote:
>
>
> > Am 10.03.2023 um 18:54 schrieb Thomas Koenig via Fortran <fortran@gcc.gnu.org>:
> >
> > Hello world, here's the patch that was discussed.
> >
> > Regression-tested. OK for trunk?
> >
> > Since this appeared only in gcc13, I see no need for a backport.
> > I will also document this in the changes file.
>
> The „problem“ is latent forever, I’m not sure it’s good to
> amend the kitchen-sink std=legacy option with -fwrapv since
> that has quite some negative effects on optimization.
In that case, it would then seem logical to remove whatever
patch was added to -O3 that causes the massive regression with
rnflow.f90 and add it instead to -Ofast. -Ofast at least
hints that is unsafe to use.
Hi Richard,
>> Since this appeared only in gcc13, I see no need for a backport.
>> I will also document this in the changes file.
> The „problem“
It's a real problem, I am afraid...
>is latent forever, I’m not sure it’s good to amend the kitchen-sink >std=legacy option with -fwrapv since that has quite some negative
> effects on optimization.
So, what are the options?
Do nothing, and get silent bad results. I do not think we can do this,
there is too much code out there. Just look at Numerical Recipes, which
has this kind of random number generator.
Apply the patch (with spelling fixes). This has the drawback that
you outlined, potential impact on optimization.
Put a warning in the release notes. This will not help in general
because 99.99% of users will not read it.
Revert the patch exposing the problem. No. That would pessimize
everything.
Put the optimization behind a special flag. That also makes no sense,
-fwrapv does the job.
Would it be possible to add a warning? Anything of the sort
a = b + c * a
where b and c are larger than (in this case) 16 bits could be flagged.
Other options?
Best regards
Thomas
@@ -549,15 +549,16 @@ Fortran standard that includes all of the extensions supported by GNU
Fortran, although warnings will be given for obsolete extensions not
recommended for use in new code. The @samp{legacy} value is
equivalent but without the warnings for obsolete extensions, and may
-be useful for old non-standard programs. The @samp{f95},
-@samp{f2003}, @samp{f2008}, and @samp{f2018} values specify strict
-conformance to the Fortran 95, Fortran 2003, Fortran 2008 and Fortran
-2018 standards, respectively; errors are given for all extensions
-beyond the relevant language standard, and warnings are given for the
-Fortran 77 features that are permitted but obsolescent in later
-standards. The deprecated option @samp{-std=f2008ts} acts as an alias for
-@samp{-std=f2018}. It is only present for backwards compatibility with
-earlier gfortran versions and should not be used any more.
+be useful for old non-standard programs. It also sets
+@option{-fwrapv}. The @samp{f95}, @samp{f2003}, @samp{f2008}, and
+@samp{f2018} values specify strict conformance to the Fortran 95,
+Fortran 2003, Fortran 2008 and Fortran 2018 standards, respectively;
+errors are given for all extensions beyond the relevant language
+standard, and warnings are given for the Fortran 77 features that are
+permitted but obsolescent in later standards. The deprecated option
+@samp{-std=f2008ts} acts as an alias for @samp{-std=f2018}. It is only
+present for backwards compatibility with earlier gfortran versions and
+should not be used any more.
@opindex @code{ftest-forall-temp}
@item -ftest-forall-temp
@@ -797,6 +797,8 @@ gfc_handle_option (size_t scode, const char *arg, HOST_WIDE_INT value,
case OPT_std_legacy:
set_default_std_flags ();
gfc_option.warn_std = 0;
+ /* -std=legacy implies -fwapv, but the user can override it. */
+ flag_wrapv = 1;
break;
case OPT_fshort_enums: