[Patch, Fortran] PR 85599: warn about short-circuiting of logical expressions for non-pure functions

[Janus Weil]

Hi all,

after the dust of the heated discussion around this PR has settled a
bit, here is another attempt to implement at least some basic warnings
about compiler-dependent behavior concerning the short-circuiting of
logical expressions.

As a reminder (and recap of the previous discussion), the Fortran
standard unfortunately is a bit sloppy in this area: It allows
compilers to short-circuit the second operand of .AND. / .OR.
operators, but does not require this. As a result, compilers can do
what they want without conflicting with the standard, and they do:
gfortran does short-circuiting (via TRUTH_ANDIF_EXPR/TRUTH_ORIF_EXPR),
ifort does not.

I'm continuing here the least-invasive approach of keeping gfortran's
current behavior, but warning about cases where compilers may produce
different results.

The attached patch is very close to the version I posted previously
(which was already approved by Janne), with the difference that the
warnings are now triggered by -Wextra and not -Wsurprising (which is
included in -Wall), as suggested by Nick Maclaren. I think this is
more reasonable, since not everyone may want to see these warnings.

Note that I don't want to warn about all possible optimizations that
might be allowed by the standard, but only about those that are
actually problematic in practice and result in compiler-dependent
behavior.

The patch regtests cleanly on x86_64-linux-gnu. Ok for trunk?

Cheers,
Janus


2018-07-11 Thomas Koenig <***@gcc.gnu.org>
Janus Weil <***@gcc.gnu.org>

PR fortran/85599
* dump-parse-tree (show_attr): Add handling of implicit_pure.
* resolve.c (impure_function_callback): New function.
(resolve_operator): Call it vial gfc_expr_walker.


2018-07-11 Janus Weil <***@gcc.gnu.org>

PR fortran/85599
* gfortran.dg/short_circuiting.f90: New test.

