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

[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)

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

[Adam Hirst]

[-- Attachment #1.1: Type: text/plain, Size: 2450 bytes --]

On 20/07/18 22:37, Janus Weil wrote:
> 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.
One thing I'm not seeing in the original discussion was whether or not
this should also count for -Og, which certainly in my experience is
(paired with -g) a common debug setting.

I would err towards -Og here being paired with -O0, but I could see it
being argued both ways - either way, I thought it might at least be
worth making explicit?

> 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: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Janus Weil]

Hi Adam,

2018-07-23 9:40 GMT+02:00 Adam Hirst <adam@aphirst.karoo.co.uk>:
> On 20/07/18 22:37, Janus Weil wrote:
>>
>> 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.
>
> One thing I'm not seeing in the original discussion was whether or not
> this should also count for -Og

good point!


> which certainly in my experience is
> (paired with -g) a common debug setting.

Agreed. I actually use it myself.


> I would err towards -Og here being paired with -O0, but I could see it
> being argued both ways - either way, I thought it might at least be
> worth making explicit?

Well, yes, the current documentation for -ffrontend-optimize is not
horribly explicit, but it does say: " Enabled by default by any -O
option." Technically that includes -Og, I guess.

Phenomenologically, it seems that -Og indeed behaves like -O{1,2,3} in
this respect. If one wanted to change that, one would probably do
this:

Index: gcc/fortran/options.c
===================================================================
--- gcc/fortran/options.c    (revision 262908)
+++ gcc/fortran/options.c    (working copy)
@@ -417,7 +417,7 @@
      specified it directly.  */

   if (flag_frontend_optimize == -1)
-    flag_frontend_optimize = optimize;
+    flag_frontend_optimize = optimize && !optimize_debug;

   /* Same for front end loop interchange.  */


I tend to agree with you that this might be a good idea, but I also
don't have a strong opinion here. (Alternatively one could leave
-ffrontend-optimize as is, and just couple the short-circuiting
behavior to "flag_frontend_optimize && !optimize_debug", but that
seems less attractive to me.) Maybe others can comment?

Cheers,
Janus

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

[Fritz Reese]

On Mon, Jul 23, 2018 at 1:11 PM Janus Weil <janus@gcc.gnu.org> wrote:
>
> Hi Adam,
>
> 2018-07-23 9:40 GMT+02:00 Adam Hirst <adam@aphirst.karoo.co.uk>:
[...]
> > One thing I'm not seeing in the original discussion was whether or not
> > this should also count for -Og
>
> good point!
>
>
> > which certainly in my experience is
> > (paired with -g) a common debug setting.
>
> Agreed. I actually use it myself.
>
>
> > I would err towards -Og here being paired with -O0, but I could see it
> > being argued both ways - either way, I thought it might at least be
> > worth making explicit?
>
> Well, yes, the current documentation for -ffrontend-optimize is not
> horribly explicit, but it does say: " Enabled by default by any -O
> option." Technically that includes -Og, I guess.
>
> Phenomenologically, it seems that -Og indeed behaves like -O{1,2,3} in
> this respect. If one wanted to change that, one would probably do
> this:
>
> Index: gcc/fortran/options.c
> ===================================================================
> --- gcc/fortran/options.c    (revision 262908)
> +++ gcc/fortran/options.c    (working copy)
> @@ -417,7 +417,7 @@
>       specified it directly.  */
>
>    if (flag_frontend_optimize == -1)
> -    flag_frontend_optimize = optimize;
> +    flag_frontend_optimize = optimize && !optimize_debug;
>
>    /* Same for front end loop interchange.  */
>
>
> I tend to agree with you that this might be a good idea, but I also
> don't have a strong opinion here. (Alternatively one could leave
> -ffrontend-optimize as is, and just couple the short-circuiting
> behavior to "flag_frontend_optimize && !optimize_debug", but that
> seems less attractive to me.) Maybe others can comment?
>

IMO it makes sense to omit frontend optimizations with -Og since one
probably expects -g/-Og to provide the most faithful reproduction of
the code (least optimized). I would be OK including this in the patch.

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

I would recommend updating invoke.texi to include a comment regarding
the effect of -ffrontend-optimize on short-circuiting. If you include
the above regarding -Og, you should also clarify "Enabled by default
by any -O option" (e.g. "Enabled by any -O option except -O0 and
-Og").

Normally I like to see testcase(s) enforcing the new behavior as well,
unless there is a good reason not to. (That way any future changes to
short-circuiting or -ffrontend-optimize should at least snag on the
testcase and cause special consideration.)

Otherwise looks OK.

Fritz

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

[Janus Weil]

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

2018-07-23 23:05 GMT+02:00 Fritz Reese <fritzoreese@gmail.com>:
> On Mon, Jul 23, 2018 at 1:11 PM Janus Weil <janus@gcc.gnu.org> wrote:
>> 2018-07-23 9:40 GMT+02:00 Adam Hirst <adam@aphirst.karoo.co.uk>:
>> > I would err towards -Og here being paired with -O0, but I could see it
>> > being argued both ways - either way, I thought it might at least be
>> > worth making explicit?
>>
>> Well, yes, the current documentation for -ffrontend-optimize is not
>> horribly explicit, but it does say: " Enabled by default by any -O
>> option." Technically that includes -Og, I guess.
>>
>> Phenomenologically, it seems that -Og indeed behaves like -O{1,2,3} in
>> this respect. If one wanted to change that, one would probably do
>> this:
>>
>> Index: gcc/fortran/options.c
>> ===================================================================
>> --- gcc/fortran/options.c    (revision 262908)
>> +++ gcc/fortran/options.c    (working copy)
>> @@ -417,7 +417,7 @@
>>       specified it directly.  */
>>
>>    if (flag_frontend_optimize == -1)
>> -    flag_frontend_optimize = optimize;
>> +    flag_frontend_optimize = optimize && !optimize_debug;
>>
>>    /* Same for front end loop interchange.  */
>>
>>
>> I tend to agree with you that this might be a good idea, but I also
>> don't have a strong opinion here. (Alternatively one could leave
>> -ffrontend-optimize as is, and just couple the short-circuiting
>> behavior to "flag_frontend_optimize && !optimize_debug", but that
>> seems less attractive to me.) Maybe others can comment?
>>
>
> IMO it makes sense to omit frontend optimizations with -Og since one
> probably expects -g/-Og to provide the most faithful reproduction of
> the code (least optimized). I would be OK including this in the patch.

