[Bug fortran/85599] Prevent short-circuiting of logical expressions for non-pure functions

[sgk at troutmask dot apl.washington.edu]

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

--- Comment #33 from Steve Kargl <sgk at troutmask dot apl.washington.edu> ---
On Fri, May 18, 2018 at 06:23:41PM +0000, janus at gcc dot gnu.org wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85599
> 
> --- Comment #32 from janus at gcc dot gnu.org ---
> (In reply to Steve Kargl from comment #31)
> > > This PR is not about reordering, but about short-circuiting.
> > 
> > AFAICT this PR is about a function with side-effects,
> > and re-ordering can have an potentially undesirable
> > result.
> 
> Yah, I guess you can dream up all kinds of possible issues with non-pure
> functions, but that is really beyond the scope of this PR. I feel like I have
> already opened up a barrel large enough, and have gotten sufficient flak for
> that. I'm really not interested in taking this discussion any further. If you
> are, please open a new PR for that.
> 

I don't think you're getting flak.  I think you're
getting discussion, where both Thomas and I disagree
with you on whether this is a bug or not.  This has
nothing to do with a pure or impure function.  This
has to do with special casing a binop to explicitly
require that both arguments must be evaluated, because
one of the operands may have a side-effect.  The Fortran
standard explicitly allows the current behavior.
Thomas is willing to compromise to at least issue a
warning that in '.false. .and. check()' check() 
may not be evaluated.   I've suggested a new option
-fno-short-circuit and a review of all binops to
force evaluation of both operands. 

If I were to give you flak, I would have suggested
that you contact the other Fortran vendors with
bug reports for a missed optimization.  It could 
take a very long time to evaluate check().  Why
bother with  the evaluation when the result for
'.false. .and. check()' is known regardless of the
result for check()?  Yes, it is a rhetorical 
question as I know your answer that check() may
have side-effects.  

And, if I were really to give you flak, I would
suggest that a patch is louder than words.

 svn diff gcc/fortran/trans-expr.c 
Index: gcc/fortran/trans-expr.c
===================================================================
--- gcc/fortran/trans-expr.c    (revision 260235)
+++ gcc/fortran/trans-expr.c    (working copy)
@@ -3446,6 +3446,12 @@ gfc_conv_expr_op (gfc_se * se, gfc_expr * expr)

   if (lop)
     {
+      if (expr->value.op.op == INTRINSIC_AND
+         || expr->value.op.op == INTRINSIC_OR)
+       {
+         lse.expr = gfc_evaluate_now (lse.expr, &se->pre);
+         rse.expr = gfc_evaluate_now (rse.expr, &se->pre);
+       }
       /* The result of logical ops is always logical_type_node.  */
       tmp = fold_build2_loc (input_location, code, logical_type_node,
                             lse.expr, rse.expr);


% gfcx -o z a.f90 && ./z
 check           1
 check           2

I completely disagree with this patch without -fno-short-circuit.

You're welcomed.