[Dominique d'Humières]

Hi Janus,

Post by Janus Weil
after the dust of the heated discussion around this PR has settled a
bit, here is another attempt to implement at least some basic warnings
about compiler-dependent behavior concerning the short-circuiting of
logical expressions. …

IMO your patch is missing the only point I agree with you on this issue, i.e.,
the short-circuit evaluation and the related portability issues should be
documented.

With your patch what happens if check() is an external function?

Your patch is focusing on pr85599 and ignore pr57160.

What to do with

if(x>0 .and. sort(x)<10.0*log(x)) …

which is not portable to compilers computing the two expressions?

Cheers,

Dominique

[Janus Weil]

Hi Dominique,

thanks for the comments.

Post by Dominique d'Humières

Post by Janus Weil
after the dust of the heated discussion around this PR has settled a
bit, here is another attempt to implement at least some basic warnings
about compiler-dependent behavior concerning the short-circuiting of
logical expressions. …

IMO your patch is missing the only point I agree with you on this issue, i.e.,

So you don't agree that it's a good idea to warn about non-portable code?

Post by Dominique d'Humières
the short-circuit evaluation and the related portability issues should be
documented.

I fully agree. Documentation never hurts. Do you have a suggestion
where this should go? There does not seem to be a particular chapter
in the manual that deals with portability across compilers (probably
because one usually assumes that sticking to the Fortran standard is
sufficient for achieving portability). The best match might be the
chapter on compiler characteristics:

https://gcc.gnu.org/onlinedocs/gcc-8.1.0/gfortran/Compiler-Characteristics.html

Post by Dominique d'Humières
With your patch what happens if check() is an external function?

Haven't tried, but I guess you would get a warning, since the compiler
cannot tell whether 'check' is pure (unless you have an explicit
interface block that contains the PURE attribute).

Post by Dominique d'Humières
Your patch is focusing on pr85599 and ignore pr57160.

Well, yes. PR 57160 is somewhat related, but orthogonal. It concerns
essentially the same issue, namely short-circuiting with logical
expressions, but proposes a different solution (making
short-circuiting depend on the -ffrontend-optimize flag). I wouldn't
say that's unreasonable, but independent of that I might still want to
know that my code is not portable, thus a warning makes sense in any
case (IMHO).

Post by Dominique d'Humières
What to do with
if(x>0 .and. sort(x)<10.0*log(x)) …
which is not portable to compilers computing the two expressions?

That's one of the typical cases that the patch should be able to
handle. If 'sort' is impure, this should trigger a warning with the
patch. If it is pure, it doesn't matter whether the rhs of the .and.
operator is executed (since there are no side effects). Or am I
missing something?

Cheers,
Janus

Post by Dominique d'Humières
Hi Janus,

Post by Janus Weil
after the dust of the heated discussion around this PR has settled a
bit, here is another attempt to implement at least some basic warnings
about compiler-dependent behavior concerning the short-circuiting of
logical expressions. …

IMO your patch is missing the only point I agree with you on this issue, i.e.,
the short-circuit evaluation and the related portability issues should be
documented.
With your patch what happens if check() is an external function?
Your patch is focusing on pr85599 and ignore pr57160.
What to do with
if(x>0 .and. sort(x)<10.0*log(x)) …
which is not portable to compilers computing the two expressions?
Cheers,
Dominique

[Dominique d'Humières]

Post by Janus Weil
Hi Dominique,
thanks for the comments.

Post by Dominique d'Humières

Post by Janus Weil
after the dust of the heated discussion around this PR has settled a
bit, here is another attempt to implement at least some basic warnings
about compiler-dependent behavior concerning the short-circuiting of
logical expressions. …

IMO your patch is missing the only point I agree with you on this issue, i.e.,

So you don't agree that it's a good idea to warn about non-portable code?

Wel, as you might guess I don’t like warnings in general: too much false positives
and missed targets (see recent bootstrap breakage).

Post by Janus Weil

Post by Dominique d'Humières
the short-circuit evaluation and the related portability issues should be
documented.

I fully agree. Documentation never hurts. Do you have a suggestion
where this should go? There does not seem to be a particular chapter
in the manual that deals with portability across compilers (probably
because one usually assumes that sticking to the Fortran standard is
sufficient for achieving portability). The best match might be the
https://gcc.gnu.org/onlinedocs/gcc-8.1.0/gfortran/Compiler-Characteristics.html

Why not? Anyway if your patch is accepted, the doc for -Wextra has to be updated accordingly.

Post by Janus Weil

Post by Dominique d'Humières
With your patch what happens if check() is an external function?

Haven't tried, but I guess you would get a warning, since the compiler
cannot tell whether 'check' is pure (unless you have an explicit
interface block that contains the PURE attribute).

Much ado about nothing?

Post by Janus Weil

Post by Dominique d'Humières
Your patch is focusing on pr85599 and ignore pr57160.

Well, yes. PR 57160 is somewhat related, but orthogonal. It concerns
essentially the same issue, namely short-circuiting with logical
expressions, but proposes a different solution (making
short-circuiting depend on the -ffrontend-optimize flag). I wouldn't
say that's unreasonable, but independent of that I might still want to
know that my code is not portable, thus a warning makes sense in any
case (IMHO).

Post by Dominique d'Humières
What to do with
if(x>0 .and. sort(x)<10.0*log(x)) …
which is not portable to compilers computing the two expressions?

That's one of the typical cases that the patch should be able to
handle. If 'sort' is impure, this should trigger a warning with the
patch. If it is pure, it doesn't matter whether the rhs of the .and.
operator is executed (since there are no side effects). Or am I
missing something?

I meant ‘sqrt’ (changed behind my back by the stupid spell-checker).

Post by Janus Weil
Cheers,
Janus

Post by Dominique d'Humières
Hi Janus,

Post by Janus Weil
after the dust of the heated discussion around this PR has settled a
bit, here is another attempt to implement at least some basic warnings
about compiler-dependent behavior concerning the short-circuiting of
logical expressions. …

IMO your patch is missing the only point I agree with you on this issue, i.e.,
the short-circuit evaluation and the related portability issues should be
documented.
With your patch what happens if check() is an external function?
Your patch is focusing on pr85599 and ignore pr57160.
What to do with
if(x>0 .and. sort(x)<10.0*log(x)) …
which is not portable to compilers computing the two expressions?
Cheers,
Dominique

[Janus Weil]

Post by Dominique d'Humières

Post by Janus Weil

Post by Dominique d'Humières

Post by Janus Weil
after the dust of the heated discussion around this PR has settled a
bit, here is another attempt to implement at least some basic warnings
about compiler-dependent behavior concerning the short-circuiting of
logical expressions. …

IMO your patch is missing the only point I agree with you on this issue, i.e.,

So you don't agree that it's a good idea to warn about non-portable code?

Wel, as you might guess I don’t like warnings in general: too much false positives
and missed targets (see recent bootstrap breakage).

So then: Do you have a better solution for the problem at hand?

The cleaner approach would certainly be to avoid short-circuiting of
impure functions altogether. If we can all agree that this is a good
idea, I'll be happy to submit a patch in that direction. But recent
discussions have shown that there is a broad opposition of hardliners
who favor optimization to portability. I don't really have the nerves
and the time to continue this endless fight, so for now I would be
satisfied if we at least had the warnings.

Post by Dominique d'Humières

Post by Janus Weil

Post by Dominique d'Humières
What to do with
if(x>0 .and. sort(x)<10.0*log(x)) …
which is not portable to compilers computing the two expressions?

That's one of the typical cases that the patch should be able to
handle. If 'sort' is impure, this should trigger a warning with the
patch. If it is pure, it doesn't matter whether the rhs of the .and.
operator is executed (since there are no side effects). Or am I
missing something?

I meant ‘sqrt’ (changed behind my back by the stupid spell-checker).

sqrt and log are both elemental, thus pure, so there will be no warning.

Cheers,
Janus

[Thomas Koenig]

Hi Janus,

Post by Janus Weil
The cleaner approach would certainly be to avoid short-circuiting of
impure functions altogether. If we can all agree that this is a good
idea,

This is a fine example of logical short-circuiting - the condition you
mention is false, therefore the rest need not be evaluated :-)

[Janus Weil]

Post by Dominique d'Humières
Hi Janus,

Post by Janus Weil
The cleaner approach would certainly be to avoid short-circuiting of
impure functions altogether. If we can all agree that this is a good
idea,

This is a fine example of logical short-circuiting - the condition you
mention is false, therefore the rest need not be evaluated :-)