Good, since we all seem to agree on that, I'm including it in the
patch (new version in the attachment).


>> The attached patch regtests cleanly on x86_64-linux-gnu. Ok for trunk?
>
> I would recommend updating invoke.texi to include a comment regarding
> the effect of -ffrontend-optimize on short-circuiting. If you include
> the above regarding -Og, you should also clarify "Enabled by default
> by any -O option" (e.g. "Enabled by any -O option except -O0 and
> -Og").

Done.


> Normally I like to see testcase(s) enforcing the new behavior as well,
> unless there is a good reason not to. (That way any future changes to
> short-circuiting or -ffrontend-optimize should at least snag on the
> testcase and cause special consideration.)

I somehow thought that the change to actual_pointer_function_1 would
be enough, but of course this does not verify the general behavior wrt
to short-circuiting. I'm attaching two test cases in this direction
now.


> Otherwise looks OK.

Thanks for the review. The new patch also disables the warnings from
PR85599 if -ffrontend-optimize is not given, as noted by Dominique.

The attached is what I'd like to commit (and is regtesting now), but
I'll wait for further comments of course.

Cheers,
Janus

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

Index: gcc/fortran/invoke.texi
===================================================================
--- gcc/fortran/invoke.texi	(revision 262950)
+++ gcc/fortran/invoke.texi	(working copy)
@@ -1793,13 +1793,17 @@ if @option{-ffrontend-optimize} is in effect.
 @opindex @code{frontend-optimize}
 @cindex Front-end optimization
 This option performs front-end optimization, based on manipulating
-parts the Fortran parse tree.  Enabled by default by any @option{-O}
-option.  Optimizations enabled by this option include inlining calls
-to @code{MATMUL}, elimination of identical function calls within
-expressions, removing unnecessary calls to @code{TRIM} in comparisons
-and assignments and replacing @code{TRIM(a)} with
-@code{a(1:LEN_TRIM(a))}.  It can be deselected by specifying
-@option{-fno-frontend-optimize}.
+parts the Fortran parse tree.  Enabled by default by any @option{-O} option
+except @option{-O0} and @option{-Og}.  Optimizations enabled by this option
+include:
+@itemize @bullet
+@item inlining calls to @code{MATMUL},
+@item elimination of identical function calls within expressions,
+@item removing unnecessary calls to @code{TRIM} in comparisons and assignments,
+@item replacing @code{TRIM(a)} with @code{a(1:LEN_TRIM(a))} and
+@item short-circuiting of logical operators (@code{.AND.} and @code{.OR.}).
+@end itemize
+It can be deselected by specifying @option{-fno-frontend-optimize}.
 
 @item -ffrontend-loop-interchange
 @opindex @code{frontend-loop-interchange}
Index: gcc/fortran/options.c
===================================================================
--- gcc/fortran/options.c	(revision 262950)
+++ gcc/fortran/options.c	(working copy)
@@ -417,7 +417,7 @@ gfc_post_options (const char **pfilename)
      specified it directly.  */
 
   if (flag_frontend_optimize == -1)
-    flag_frontend_optimize = optimize;
+    flag_frontend_optimize = optimize && !optimize_debug;
 
   /* Same for front end loop interchange.  */
 
Index: gcc/fortran/resolve.c
===================================================================
--- gcc/fortran/resolve.c	(revision 262950)
+++ gcc/fortran/resolve.c	(working copy)
@@ -3982,7 +3982,8 @@ resolve_operator (gfc_expr *e)
 	  else if (op2->ts.kind < e->ts.kind)
 	    gfc_convert_type (op2, &e->ts, 2);
 
-	  if (e->value.op.op == INTRINSIC_AND || e->value.op.op == INTRINSIC_OR)
+	  if (flag_frontend_optimize &&
+	    (e->value.op.op == INTRINSIC_AND || e->value.op.op == INTRINSIC_OR))
 	    {
 	      /* Warn about short-circuiting
 	         with impure function as second operand.  */
Index: gcc/fortran/trans-expr.c
===================================================================
--- gcc/fortran/trans-expr.c	(revision 262950)
+++ 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 262950)
+++ 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)

