Re: [Patch, Fortran] PR 57160: short-circuit IF only with -ffrontend-optimize

Re: [Patch, Fortran] PR 57160: short-circuit IF only with -ffrontend-optimize

[Dominique d'Humières]

Hi Janus,

> gfortran currently does short-circuiting, and after my patch for PR
> 85599 warns about cases where this might remove an impure function
> call (which potentially can change results).
>
> Now, this PR (57160) is about code which relies on the
> short-circuiting behavior. Since short-circuiting is not guaranteed by
> the standard, such code is invalid. Generating a warning or an error
> at compile-time is a bit harder here, though, since there are multiple
> variations of such a situation, e.g.:
> * ASSOCIATED(p) .AND. p%T
> * ALLOCATED(a) .AND. a%T
> * i<ubound(x) .AND. x(i)
> * …
>

Aren’t you confusing portability with validity?
The above codes are indeed invalid without short-circuit evaluation,
 but I did not find anything in the standard saying such codes are
invalid with short-circuit evaluation.

> The suggestion in the PR was to do short-circuiting only with
> optimization flags, but inhibit it with -O0, so that the faulty code
> will run into a segfault (or runtime error) at least when
> optimizations are disabled, and the problem can be identified.

This PR has nothing to do with optimization and I think
 it is a very bad idea to (ab)use any optimization option.

Please leave the default behavior (and test) as they are now.
If you want non short-circuit evaluation, introduce an option for it.

Note that the warning introduce for pr85599 should be disabled
for non short-circuit evaluations.

TIA

Dominique

Re: [Patch, Fortran] PR 57160: short-circuit IF only with -ffrontend-optimize

[Janus Weil]

2018-07-24 11:12 GMT+02:00 Dominique d'Humières <dominiq@lps.ens.fr>:
> Hi Janus,
>
>> gfortran currently does short-circuiting, and after my patch for PR
>> 85599 warns about cases where this might remove an impure function
>> call (which potentially can change results).
>>
>> Now, this PR (57160) is about code which relies on the
>> short-circuiting behavior. Since short-circuiting is not guaranteed by
>> the standard, such code is invalid. Generating a warning or an error
>> at compile-time is a bit harder here, though, since there are multiple
>> variations of such a situation, e.g.:
>> * ASSOCIATED(p) .AND. p%T
>> * ALLOCATED(a) .AND. a%T
>> * i<ubound(x) .AND. x(i)
>> * …
>>
>
> Aren’t you confusing portability with validity?

I don' think so.


> The above codes are indeed invalid without short-circuit evaluation,
>  but I did not find anything in the standard saying such codes are
> invalid with short-circuit evaluation.

If they're not valid without short-circuiting, then they're not valid
at all, because the Fortran standard does not require a processor to
do short-circuiting.


>> The suggestion in the PR was to do short-circuiting only with
>> optimization flags, but inhibit it with -O0, so that the faulty code
>> will run into a segfault (or runtime error) at least when
>> optimizations are disabled, and the problem can be identified.
>
> This PR has nothing to do with optimization and I think
>  it is a very bad idea to (ab)use any optimization option.

The PR is definitely related to optimization, because the fact that
the invalid test case works at all is only due to an optional
optimization called short-circuiting.

Don't get me wrong. I think it would be great if the Fortran standard
would *require* short-circuiting for and/or operators (or had separate
operators which do that). That would allow to write code like
"ASSOCIATED(p) .AND. p%T" (which would actually be useful).
Unfortunately that's not the case, therefore such code is plain
invalid.


> Please leave the default behavior (and test) as they are now.

You seriously prefer to keep an invalid test case in the testsuite?


> If you want non short-circuit evaluation, introduce an option for it.

Your argument could easily be reversed: If you want short-circuiting,
go introduce an option for it.

I'm sure we'll not get anywhere this way, and I do think that Joost's
suggestion to avoid short-circuiting with -O0 (and possibly -Og) is
very reasonable: People who want maximum performance still get that
with -O3, and at the same time they can still check their codes for
errors with -O0.

What is your problem?!?


> Note that the warning introduce for pr85599 should be disabled
> for non short-circuit evaluations.

That's a valid point.

Cheers,
Janus

Re: [Patch, Fortran] PR 57160: short-circuit IF only with -ffrontend-optimize

[Janne Blomqvist]

On Tue, Jul 24, 2018 at 4:46 PM, Janus Weil <janus@gcc.gnu.org> wrote:

> 2018-07-24 11:12 GMT+02:00 Dominique d'Humières <dominiq@lps.ens.fr>:
> > If you want non short-circuit evaluation, introduce an option for it.
>
> Your argument could easily be reversed: If you want short-circuiting,
> go introduce an option for it.
>
> I'm sure we'll not get anywhere this way, and I do think that Joost's
> suggestion to avoid short-circuiting with -O0 (and possibly -Og) is
> very reasonable: People who want maximum performance still get that
> with -O3, and at the same time they can still check their codes for
> errors with -O0.
>

I agree with Dominique, that this would be better as a separate option.
Optimization bugs that pop up at different optimization levels are hard
enough for users to figure out, without the frontend generating different
code to begin with depending on the optimization level. Also, with a
separate option it would be easy to check how it affects performance at
different optimization levels.

What about making it a -fcheck=short-circuit-logicals (or however you want
to spell it?) option, that also would be enabled with -fcheck=all?

-- 
Janne Blomqvist

Re: [Patch, Fortran] PR 57160: short-circuit IF only with -ffrontend-optimize

[Janus Weil]

2018-07-24 17:41 GMT+02:00 Janne Blomqvist <blomqvist.janne@gmail.com>:
> Optimization bugs that pop up at different optimization levels are hard
> enough for users to figure out

Right, and they're impossible to detect if there is no way to disable
the optimization, which is what this PR is about.


> without the frontend generating different
> code to begin with depending on the optimization level.

In the end it doesn't make much of a difference whether the
optimizations are done in the front or the middle end. The user knows
nothing about this, and he doesn't need to.

The problematic point here is that short-circuiting is an optimization
that is enabled already at -O0.


> Also, with a
> separate option it would be easy to check how it affects performance at
> different optimization levels.

For the case at hand, the short-circuiting is an absolutely valid
optimization. There is no reason why you wouldn't wanna do it (with -O
flags).