Thought so :(

Also: Yay, the discussion reached the first meta level ;)

Since you seem to be capable of giving warnings about
short-circuiting, we only need to teach that trick to gfortran now
...!

Cheers,
Janus

[Janus Weil]

Hi all,

here is a minor update of the patch, which cures some problems with
implicitly pure functions in the previous version.

Most implicitly pure functions were actually detected correctly, but
implicitly pure functions that called other implicitly pure functions
were not detected properly, and therefore could trigger a warning.
This is fixed in the current version, which still regtests cleanly
(note that alloc-comp-3.f90 currently fails due to PR 86417). The test
case is also enhanced to include the problematic case.

Ok for trunk?

Cheers,
Janus

Post by Janus Weil
Hi all,
after the dust of the heated discussion around this PR has settled a
bit, here is another attempt to implement at least some basic warnings
about compiler-dependent behavior concerning the short-circuiting of
logical expressions.
As a reminder (and recap of the previous discussion), the Fortran
standard unfortunately is a bit sloppy in this area: It allows
compilers to short-circuit the second operand of .AND. / .OR.
operators, but does not require this. As a result, compilers can do
gfortran does short-circuiting (via TRUTH_ANDIF_EXPR/TRUTH_ORIF_EXPR),
ifort does not.
I'm continuing here the least-invasive approach of keeping gfortran's
current behavior, but warning about cases where compilers may produce
different results.
The attached patch is very close to the version I posted previously
(which was already approved by Janne), with the difference that the
warnings are now triggered by -Wextra and not -Wsurprising (which is
included in -Wall), as suggested by Nick Maclaren. I think this is
more reasonable, since not everyone may want to see these warnings.
Note that I don't want to warn about all possible optimizations that
might be allowed by the standard, but only about those that are
actually problematic in practice and result in compiler-dependent
behavior.
The patch regtests cleanly on x86_64-linux-gnu. Ok for trunk?
Cheers,
Janus
PR fortran/85599
* dump-parse-tree (show_attr): Add handling of implicit_pure.
* resolve.c (impure_function_callback): New function.
(resolve_operator): Call it vial gfc_expr_walker.
PR fortran/85599
* gfortran.dg/short_circuiting.f90: New test.

[Janus Weil]

Just noticed another problematic case: Calls to generic interfaces are
not detected as implicitly pure, see enhanced test case in attachment.
Will look into this problem on the weekend ...