[-- Attachment #3: short_circuiting_2.f90 --]
[-- Type: text/x-fortran, Size: 475 bytes --]

! { dg-do run }
! { dg-options "-O0" }
!
! PR 57160: short-circuit IF only with -ffrontend-optimize
!
! this checks that short-circuiting is not done with -O0
!
! Contributed by Janus Weil <janus@gcc.gnu.org>

program short_circuit

   integer, save :: i = 0
   logical :: flag

   flag = .false.
   flag = check() .and. flag
   flag = flag .and. check()

   if (i /= 2) stop 1

contains

   logical function check()
      i = i + 1
      check = .true.
   end function

end

[-- Attachment #4: short_circuiting_3.f90 --]
[-- Type: text/x-fortran, Size: 471 bytes --]

! { dg-do run }
! { dg-options "-O3" }
!
! PR 57160: short-circuit IF only with -ffrontend-optimize
!
! this checks that short-circuiting is done with -O3
!
! Contributed by Janus Weil <janus@gcc.gnu.org>

program short_circuit

   integer, save :: i = 0
   logical :: flag

   flag = .false.
   flag = check() .and. flag
   flag = flag .and. check()

   if (i /= 1) stop 1

contains

   logical function check()
      i = i + 1
      check = .true.
   end function

end

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

[Janus Weil]

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

2018-07-24 20:46 GMT+02:00 Janus Weil <janus@gcc.gnu.org>:
> 2018-07-23 23:05 GMT+02:00 Fritz Reese <fritzoreese@gmail.com>:
>> Otherwise looks OK.
>
> Thanks for the review. The new patch also disables the warnings from
> PR85599 if -ffrontend-optimize is not given, as noted by Dominique.
>
> The attached is what I'd like to commit (and is regtesting now), but
> I'll wait for further comments of course.

Regtesting showed one failure in inline_matmul_23, which is fixed by
adding -ffrontend-optimize. Updated patch attached.

Cheers,
Janus

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

Index: gcc/fortran/invoke.texi
===================================================================
--- gcc/fortran/invoke.texi	(revision 262950)
+++ gcc/fortran/invoke.texi	(working copy)
@@ -1793,13 +1793,17 @@ if @option{-ffrontend-optimize} is in effect.
 @opindex @code{frontend-optimize}
 @cindex Front-end optimization
 This option performs front-end optimization, based on manipulating
-parts the Fortran parse tree.  Enabled by default by any @option{-O}
-option.  Optimizations enabled by this option include inlining calls
-to @code{MATMUL}, elimination of identical function calls within
-expressions, removing unnecessary calls to @code{TRIM} in comparisons
-and assignments and replacing @code{TRIM(a)} with
-@code{a(1:LEN_TRIM(a))}.  It can be deselected by specifying
-@option{-fno-frontend-optimize}.
+parts the Fortran parse tree.  Enabled by default by any @option{-O} option
+except @option{-O0} and @option{-Og}.  Optimizations enabled by this option
+include:
+@itemize @bullet
+@item inlining calls to @code{MATMUL},
+@item elimination of identical function calls within expressions,
+@item removing unnecessary calls to @code{TRIM} in comparisons and assignments,
+@item replacing @code{TRIM(a)} with @code{a(1:LEN_TRIM(a))} and
+@item short-circuiting of logical operators (@code{.AND.} and @code{.OR.}).
+@end itemize
+It can be deselected by specifying @option{-fno-frontend-optimize}.
 
 @item -ffrontend-loop-interchange
 @opindex @code{frontend-loop-interchange}
Index: gcc/fortran/options.c
===================================================================
--- gcc/fortran/options.c	(revision 262950)
+++ gcc/fortran/options.c	(working copy)
@@ -417,7 +417,7 @@ gfc_post_options (const char **pfilename)
      specified it directly.  */
 
   if (flag_frontend_optimize == -1)
-    flag_frontend_optimize = optimize;
+    flag_frontend_optimize = optimize && !optimize_debug;
 
   /* Same for front end loop interchange.  */
 
Index: gcc/fortran/resolve.c
===================================================================
--- gcc/fortran/resolve.c	(revision 262950)
+++ gcc/fortran/resolve.c	(working copy)
@@ -3982,7 +3982,8 @@ resolve_operator (gfc_expr *e)
 	  else if (op2->ts.kind < e->ts.kind)
 	    gfc_convert_type (op2, &e->ts, 2);
 
-	  if (e->value.op.op == INTRINSIC_AND || e->value.op.op == INTRINSIC_OR)
+	  if (flag_frontend_optimize &&
+	    (e->value.op.op == INTRINSIC_AND || e->value.op.op == INTRINSIC_OR))
 	    {
 	      /* Warn about short-circuiting
 	         with impure function as second operand.  */
Index: gcc/fortran/trans-expr.c
===================================================================
--- gcc/fortran/trans-expr.c	(revision 262950)
+++ 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 262950)
+++ 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)
Index: gcc/testsuite/gfortran.dg/inline_matmul_23.f90
===================================================================
--- gcc/testsuite/gfortran.dg/inline_matmul_23.f90	(revision 262950)
+++ gcc/testsuite/gfortran.dg/inline_matmul_23.f90	(working copy)
@@ -1,5 +1,5 @@
 ! { dg-do compile }
-! { dg-options "-Og -fcheck=bounds -fdump-tree-optimized" }
+! { dg-options "-Og -ffrontend-optimize -fcheck=bounds -fdump-tree-optimized" }
 ! Check that bounds checking is done only before the matrix
 ! multiplication.
 

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

[Thomas Koenig]

Am 24.07.2018 um 20:46 schrieb Janus Weil:
> Good, since we all seem to agree on that, I'm including it in the
> patch (new version in the attachment).

Sorry for chiming in so late, but I don't (and Dominique doesn't
either).

If you want to enforce either short-circuit evaluation or
forced evaluation, please use a dedicated option.

I don't think it is a good idea to have

if (allocated(x) .and. any(x>0))

crash on -O0 and not crash on -O.

-O should not change the semantics of a program in
such a way.

Regards

	Thomas

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

[Janus Weil]

2018-07-24 21:49 GMT+02:00 Thomas Koenig <tkoenig@netcologne.de>:
> Am 24.07.2018 um 20:46 schrieb Janus Weil:
>>
>> Good, since we all seem to agree on that, I'm including it in the
>> patch (new version in the attachment).
>
> Sorry for chiming in so late, but I don't (and Dominique doesn't
> either).

Dominique has not commented on the question whether -Og should imply
-ffrontend-optimization, therefore I don't know his opinion on that
matter.


> If you want to enforce either short-circuit evaluation or
> forced evaluation, please use a dedicated option.

I don't want to enforce any of that. gfortran is enforcing
short-circuiting right now (with any option), and I want to change
that.


> I don't think it is a good idea to have
>
> if (allocated(x) .and. any(x>0))
>
> crash on -O0 and not crash on -O.

I actually do. The code is invalid. Crashing it is good, because it
draws the user's attention to that fact.

You can find lots of invalid programs that change their behavior with
different optimization flags.


> -O should not change the semantics of a program in
> such a way.

You should thank the authors of the Fortran standard for not fixing
the semantics of and/or operands.

Note that -O will not change the results of valid code (except if
impure functions are involved, and those trigger a warning since
recently).

Cheers,
Janus

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

[Janus Weil]

2018-07-24 22:14 GMT+02:00 Janus Weil <janus@gcc.gnu.org>:
> 2018-07-24 21:49 GMT+02:00 Thomas Koenig <tkoenig@netcologne.de>:
>> If you want to enforce either short-circuit evaluation or
>> forced evaluation, please use a dedicated option.
>
> I don't want to enforce any of that. gfortran is enforcing
> short-circuiting right now (with any option), and I want to change
> that.

To expand on that point: I really don't see why a dedicated option is
needed. Short-circuiting of logical expressions is certainly an
"optimization" (which is allowed but not required by the Fortran
standard), and it is done in the Fortran front end, thus enabling it
with -ffrontend-optimize makes perfect sense to me. Actually I don't
really care which one of the plenty optimization flags of GCC turns it
on, but I do think that, as all other optimizations, it should
certainly be disabled at -O0.

In fact the the idea put forward by Joost in PR57160 (which I'm
implementing here) coincides with the policy proposed by Jakub in
https://gcc.gnu.org/ml/fortran/2018-06/msg00215.html:

> So for -O0 at least always use
> TRUTH_{AND,OR}_EXPR, so that people can actually make sure that their
> programs are valid Fortran and can also step into those functions when
> debugging.  For -O1 and higher perhaps use temporarily the *IF_EXPR, or
> better, as I said in another mail, let's add an attribute that will optimize
> all the calls that can be optimized, not just one special case.