> What about making it a -fcheck=short-circuit-logicals (or however you want
> to spell it?) option, that also would be enabled with -fcheck=all?

What would such a flag even do? The actually invalid operation in the
test case is a null-pointer access, which could be caught by
-fcheck=pointer if we disable the optimization that removes it (i.e.
short-circuiting).

However, it also seems like -fcheck=pointer could use some
enhancement, since it does not even seem to catch simple cases like
this:

integer, pointer :: p => null()
print *,p
end

Cheers,
Janus

[Patch, Fortran] PR 57160: short-circuit IF only with -ffrontend-optimize

[Janus Weil]

[-- Attachment #1: Type: text/plain, Size: 1919 bytes --]

Hi all,

here is a follow-up patch to my recent commit for PR 85599, also
dealing with the short-circuiting of logical operators. In the course
of the extensive discussion of that PR it became clear that the
Fortran standard allows the short-circuiting of .AND. and .OR.
operators, but does not mandate it.

gfortran currently does short-circuiting, and after my patch for PR
85599 warns about cases where this might remove an impure function
call (which potentially can change results).

Now, this PR (57160) is about code which relies on the
short-circuiting behavior. Since short-circuiting is not guaranteed by
the standard, such code is invalid. Generating a warning or an error
at compile-time is a bit harder here, though, since there are multiple
variations of such a situation, e.g.:
* ASSOCIATED(p) .AND. p%T
* ALLOCATED(a) .AND. a%T
* i<ubound(x) .AND. x(i)
* ...

The suggestion in the PR was to do short-circuiting only with
optimization flags, but inhibit it with -O0, so that the faulty code
will run into a segfault (or runtime error) at least when
optimizations are disabled, and the problem can be identified.

I find this suggestion very reasonable. It makes it possible to detect
invalid code at -O0, while keeping good performance for valid code at
-O{1,2,3}. Also it is technically very simple to implement, and it
immediately identified a faulty test case that has lived in the
testsuite for eleven years without being detected.

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

Cheers,
Janus


2018-07-20  Janus Weil  <janus@gcc.gnu.org>

    PR fortran/57160
    * trans-expr.c (gfc_conv_expr_op): Use short-circuiting operators only
    with -ffrontend-optimize. Without that flag, make sure that both
    operands are evaluated.


2018-07-20  Janus Weil  <janus@gcc.gnu.org>

    PR fortran/57160
    * gfortran.dg/actual_pointer_function_1.f90: Fix invalid test case.

[-- Attachment #2: pr57160.diff --]
[-- Type: text/x-patch, Size: 1277 bytes --]

Index: gcc/fortran/trans-expr.c
===================================================================
--- gcc/fortran/trans-expr.c	(revision 262908)
+++ gcc/fortran/trans-expr.c	(working copy)
@@ -3348,12 +3348,12 @@ gfc_conv_expr_op (gfc_se * se, gfc_expr * expr)
       return;
 
     case INTRINSIC_AND:
-      code = TRUTH_ANDIF_EXPR;
+      code = flag_frontend_optimize ? TRUTH_ANDIF_EXPR : TRUTH_AND_EXPR;
       lop = 1;
       break;
 
     case INTRINSIC_OR:
-      code = TRUTH_ORIF_EXPR;
+      code = flag_frontend_optimize ? TRUTH_ORIF_EXPR : TRUTH_OR_EXPR;
       lop = 1;
       break;
 
Index: gcc/testsuite/gfortran.dg/actual_pointer_function_1.f90
===================================================================
--- gcc/testsuite/gfortran.dg/actual_pointer_function_1.f90	(revision 262908)
+++ gcc/testsuite/gfortran.dg/actual_pointer_function_1.f90	(working copy)
@@ -17,7 +17,11 @@ CONTAINS
 
   logical function cp_logger_log(logger)
     TYPE(cp_logger_type), POINTER ::logger
-    cp_logger_log = associated (logger) .and. (logger%a .eq. 42)
+    if (associated (logger)) then
+      cp_logger_log = (logger%a .eq. 42)
+    else
+      cp_logger_log = .false.
+    end if
   END function
 
   FUNCTION cp_get_default_logger(v) RESULT(res)