Cheers,
Janus

Post by Janus Weil
Hi all,
here is a minor update of the patch, which cures some problems with
implicitly pure functions in the previous version.
Most implicitly pure functions were actually detected correctly, but
implicitly pure functions that called other implicitly pure functions
were not detected properly, and therefore could trigger a warning.
This is fixed in the current version, which still regtests cleanly
(note that alloc-comp-3.f90 currently fails due to PR 86417). The test
case is also enhanced to include the problematic case.
Ok for trunk?
Cheers,
Janus

Post by Janus Weil
Hi all,
after the dust of the heated discussion around this PR has settled a
bit, here is another attempt to implement at least some basic warnings
about compiler-dependent behavior concerning the short-circuiting of
logical expressions.
As a reminder (and recap of the previous discussion), the Fortran
standard unfortunately is a bit sloppy in this area: It allows
compilers to short-circuit the second operand of .AND. / .OR.
operators, but does not require this. As a result, compilers can do
gfortran does short-circuiting (via TRUTH_ANDIF_EXPR/TRUTH_ORIF_EXPR),
ifort does not.
I'm continuing here the least-invasive approach of keeping gfortran's
current behavior, but warning about cases where compilers may produce
different results.
The attached patch is very close to the version I posted previously
(which was already approved by Janne), with the difference that the
warnings are now triggered by -Wextra and not -Wsurprising (which is
included in -Wall), as suggested by Nick Maclaren. I think this is
more reasonable, since not everyone may want to see these warnings.
Note that I don't want to warn about all possible optimizations that
might be allowed by the standard, but only about those that are
actually problematic in practice and result in compiler-dependent
behavior.
The patch regtests cleanly on x86_64-linux-gnu. Ok for trunk?
Cheers,
Janus
PR fortran/85599
* dump-parse-tree (show_attr): Add handling of implicit_pure.
* resolve.c (impure_function_callback): New function.
(resolve_operator): Call it vial gfc_expr_walker.
PR fortran/85599
* gfortran.dg/short_circuiting.f90: New test.

[Janus Weil]

Hi all,

here is another update of the patch. It cures the previously-mentioned
problems with generic interfaces, adds some documentation (as
suggested by Dominique) and slightly enhances the error message
(mentioning the impurity of the function we're warning about).

I tested it on a fairly large code base and found no further false
positives. Also it still regtests cleanly. Ok for trunk?

Cheers,
Janus


2018-07-15 Thomas Koenig <***@gcc.gnu.org>
Janus Weil <***@gcc.gnu.org>

PR fortran/85599
* dump-parse-tree.c (show_attr): Add handling of implicit_pure.
* gfortran.texi: Add chapter on evaluation of logical expressions.
* resolve.c (implicit_pure_function): New function.
(check_pure_function): Use it here.
(impure_function_callback): New function.
(resolve_operator): Call it via gfc_expr_walker.


2018-07-15 Janus Weil <***@gcc.gnu.org>

PR fortran/85599
* gfortran.dg/short_circuiting.f90: New test.

Post by Janus Weil
Just noticed another problematic case: Calls to generic interfaces are
not detected as implicitly pure, see enhanced test case in attachment.
Will look into this problem on the weekend ...
Cheers,
Janus

Post by Janus Weil
Hi all,
here is a minor update of the patch, which cures some problems with
implicitly pure functions in the previous version.
Most implicitly pure functions were actually detected correctly, but
implicitly pure functions that called other implicitly pure functions
were not detected properly, and therefore could trigger a warning.
This is fixed in the current version, which still regtests cleanly
(note that alloc-comp-3.f90 currently fails due to PR 86417). The test
case is also enhanced to include the problematic case.
Ok for trunk?
Cheers,
Janus

Post by Janus Weil
Hi all,
after the dust of the heated discussion around this PR has settled a
bit, here is another attempt to implement at least some basic warnings
about compiler-dependent behavior concerning the short-circuiting of
logical expressions.
As a reminder (and recap of the previous discussion), the Fortran
standard unfortunately is a bit sloppy in this area: It allows
compilers to short-circuit the second operand of .AND. / .OR.
operators, but does not require this. As a result, compilers can do
gfortran does short-circuiting (via TRUTH_ANDIF_EXPR/TRUTH_ORIF_EXPR),
ifort does not.
I'm continuing here the least-invasive approach of keeping gfortran's
current behavior, but warning about cases where compilers may produce
different results.
The attached patch is very close to the version I posted previously
(which was already approved by Janne), with the difference that the
warnings are now triggered by -Wextra and not -Wsurprising (which is
included in -Wall), as suggested by Nick Maclaren. I think this is
more reasonable, since not everyone may want to see these warnings.
Note that I don't want to warn about all possible optimizations that
might be allowed by the standard, but only about those that are
actually problematic in practice and result in compiler-dependent
behavior.
The patch regtests cleanly on x86_64-linux-gnu. Ok for trunk?
Cheers,
Janus
PR fortran/85599
* dump-parse-tree (show_attr): Add handling of implicit_pure.
* resolve.c (impure_function_callback): New function.
(resolve_operator): Call it vial gfc_expr_walker.
PR fortran/85599
* gfortran.dg/short_circuiting.f90: New test.

[Thomas Koenig]

Hi Janus,

Post by Janus Weil
I tested it on a fairly large code base and found no further false
positives. Also it still regtests cleanly. Ok for trunk?

while I still disagree with this on principle, I will not stand
in the way.

However, one point: I think that the warning should be under a separate
warning, which should then be enabled by -Wextra.
-Waggressive-function-elimination, could be reused for this,
or something else

Regards

Thomas

[Janus Weil]

Hi Thomas,

Post by Thomas Koenig

Post by Janus Weil
I tested it on a fairly large code base and found no further false
positives. Also it still regtests cleanly. Ok for trunk?

while I still disagree with this on principle, I will not stand
in the way.

IIUC you disagree in the sense that you would like gfortran to have
more such optimizations (with more corresponding warnings). Is that
correct?

I hope you can at least agree that the warnings I'm adding in the
patch are a) useful and b) sufficient for the current state of
optimizations?