> 2018-07-24 21:49 GMT+02:00 Thomas Koenig <tkoenig@netcologne.de>:
>> I don't think it is a good idea to have
>>
>> if (allocated(x) .and. any(x>0))
>>
>> crash on -O0 and not crash on -O.
>
> I actually do. The code is invalid. Crashing it is good, because it
> draws the user's attention to that fact.

What would be even better than a hard crash (segfault) would be a
proper runtime error that gives more information about the nature of
the problem. -fcheck=all may already catch some of the situations
mentioned in the beginning of this thread, but it clearly needs
improvement/extension, which could be tackled as a follow-up.

If there is further constructive criticism regarding v3 of the patch,
as posted at https://gcc.gnu.org/ml/fortran/2018-07/msg00103.html,
I'll be happy to deal with that. If not, I'd like to commit it by the
weekend.

Cheers,
Janus

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

[Nicolas Koenig]

HI Janus,

> To expand on that point: I really don't see why a dedicated option is
> needed. Short-circuiting of logical expressions is certainly an
> "optimization" (which is allowed but not required by the Fortran
> standard),

If you do mean optimizations, why the quotes?

And if you think this is an optimization, why did you object to
my patch which actually used the leeway given by the
Fortran standard?

And if you don't think this is an optimization, coupling this to
an -O option is not the right way.

Regards

	Thomas

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

[Thomas Koenig]

Nicolas Koenig did not actually write...

> HI Janus,

Oops - sorry for that. I had selected the wrong mail account for
this, that was actually me writing, not Nicolas.

Regards

	Thomas

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

[Janus Weil]

Hi Thomas,

>> To expand on that point: I really don't see why a dedicated option is
>> needed. Short-circuiting of logical expressions is certainly an
>> "optimization" (which is allowed but not required by the Fortran
>> standard),
>
> If you do mean optimizations, why the quotes?
>
> And if you think this is an optimization, why did you object to
> my patch which actually used the leeway given by the
> Fortran standard?
>
> And if you don't think this is an optimization, coupling this to
> an -O option is not the right way.

first off: I clearly think that this is an optimization (I used the
quotes merely as an emphasis).

But in any case it's not about what I think, but about what the
standard says. And the standard says things like:

"It is not necessary for a processor to evaluate all of the operands
of an expression, or to evaluate entirely each
operand, if the value of the expression can be determined otherwise."

To me that sounds like omitting the evaluation of an operand (aka
"short-circuiting") is something that a compiler is allowed to do, but
not required. The main motivation for omitting the evaluation is
typically performance. So, according to my interpretation of the
standard, short-circuiting is what you would call an "optimization".

Note that this in contrast to the C/C++ case, where short-circuiting
is part of the semantics of the && and || operators, thus it's
required (not optional) and therefore not an optimization.


One of the reasons why I objected to your earlier patch was precisely
the missing possibility to disable the optimization. If this
possibility is provided, I'd actually be willing to discuss further
optimizations (provided they can guarantee for all valid code that the
results are not modified) ...

Cheers,
Janus

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

[Janus Weil]

Am Fr., 10. Aug. 2018 um 02:38 Uhr schrieb Jerry DeLisle
<jvdelisle@charter.net>:
>
> On 08/09/2018 02:14 PM, Janus Weil wrote:
> > 2018-08-09 3:04 GMT+02:00 Jerry DeLisle <jvdelisle@charter.net>:
> >> On 08/08/2018 01:48 PM, Janus Weil wrote:
> >>>
> >>> 2018-08-08 20:20 GMT+02:00 Janus Weil <janus@gcc.gnu.org>:
> >>>>
> >>>> Hi all,
> >>>>
> >>>> since there seems to be significant support for this after all, I just
> >>>> rebased the last patch version and pushed it to github (I sincerely
> >>>> hope gcc will officially make the switch to git very soon):
> >>>>
> >>>>
> >>>> https://github.com/janusw/gcc/commit/35aae880ff2b3d4a9f6a9c821376fb510202961c
> >>>>
> >>>> This is still the most reasonable approach I can see. And I'd still be
> >>>> grateful for (technical) comments on the patch.
> >>>>
> >>>> What's the procedure in GCC for dealing with such controversial
> >>>> patches? The procedure for simple cases is: The patch author asks for
> >>>> review, an authorized reviewer/maintainer approves it or asks for
> >>>> improvements. What happens if that fails? Is there any rule on how
> >>>> decisions are to be made in the non-trivial case?
> >>>
> >>>
> >>> Re-reading the thread, I noticed that I haven't actually gotten a
> >>> clear "yes" or "no" from anyone with review privileges for the Fortran
> >>> front-end.
> >>>
> >>> So, I'll just continue with the usual procedure: The patch above
> >>> regtests cleanly on x86_64-linux-gnu and provides a clear improvement
> >>> regarding the standard-conformance of the GNU Fortan compiler. Ok to
> >>> commit to trunk?
> >>>
> >>
> >> Yes, OK for trunk.
> >
> > Thanks, Jerry. I'll wait until Saturday to allow for further comments ...
> >
>
> I dont see any need to wait, this topic has been beat to death, Of
> course, your choice.

Alright then. Committed to trunk as r263471.

Cheers,
Janus

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

[Jerry DeLisle]

On 08/09/2018 02:14 PM, Janus Weil wrote:
> 2018-08-09 3:04 GMT+02:00 Jerry DeLisle <jvdelisle@charter.net>:
>> On 08/08/2018 01:48 PM, Janus Weil wrote:
>>>
>>> 2018-08-08 20:20 GMT+02:00 Janus Weil <janus@gcc.gnu.org>:
>>>>
>>>> Hi all,
>>>>
>>>> since there seems to be significant support for this after all, I just
>>>> rebased the last patch version and pushed it to github (I sincerely
>>>> hope gcc will officially make the switch to git very soon):
>>>>
>>>>
>>>> https://github.com/janusw/gcc/commit/35aae880ff2b3d4a9f6a9c821376fb510202961c
>>>>
>>>> This is still the most reasonable approach I can see. And I'd still be
>>>> grateful for (technical) comments on the patch.
>>>>
>>>> What's the procedure in GCC for dealing with such controversial
>>>> patches? The procedure for simple cases is: The patch author asks for
>>>> review, an authorized reviewer/maintainer approves it or asks for
>>>> improvements. What happens if that fails? Is there any rule on how
>>>> decisions are to be made in the non-trivial case?
>>>
>>>
>>> Re-reading the thread, I noticed that I haven't actually gotten a
>>> clear "yes" or "no" from anyone with review privileges for the Fortran
>>> front-end.
>>>
>>> So, I'll just continue with the usual procedure: The patch above
>>> regtests cleanly on x86_64-linux-gnu and provides a clear improvement
>>> regarding the standard-conformance of the GNU Fortan compiler. Ok to
>>> commit to trunk?
>>>
>>
>> Yes, OK for trunk.
> 
> Thanks, Jerry. I'll wait until Saturday to allow for further comments ...
> 
> Cheers,
> Janus
> 

I dont see any need to wait, this topic has been beat to death, Of 
course, your choice.

Jerry

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

[Janus Weil]

2018-08-09 3:04 GMT+02:00 Jerry DeLisle <jvdelisle@charter.net>:
> On 08/08/2018 01:48 PM, Janus Weil wrote:
>>
>> 2018-08-08 20:20 GMT+02:00 Janus Weil <janus@gcc.gnu.org>:
>>>
>>> Hi all,
>>>
>>> since there seems to be significant support for this after all, I just
>>> rebased the last patch version and pushed it to github (I sincerely
>>> hope gcc will officially make the switch to git very soon):
>>>
>>>
>>> https://github.com/janusw/gcc/commit/35aae880ff2b3d4a9f6a9c821376fb510202961c
>>>
>>> This is still the most reasonable approach I can see. And I'd still be
>>> grateful for (technical) comments on the patch.
>>>
>>> What's the procedure in GCC for dealing with such controversial
>>> patches? The procedure for simple cases is: The patch author asks for
>>> review, an authorized reviewer/maintainer approves it or asks for
>>> improvements. What happens if that fails? Is there any rule on how
>>> decisions are to be made in the non-trivial case?
>>
>>
>> Re-reading the thread, I noticed that I haven't actually gotten a
>> clear "yes" or "no" from anyone with review privileges for the Fortran
>> front-end.
>>
>> So, I'll just continue with the usual procedure: The patch above
>> regtests cleanly on x86_64-linux-gnu and provides a clear improvement
>> regarding the standard-conformance of the GNU Fortan compiler. Ok to
>> commit to trunk?
>>
>
> Yes, OK for trunk.

Thanks, Jerry. I'll wait until Saturday to allow for further comments ...

Cheers,
Janus

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

[Jerry DeLisle]

On 08/08/2018 01:48 PM, Janus Weil wrote:
> 2018-08-08 20:20 GMT+02:00 Janus Weil <janus@gcc.gnu.org>:
>> Hi all,
>>
>> since there seems to be significant support for this after all, I just
>> rebased the last patch version and pushed it to github (I sincerely
>> hope gcc will officially make the switch to git very soon):
>>
>> https://github.com/janusw/gcc/commit/35aae880ff2b3d4a9f6a9c821376fb510202961c
>>
>> This is still the most reasonable approach I can see. And I'd still be
>> grateful for (technical) comments on the patch.
>>
>> What's the procedure in GCC for dealing with such controversial
>> patches? The procedure for simple cases is: The patch author asks for
>> review, an authorized reviewer/maintainer approves it or asks for
>> improvements. What happens if that fails? Is there any rule on how
>> decisions are to be made in the non-trivial case?
> 
> Re-reading the thread, I noticed that I haven't actually gotten a
> clear "yes" or "no" from anyone with review privileges for the Fortran
> front-end.
> 
> So, I'll just continue with the usual procedure: The patch above
> regtests cleanly on x86_64-linux-gnu and provides a clear improvement
> regarding the standard-conformance of the GNU Fortan compiler. Ok to
> commit to trunk?
> 
> Cheers,
> Janus
> 

Yes, OK for trunk.

Thanks for the effort.

Jerry

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

[Janus Weil]

2018-08-08 20:20 GMT+02:00 Janus Weil <janus@gcc.gnu.org>:
> Hi all,
>
> since there seems to be significant support for this after all, I just
> rebased the last patch version and pushed it to github (I sincerely
> hope gcc will officially make the switch to git very soon):
>
> https://github.com/janusw/gcc/commit/35aae880ff2b3d4a9f6a9c821376fb510202961c
>
> This is still the most reasonable approach I can see. And I'd still be
> grateful for (technical) comments on the patch.
>
> What's the procedure in GCC for dealing with such controversial
> patches? The procedure for simple cases is: The patch author asks for
> review, an authorized reviewer/maintainer approves it or asks for
> improvements. What happens if that fails? Is there any rule on how
> decisions are to be made in the non-trivial case?

Re-reading the thread, I noticed that I haven't actually gotten a
clear "yes" or "no" from anyone with review privileges for the Fortran
front-end.

So, I'll just continue with the usual procedure: The patch above
regtests cleanly on x86_64-linux-gnu and provides a clear improvement
regarding the standard-conformance of the GNU Fortan compiler. Ok to
commit to trunk?

Cheers,
Janus

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

[Janus Weil]

2018-08-08 10:35 GMT+02:00 Manfred Schwarb <manfred99@gmx.ch>:
> PS: left-to-right short-circuiting is very handy, indeed. But I think it would be
>     wrong to nail down some behavior in gfortran which is not mandated by the standards.
>     I rather would prefer to aggressively implement some explicitly non-standard .ANDELSE.
>     (or similar) syntax and not to wait for the committees. FORTRAN has a long tradition
>     of compilers driving progress and standards to follow, after all...

Absolutely agree. I just opened the following PR:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86893

Cheers,
Janus

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

[Janus Weil]

Hi all,

since there seems to be significant support for this after all, I just
rebased the last patch version and pushed it to github (I sincerely
hope gcc will officially make the switch to git very soon):

https://github.com/janusw/gcc/commit/35aae880ff2b3d4a9f6a9c821376fb510202961c

This is still the most reasonable approach I can see. And I'd still be
grateful for (technical) comments on the patch.

What's the procedure in GCC for dealing with such controversial
patches? The procedure for simple cases is: The patch author asks for
review, an authorized reviewer/maintainer approves it or asks for
improvements. What happens if that fails? Is there any rule on how
decisions are to be made in the non-trivial case?

Cheers,
Janus