Modifying gfortran's runtime behavior is really a separate question
that we can continue to discuss later, probably with some amount of
controversy. But before we even go there, I would really like to have
warnings for the optimizations we have now ...

Post by Thomas Koenig
However, one point: I think that the warning should be under a separate
warning, which should then be enabled by -Wextra.
-Waggressive-function-elimination, could be reused for this,
or something else

I don't actually see such a flag in the manual.

I do see "-faggressive-function-elimination"
(https://gcc.gnu.org/onlinedocs/gfortran/Code-Gen-Options.html), but
that is about changing runtime behavior, not about throwing warnings.

Do you propose to couple the warning to
-faggressive-function-elimination (and also the short-circuiting
behavior itself)?
Or do you propose to add a flag named -Waggressive-function-elimination?

Cheers,
Janus

[Thomas Koenig]

Post by Janus Weil

Post by Thomas Koenig
However, one point: I think that the warning should be under a separate
warning, which should then be enabled by -Wextra.
-Waggressive-function-elimination, could be reused for this,
or something else

I don't actually see such a flag in the manual.

Ah, sorry, I misremembered the option, it is actually
-Wfunction-elimination.

What I would suggest is to enable -Wfunction-eliminiation with
-Wextra and also use that for your new warning.

(I would also suggest to enable -faggressive-function-elimination
at least for -Ofast, but that is another matter).

Regards

Thomas

[Janus Weil]

Post by Thomas Koenig

Post by Janus Weil

Post by Thomas Koenig
However, one point: I think that the warning should be under a separate
warning, which should then be enabled by -Wextra.
-Waggressive-function-elimination, could be reused for this,
or something else

I don't actually see such a flag in the manual.

Ah, sorry, I misremembered the option, it is actually
-Wfunction-elimination.
What I would suggest is to enable -Wfunction-eliminiation with
-Wextra and also use that for your new warning.

Thanks for the comments. Makes sense. Updated patch attached.

I'll wait two more days to allow for further comments and will commit
this to trunk on Thursday if I hear no further complaints.

Cheers,
Janus

[Janus Weil]

Post by Janus Weil

Post by Thomas Koenig
What I would suggest is to enable -Wfunction-eliminiation with
-Wextra and also use that for your new warning.

Thanks for the comments. Makes sense. Updated patch attached.

Huh, after actually trying -Wfunction-elimination, I'm not so sure any
more if it really makes sense to throw the new warnings in there,
mainly for two reasons:

a) -Wfunction-elimination is slightly different, in that it warns
about removing *any* kind of function, not just impure ones. This
makes it pretty verbose, and I'm not sure why one would need to know
if a pure function call is removed.
b) It gives warnings on places that I don't understand. Simple example:

subroutine sub(r)
implicit none
real, intent(in) :: r
if ((abs(r) > 1e6) .or. (abs(r) < 1e-6)) then
print *, "rrr"
end if
end

Compiling this with "gfortran-8 -O1 -Wfunction-elimination" gives me:

if ((abs(r) > 1e6) .or. (abs(r) < 1e-6)) then
1
Warning: Removing call to function ‘abs’ at (1) [-Wfunction-elimination]


Why can I remove the call to ABS there? If I read the dump correctly,
then the two calls to ABS are optimized into a single one, which is
used for both comparisons via a temporary. Is that what the warning is
trying to tell me? And if yes, why should I care (if the function is
pure)? The middle-end certainly does tons of optimizations that
rearrange various expressions, without warning me about it ...

In other words: Does it make sense to tone down
-Wfunction-elimination, by only warning about impure functions? (We
certainly have the diagnostic capabilites for this.) If not, I'd
rather have a separate flag for the new warnings.
-Wfunction-elimination is just too noisy for my taste in its current
form.

Cheers,
Janus

[Janus Weil]

Post by Janus Weil

Post by Janus Weil

Post by Thomas Koenig
What I would suggest is to enable -Wfunction-eliminiation with
-Wextra and also use that for your new warning.

Thanks for the comments. Makes sense. Updated patch attached.

Huh, after actually trying -Wfunction-elimination, I'm not so sure any
more if it really makes sense to throw the new warnings in there,
a) -Wfunction-elimination is slightly different, in that it warns
about removing *any* kind of function, not just impure ones. This
makes it pretty verbose, and I'm not sure why one would need to know
if a pure function call is removed.
subroutine sub(r)
implicit none
real, intent(in) :: r
if ((abs(r) > 1e6) .or. (abs(r) < 1e-6)) then
print *, "rrr"
end if
end
if ((abs(r) > 1e6) .or. (abs(r) < 1e-6)) then
1
Warning: Removing call to function ‘abs’ at (1) [-Wfunction-elimination]
Why can I remove the call to ABS there? If I read the dump correctly,
then the two calls to ABS are optimized into a single one, which is
used for both comparisons via a temporary. Is that what the warning is
trying to tell me? And if yes, why should I care (if the function is
pure)? The middle-end certainly does tons of optimizations that
rearrange various expressions, without warning me about it ...
In other words: Does it make sense to tone down
-Wfunction-elimination, by only warning about impure functions?

Here is an update of the patch which does that. Regtesting now ...

Cheers,
Janus

[Fritz Reese]

Post by Janus Weil

Post by Janus Weil

Post by Janus Weil

Post by Thomas Koenig
What I would suggest is to enable -Wfunction-eliminiation with
-Wextra and also use that for your new warning.

Thanks for the comments. Makes sense. Updated patch attached.

Huh, after actually trying -Wfunction-elimination, I'm not so sure any
more if it really makes sense to throw the new warnings in there,

[...]

Post by Janus Weil

Post by Janus Weil
In other words: Does it make sense to tone down
-Wfunction-elimination, by only warning about impure functions?

Here is an update of the patch which does that. Regtesting now ...
Cheers,
Janus

Would not this break the testcase function_optimize_5.f90 :