2018-08-08 13:29 GMT+02:00 Arjen Markus <arjen.markus895@gmail.com>:
> FWIW, I second Manfred's opinion. I have no strong preference to such
> short-circuiting or against it, but I feel that putting the programmer
> in control is the better option.
>
> Regards,
>
> Arjen
>
> 2018-08-08 13:22 GMT+02:00 Janus Weil <janus@gcc.gnu.org>:
>> Oh, look, a reasonable person! Where have you been hiding all this
>> time? And is there more of your kind in that place?  ;D
>>
>>
>>
>> 2018-08-08 10:35 GMT+02:00 Manfred Schwarb <manfred99@gmx.ch>:
>>> Hi Janus, hi Dominique,
>>>
>>> I did not raise my voice so far, as it is not so terribly relevant I guess,
>>> nevertheless...
>>>
>>> Am 07.08.2018 um 19:14 schrieb Janus Weil:
>>>> 2018-08-07 12:11 GMT+02:00 Dominique d'Humières <dominiq@lps.ens.fr>:
>>>>>
>>>>>> It's so easy to scream "bullshit", and so hard to find a good
>>>>>> solution. Still, we need one …
>>>>>
>>>>> Sorry if I have not been clear enough. I see only two
>>>>> sensible solutions:
>>>>>
>>>>> (1) The statu quo, closing the PR as WONTFIX
>>>>
>>>> How is that even an option?!? gfortran accepts invalid code. What's
>>>> wrong with fixing that?
>>>>
>>>> If your aim is to keep "the status quo" (read: a compiler with
>>>> gazillions of bugs), why don't we just shut down bugzilla altogether?
>>>>
>>>>
>>>>> (2) Introduce a new option, for instance
>>>>> -fshort-circuit (default) for the short-circuit evaluation
>>>>> -fno-short-circuit to force the evaluation of both expressions.
>>>>
>>>> For starters, no one will find a bug in his code if it requires
>>>> -fsome-obscure-option-that-nobody-knows.
>>>>
>>>> Also I could ask the innocent question why -fshort-circuit should be
>>>> the default, but I guess I won't bother to go there.
>>>>
>>>
>>> I think both of you are right to some degree. I think it is a (good) custom
>>> in GCC to couple major optimizations with its own option flag. And to activate
>>> it for some -Ox levels.
>>> Because IMHO it is just this, a bog ordinary optimization.
>>> And usually, optimizations which obfuscate the control flow and degrade debug-ability
>>> are deactivated at level -O0.
>>> Why can't this be handled just like any other optimization?
>>>
>>> Cheers,
>>> Manfred
>>>
>>>
>>> PS: left-to-right short-circuiting is very handy, indeed. But I think it would be
>>>     wrong to nail down some behavior in gfortran which is not mandated by the standards.
>>>     I rather would prefer to aggressively implement some explicitly non-standard .ANDELSE.
>>>     (or similar) syntax and not to wait for the committees. FORTRAN has a long tradition
>>>     of compilers driving progress and standards to follow, after all...
>>>
>>>
>>>> I still find it deeply unsatisfying that gfortran is incompatible with
>>>> other compilers on a very fundamental level. And I find it very
>>>> disturbing that no one even cares. Anyway, I'm kinda through with this
>>>> whole issue. I guess I'll just stop caring as well.
>>>>
>>>> Goodbye ...
>>>>
>>>

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

[Arjen Markus]

FWIW, I second Manfred's opinion. I have no strong preference to such
short-circuiting or against it, but I feel that putting the programmer
in control is the better option.

Regards,

Arjen

2018-08-08 13:22 GMT+02:00 Janus Weil <janus@gcc.gnu.org>:
> Oh, look, a reasonable person! Where have you been hiding all this
> time? And is there more of your kind in that place?  ;D
>
>
>
> 2018-08-08 10:35 GMT+02:00 Manfred Schwarb <manfred99@gmx.ch>:
>> Hi Janus, hi Dominique,
>>
>> I did not raise my voice so far, as it is not so terribly relevant I guess,
>> nevertheless...
>>
>> Am 07.08.2018 um 19:14 schrieb Janus Weil:
>>> 2018-08-07 12:11 GMT+02:00 Dominique d'Humières <dominiq@lps.ens.fr>:
>>>>
>>>>> It's so easy to scream "bullshit", and so hard to find a good
>>>>> solution. Still, we need one …
>>>>
>>>> Sorry if I have not been clear enough. I see only two
>>>> sensible solutions:
>>>>
>>>> (1) The statu quo, closing the PR as WONTFIX
>>>
>>> How is that even an option?!? gfortran accepts invalid code. What's
>>> wrong with fixing that?
>>>
>>> If your aim is to keep "the status quo" (read: a compiler with
>>> gazillions of bugs), why don't we just shut down bugzilla altogether?
>>>
>>>
>>>> (2) Introduce a new option, for instance
>>>> -fshort-circuit (default) for the short-circuit evaluation
>>>> -fno-short-circuit to force the evaluation of both expressions.
>>>
>>> For starters, no one will find a bug in his code if it requires
>>> -fsome-obscure-option-that-nobody-knows.
>>>
>>> Also I could ask the innocent question why -fshort-circuit should be
>>> the default, but I guess I won't bother to go there.
>>>
>>
>> I think both of you are right to some degree. I think it is a (good) custom
>> in GCC to couple major optimizations with its own option flag. And to activate
>> it for some -Ox levels.
>> Because IMHO it is just this, a bog ordinary optimization.
>> And usually, optimizations which obfuscate the control flow and degrade debug-ability
>> are deactivated at level -O0.
>> Why can't this be handled just like any other optimization?
>>
>> Cheers,
>> Manfred
>>
>>
>> PS: left-to-right short-circuiting is very handy, indeed. But I think it would be
>>     wrong to nail down some behavior in gfortran which is not mandated by the standards.
>>     I rather would prefer to aggressively implement some explicitly non-standard .ANDELSE.
>>     (or similar) syntax and not to wait for the committees. FORTRAN has a long tradition
>>     of compilers driving progress and standards to follow, after all...
>>
>>
>>> I still find it deeply unsatisfying that gfortran is incompatible with
>>> other compilers on a very fundamental level. And I find it very
>>> disturbing that no one even cares. Anyway, I'm kinda through with this
>>> whole issue. I guess I'll just stop caring as well.
>>>
>>> Goodbye ...
>>>
>>

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

[Janus Weil]

Oh, look, a reasonable person! Where have you been hiding all this
time? And is there more of your kind in that place?  ;D



2018-08-08 10:35 GMT+02:00 Manfred Schwarb <manfred99@gmx.ch>:
> Hi Janus, hi Dominique,
>
> I did not raise my voice so far, as it is not so terribly relevant I guess,
> nevertheless...
>
> Am 07.08.2018 um 19:14 schrieb Janus Weil:
>> 2018-08-07 12:11 GMT+02:00 Dominique d'Humières <dominiq@lps.ens.fr>:
>>>
>>>> It's so easy to scream "bullshit", and so hard to find a good
>>>> solution. Still, we need one …
>>>
>>> Sorry if I have not been clear enough. I see only two
>>> sensible solutions:
>>>
>>> (1) The statu quo, closing the PR as WONTFIX
>>
>> How is that even an option?!? gfortran accepts invalid code. What's
>> wrong with fixing that?
>>
>> If your aim is to keep "the status quo" (read: a compiler with
>> gazillions of bugs), why don't we just shut down bugzilla altogether?
>>
>>
>>> (2) Introduce a new option, for instance
>>> -fshort-circuit (default) for the short-circuit evaluation
>>> -fno-short-circuit to force the evaluation of both expressions.
>>
>> For starters, no one will find a bug in his code if it requires
>> -fsome-obscure-option-that-nobody-knows.
>>
>> Also I could ask the innocent question why -fshort-circuit should be
>> the default, but I guess I won't bother to go there.
>>
>
> I think both of you are right to some degree. I think it is a (good) custom
> in GCC to couple major optimizations with its own option flag. And to activate
> it for some -Ox levels.
> Because IMHO it is just this, a bog ordinary optimization.
> And usually, optimizations which obfuscate the control flow and degrade debug-ability
> are deactivated at level -O0.
> Why can't this be handled just like any other optimization?
>
> Cheers,
> Manfred
>
>
> PS: left-to-right short-circuiting is very handy, indeed. But I think it would be
>     wrong to nail down some behavior in gfortran which is not mandated by the standards.
>     I rather would prefer to aggressively implement some explicitly non-standard .ANDELSE.
>     (or similar) syntax and not to wait for the committees. FORTRAN has a long tradition
>     of compilers driving progress and standards to follow, after all...
>
>
>> I still find it deeply unsatisfying that gfortran is incompatible with
>> other compilers on a very fundamental level. And I find it very
>> disturbing that no one even cares. Anyway, I'm kinda through with this
>> whole issue. I guess I'll just stop caring as well.
>>
>> Goodbye ...
>>
>

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

[Manfred Schwarb]

Hi Janus, hi Dominique,

I did not raise my voice so far, as it is not so terribly relevant I guess,
nevertheless...
 
Am 07.08.2018 um 19:14 schrieb Janus Weil:
> 2018-08-07 12:11 GMT+02:00 Dominique d'Humières <dominiq@lps.ens.fr>:
>>
>>> It's so easy to scream "bullshit", and so hard to find a good
>>> solution. Still, we need one …
>>
>> Sorry if I have not been clear enough. I see only two
>> sensible solutions:
>>
>> (1) The statu quo, closing the PR as WONTFIX
> 
> How is that even an option?!? gfortran accepts invalid code. What's
> wrong with fixing that?
> 
> If your aim is to keep "the status quo" (read: a compiler with
> gazillions of bugs), why don't we just shut down bugzilla altogether?
> 
> 
>> (2) Introduce a new option, for instance
>> -fshort-circuit (default) for the short-circuit evaluation
>> -fno-short-circuit to force the evaluation of both expressions.
> 
> For starters, no one will find a bug in his code if it requires
> -fsome-obscure-option-that-nobody-knows.
> 
> Also I could ask the innocent question why -fshort-circuit should be
> the default, but I guess I won't bother to go there.
> 

I think both of you are right to some degree. I think it is a (good) custom
in GCC to couple major optimizations with its own option flag. And to activate
it for some -Ox levels.
Because IMHO it is just this, a bog ordinary optimization.
And usually, optimizations which obfuscate the control flow and degrade debug-ability
are deactivated at level -O0.
Why can't this be handled just like any other optimization?

Cheers,
Manfred


PS: left-to-right short-circuiting is very handy, indeed. But I think it would be 
    wrong to nail down some behavior in gfortran which is not mandated by the standards.
    I rather would prefer to aggressively implement some explicitly non-standard .ANDELSE.
    (or similar) syntax and not to wait for the committees. FORTRAN has a long tradition
    of compilers driving progress and standards to follow, after all...


> I still find it deeply unsatisfying that gfortran is incompatible with
> other compilers on a very fundamental level. And I find it very
> disturbing that no one even cares. Anyway, I'm kinda through with this
> whole issue. I guess I'll just stop caring as well.
> 
> Goodbye ...
> 

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

[Janus Weil]

Hi Thomas,

>> How is that even an option?!? gfortran accepts invalid code. What's
>> wrong with fixing that?
>
> Just a general remark, I am on a business trip in the US at the moment.
>
> There are errors which the compiler is required to diagnose, when a constraint is violated. And there are errors which the compiler is not required to catch, those marked with „shall“ in the standard.
>
> Issuing an error or a warning for those is always a balancing act because if the possibility of false positives.

sure, I'm aware of that general fact.

However, it's certainly a good idea to diagnose everything you can, no
matter if the standard requires you to do that or not. Having
undetectably invalid code does not increase the appeal of the compiler
(or the language in general). Just like having compiler-dependent
code. All of that is a major turn-off (at least to me).

I'll also note that my approach to fixing PR 57160 had zero false positives.

Cheers,
Janus

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

[William Clodius]

The confusion of false negative is also a concern.

> On Aug 7, 2018, at 4:21 PM, Thomas König <tk@tkoenig.net> wrote:
> 
> Hi Janus,
> 
>> How is that even an option?!? gfortran accepts invalid code. What's
>> wrong with fixing that?
> 
> Just a general remark, I am on a business trip in the US at the moment.
> 
> There are errors which the compiler is required to diagnose, when a constraint is violated. And there are errors which the compiler is not required to catch, those marked with „shall“ in the standard.
> 
> Issuing an error or a warning for those is always a balancing act because if the possibility of false positives.
> 
> Regards, Thomas

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

[Thomas König]

Hi Janus,

> How is that even an option?!? gfortran accepts invalid code. What's
> wrong with fixing that?

Just a general remark, I am on a business trip in the US at the moment.

There are errors which the compiler is required to diagnose, when a constraint is violated. And there are errors which the compiler is not required to catch, those marked with „shall“ in the standard.

Issuing an error or a warning for those is always a balancing act because if the possibility of false positives.