write (unit=line, fmt='(4F7.2)') matmul(a,b) & ! { dg-warning
"Removing call to function 'matmul'" }
& + matmul(a,b)
z = sin(x) + 2.0 + sin(x) ! { dg-warning "Removing call to function 'sin'" }
print *,z
x = ext_func(a) + 23 + ext_func(a)
print *,d,x
z = element(x) + element(x) ! { dg-warning "Removing call to
function 'element'" }
print *,z
i = mypure(x) - mypure(x) ! { dg-warning "Removing call to function
'mypure'" }

Post by Janus Weil
Warn if any calls to functions are eliminated by the optimizations

However, with your patch, it should probably read something like "warn
if any calls to impure functions are eliminated..." Possibly with an
explicit remark indicating that pure functions are not warned.

Additionally:

$ ./contrib/check_GNU_style.sh pr85599_v6.diff

Lines should not exceed 80 characters.
33:+ if (e->expr_type == EXPR_FUNCTION && !gfc_pure_function (e,
&name) && !gfc_implicit_pure_function (e))
36:+ gfc_warning (OPT_Wfunction_elimination, "Removing call to
impure function %qs at %L", name, &(e->where));
38:+ gfc_warning (OPT_Wfunction_elimination, "Removing call to
impure function at %L", &(e->where));
201:+ if (f != last && !gfc_pure_function (f, &name) &&
!gfc_implicit_pure_function (f))

Blocks of 8 spaces should be replaced with tabs.
36:+ gfc_warning (OPT_Wfunction_elimination, "Removing call to
impure function %qs at %L", name, &(e->where));
38:+ gfc_warning (OPT_Wfunction_elimination, "Removing call to
impure function at %L", &(e->where));
230:+ with impure function as second operand. */

Dot, space, space, new sentence.
79:+expression, if they do not contribute to the final result. For logical
82:+result of the expression can be established without them. However, since not

Have you considered using levels for the flag, such that the original
behavior of -Wfunction-elimination would be retained with e.g.
-Wfunction-elimination=2 whereas one could use
-Wfunction-elimination=1 (or similar) to omit warnings about impure
functions?

- Fritz

[Janus Weil]

Post by Fritz Reese

Post by Janus Weil

Post by Janus Weil
In other words: Does it make sense to tone down
-Wfunction-elimination, by only warning about impure functions?

Here is an update of the patch which does that. Regtesting now ...

My regtest says so as well ;)

Post by Fritz Reese

Post by Janus Weil
Warn if any calls to functions are eliminated by the optimizations

However, with your patch, it should probably read something like "warn
if any calls to impure functions are eliminated..." Possibly with an
explicit remark indicating that pure functions are not warned.

Yes.

However, the test case above seems to indicate that the
function-elimination optimization is not applied to impure functions
anyway (which is good IMHO). It that is true, then my modifications
practically disable the old -Wfunction-elimination warnings completely
:/

Post by Fritz Reese
Have you considered using levels for the flag, such that the original
behavior of -Wfunction-elimination would be retained with e.g.
-Wfunction-elimination=2 whereas one could use
-Wfunction-elimination=1 (or similar) to omit warnings about impure
functions?

One could do that, but IMHO it would be overkill. I don't see much
sense in warning for the elimination of pure functions. But maybe I'm
missing something?

Cheers,
Janus

[Thomas Koenig]

Post by Janus Weil

Post by Fritz Reese

Post by Janus Weil

Post by Janus Weil
In other words: Does it make sense to tone down
-Wfunction-elimination, by only warning about impure functions?

Here is an update of the patch which does that. Regtesting now ...

My regtest says so as well ;)

Post by Fritz Reese

Post by Janus Weil
Warn if any calls to functions are eliminated by the optimizations

However, with your patch, it should probably read something like "warn
if any calls to impure functions are eliminated..." Possibly with an
explicit remark indicating that pure functions are not warned.

Yes.
However, the test case above seems to indicate that the
function-elimination optimization is not applied to impure functions
anyway (which is good IMHO).

If you specify -faggressive-function-elimination, it is also
done for impure (and non implicitly-pure) functions.

Problem is that, in all probability, nobody uses this option at the
moment.

Post by Janus Weil
It that is true, then my modifications
practically disable the old -Wfunction-elimination warnings completely
:/

I do not think it would be a problem not to warn for removing
calls to pure or implicitly pure fuctions. The test cases can
easily be modified not to emit this warning, as you did.

As the author of the original test cases, I may be able to
say so with a certain amount of credibility.

The actual elimination is checked for by counting the
function names in the *.original dump file, which is done.

[Janus Weil]

Post by Thomas Koenig

Post by Janus Weil
However, the test case above seems to indicate that the
function-elimination optimization is not applied to impure functions
anyway (which is good IMHO).

If you specify -faggressive-function-elimination, it is also
done for impure (and non implicitly-pure) functions.

Ah, ok.

Post by Thomas Koenig
Problem is that, in all probability, nobody uses this option at the
moment.

That's probably true, but it should get a little better once we move
it into -Wextra.

Post by Thomas Koenig

Post by Janus Weil
It that is true, then my modifications
practically disable the old -Wfunction-elimination warnings completely
:/

I do not think it would be a problem not to warn for removing
calls to pure or implicitly pure fuctions. The test cases can
easily be modified not to emit this warning, as you did.
As the author of the original test cases, I may be able to
say so with a certain amount of credibility.

Good, so let's do this. Attached is another update of my patch, which
incorporates all of Fritz' comments and should regtest cleanly now.

In function_optimize_5.f90 I have removed the warnings for pure
functions and instead added -faggressive-function-elimination and the
corresponding warnings for impure functions, which were apparently not
covered by the testsuite before.

I do hope that things have converged by now and that this will be the
last incarnation of the patch. If there is no more feedback in the
next 24 hours, I'll commit this tomorrow.

Cheers,
Janus

[Fritz Reese]

[...]

Post by Janus Weil
I do hope that things have converged by now and that this will be the
last incarnation of the patch. If there is no more feedback in the
next 24 hours, I'll commit this tomorrow.
Cheers,
Janus

I hate to be pedantic but it is still worth fixing the style discrepancies:

$ ./contrib/check_GNU_style.sh pr85599_v7.diff

Post by Janus Weil
Lines should not exceed 80 characters.
209:+ if (f != last && !gfc_pure_function (f, &name) && !gfc_implicit_pure_function (f))
Blocks of 8 spaces should be replaced with tabs.
37:+ gfc_warning (OPT_Wfunction_elimination,
41:+ gfc_warning (OPT_Wfunction_elimination,
238:+ with impure function as second operand. */
Dot, space, space, new sentence.
84:+expression, if they do not contribute to the final result. For logical
87:+result of the expression can be established without them. However, since not

My only other comment is I am not sure why you make
pure_function()/gfc_implicit_pure_function() public... but I have no
real problem with it. Just means rebuilding all of f951 instead of one
object. :-(
Otherwise if the patch does what it appears to do and passes tests
then it seems fine to me.

Fritz

[Janus Weil]

Post by Fritz Reese
[...]

Post by Janus Weil
I do hope that things have converged by now and that this will be the
last incarnation of the patch. If there is no more feedback in the
next 24 hours, I'll commit this tomorrow.

Oh, sure. Such things are non-optional in GCC, I was just a bit sloppy
with this. Thanks for catching! Should be fixed in the attached
update.

Post by Fritz Reese
My only other comment is I am not sure why you make
pure_function()/gfc_implicit_pure_function() public... but I have no
real problem with it. Just means rebuilding all of f951 instead of one
object. :-(

Well, originally they were only used in resolve.c, but now I need them
also in frontend-passes.c, therefore they have to be public.

Post by Fritz Reese
Otherwise if the patch does what it appears to do and passes tests
then it seems fine to me.

Thanks for the review!

Cheers,
Janus

[Janus Weil]

I have now finally committed this as r262860. Thanks everyone for
helping to bring this to a constructive end (and especially to Thomas
for the useful input and for putting up with me).

Cheers,
Janus

PS: The next step here would be to tackle PR57160 and avoid the
short-circuiting with -O0 (I might actually look into that soon). Once
that is done we could even talk about further optimizations (as
previously suggested by Thomas), provided they are only done with
-O... flags and warned about with appropriate -W... flags.

Post by Janus Weil

Post by Fritz Reese
[...]

Post by Janus Weil
I do hope that things have converged by now and that this will be the
last incarnation of the patch. If there is no more feedback in the
next 24 hours, I'll commit this tomorrow.

Oh, sure. Such things are non-optional in GCC, I was just a bit sloppy
with this. Thanks for catching! Should be fixed in the attached
update.

Post by Fritz Reese
My only other comment is I am not sure why you make
pure_function()/gfc_implicit_pure_function() public... but I have no
real problem with it. Just means rebuilding all of f951 instead of one
object. :-(

Well, originally they were only used in resolve.c, but now I need them
also in frontend-passes.c, therefore they have to be public.

Post by Fritz Reese
Otherwise if the patch does what it appears to do and passes tests
then it seems fine to me.

Thanks for the review!
Cheers,
Janus