Regards, Thomas

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

[Janus Weil]

2018-08-07 12:11 GMT+02:00 Dominique d'Humières <dominiq@lps.ens.fr>:
>
>> It's so easy to scream "bullshit", and so hard to find a good
>> solution. Still, we need one …
>
> Sorry if I have not been clear enough. I see only two
> sensible solutions:
>
> (1) The statu quo, closing the PR as WONTFIX

How is that even an option?!? gfortran accepts invalid code. What's
wrong with fixing that?

If your aim is to keep "the status quo" (read: a compiler with
gazillions of bugs), why don't we just shut down bugzilla altogether?


> (2) Introduce a new option, for instance
> -fshort-circuit (default) for the short-circuit evaluation
> -fno-short-circuit to force the evaluation of both expressions.

For starters, no one will find a bug in his code if it requires
-fsome-obscure-option-that-nobody-knows.

Also I could ask the innocent question why -fshort-circuit should be
the default, but I guess I won't bother to go there.

I still find it deeply unsatisfying that gfortran is incompatible with
other compilers on a very fundamental level. And I find it very
disturbing that no one even cares. Anyway, I'm kinda through with this
whole issue. I guess I'll just stop caring as well.

Goodbye ...

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

[Dominique d'Humières]

> It's so easy to scream "bullshit", and so hard to find a good
> solution. Still, we need one …

Sorry if I have not been clear enough. I see only two
sensible solutions:

(1) The statu quo, closing the PR as WONTFIX
(2) Introduce a new option, for instance
-fshort-circuit (default) for the short-circuit evaluation
-fno-short-circuit to force the evaluation of both expressions.

Dominique

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

[Janus Weil]

2018-08-01 22:46 GMT+02:00 Janus Weil <janus@gcc.gnu.org>:
> 2018-07-28 10:03 GMT+02:00 Janus Weil <janus@gcc.gnu.org>:
>> 2018-07-27 18:42 GMT+02:00 Dominique d'Humières <dominiq@lps.ens.fr>:
>>>> If there is further constructive criticism regarding v3 of the patch,
>>>> as posted at https://gcc.gnu.org/ml/fortran/2018-07/msg00103.html,
>>>> I'll be happy to deal with that. If not, I'd like to commit it by the
>>>> weekend.
>>>
>>> This patch breaks backward compatibility
>>
>> Not for valid Fortran code. The patch only affects code that is not
>> guaranteed to work by the Fortran standard, like "if (associated(a)
>> .and. a>0)".
>>
>> "Breaking" such code is precisely the purpose of the patch. The idea
>> is to discourage users from writing such code, because it is not valid
>> Fortran code (and therefore not portable). Are you trying to argue
>> that such code should work with gfortran?
>>
>>
>>> Hence I am definitively against this patch.
>>
>> If you are against it, do you have *any* other solution for the problem at hand?
>
> Ping!
>
> We have a problem here which deserves a solution. So, Dominique, if
> you don't like my solution, how do you propose to solve PR 57160?

ping**2

It's so easy to scream "bullshit", and so hard to find a good
solution. Still, we need one ...

I suggest the naysayers get active. I'm out.

Cheers,
Janus

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

[Janus Weil]

2018-07-28 10:03 GMT+02:00 Janus Weil <janus@gcc.gnu.org>:
> 2018-07-27 18:42 GMT+02:00 Dominique d'Humières <dominiq@lps.ens.fr>:
>>> If there is further constructive criticism regarding v3 of the patch,
>>> as posted at https://gcc.gnu.org/ml/fortran/2018-07/msg00103.html,
>>> I'll be happy to deal with that. If not, I'd like to commit it by the
>>> weekend.
>>
>> This patch breaks backward compatibility
>
> Not for valid Fortran code. The patch only affects code that is not
> guaranteed to work by the Fortran standard, like "if (associated(a)
> .and. a>0)".
>
> "Breaking" such code is precisely the purpose of the patch. The idea
> is to discourage users from writing such code, because it is not valid
> Fortran code (and therefore not portable). Are you trying to argue
> that such code should work with gfortran?
>
>
>> Hence I am definitively against this patch.
>
> If you are against it, do you have *any* other solution for the problem at hand?

Ping!

We have a problem here which deserves a solution. So, Dominique, if
you don't like my solution, how do you propose to solve PR 57160?

Thanks,
Janus

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

[Janus Weil]

2018-07-27 18:42 GMT+02:00 Dominique d'Humières <dominiq@lps.ens.fr>:
>> If there is further constructive criticism regarding v3 of the patch,
>> as posted at https://gcc.gnu.org/ml/fortran/2018-07/msg00103.html,
>> I'll be happy to deal with that. If not, I'd like to commit it by the
>> weekend.
>
> This patch breaks backward compatibility

Not for valid Fortran code. The patch only affects code that is not
guaranteed to work by the Fortran standard, like "if (associated(a)
.and. a>0)".

"Breaking" such code is precisely the purpose of the patch. The idea
is to discourage users from writing such code, because it is not valid
Fortran code (and therefore not portable). Are you trying to argue
that such code should work with gfortran?


> Hence I am definitively against this patch.

If you are against it, do you have *any* other solution for the problem at hand?


> Now I have wasted more than enough of my time on this nonsense and I won’t post anything else on this thread.

Say what ...? You know, your behavior is pissing me off like hell, dude.

What you're saying here is that you disapprove of the patch, but
you're not willing to discuss the reasons. That is not constructive at
all. I'm trying to make a contribution to improving gfortran, and you
are just brute-force blocking that effort due some very emotional
personal opinion, and with zero objective arguments. I don't think
that's how things should work, and it greatly reduces my motivation to
make any further contributions here.

In fact it's me who's wasting his time, not you. You are just blaring
out three-sentence replies with words like "nonsense" that don't
contribute anything to this discussion except bad vibrations. That's
not helping anyone, and it's certainly not helping to improve
gfortran.

Cheers,
Janus

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

[Dominique d'Humières]

> If there is further constructive criticism regarding v3 of the patch,
> as posted at https://gcc.gnu.org/ml/fortran/2018-07/msg00103.html,
> I'll be happy to deal with that. If not, I'd like to commit it by the
> weekend.

This patch breaks backward compatibility and make the situation more confusing that it is presently.

Hence I am definitively against this patch.

Now I have wasted more than enough of my time on this nonsense and I won’t post anything else on this thread.

Dominique

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

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 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

[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