[patch, fortran] Handling of .and. and .or. expressions

classic Classic list List threaded Threaded
45 messages Options
123
Reply | Threaded
Open this post in threaded view
|

[patch, fortran] Handling of .and. and .or. expressions

Thomas Koenig-6
Hello world,

the attached patch introduces the following changes:

If a logical .and. or .or. expression contains a reference to a function
which is impure and which also does not behave like a pure function
(i.e. does not have the implicit_pure attribute set), it emits a
warning with -Wsurprising that the function might not be evaluated.
(-Wsurprising is enabled by -Wall).

It special cases the idiom  if (associated(m) .and. m%t) which
people appear to use.

And, if there is an expression like   func() .and. flag , it
reverses the test as an optimization. The middle end should be
capable of doing this, but apparently it doesn't, so the front
end might as well do this.

What it does not do is one part of PR 57160, i.e. warn against
if (a /= 0 .and. 1/a > 5) which people who are used to C might
also like to write.

There is already quite some discussion in the PRs, especially 85599,
where not all people were of the same opinion. Let us see where the
discussion here leads us.

Regression-tested (which found one bug in the testsuite).

OK for trunk?

Regards

        Thomas

2018-06-11  Thomas Koenig  <[hidden email]>

         PR fortran/57160
         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. Special-case
         if (associated(m) .and. m%t).  If an .and. or .or. expression
         has a function or a non-function, exchange the operands.

2018-06-11  Thomas Koenig  <[hidden email]>

         PR fortran/57160
         PR fortran/85599
         * gfortran.dg/logical_evaluation_1.f90: New test.
         * gfortran.dg/alloc_comp_default_init_2.f90: Fix code which
         implicitly depends on short-circuiting.

p4.diff (4K) Download Attachment
logical_evaluation_1.f90 (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Handling of .and. and .or. expressions

Toon Moene-3
On 06/11/2018 09:22 PM, Thomas Koenig wrote:

> What it does not do is one part of PR 57160, i.e. warn against
> if (a /= 0 .and. 1/a > 5) which people who are used to C might
> also like to write

Even die-hard Fortran programmers make these kinds of mistakes. Digital
Equipment Fortran found the following error (simplified for display
purposes) for us, by crashing the executable with a Segmentation Fault:

       SUBROUTINE AAP(A, N, I)
C Let's try this with I greater than N:
       DIMENSION A(N)
       IF (I .LE. N .AND. A(I) .GT. 0.0) PRINT*, A(I)
       END

in 1992. It had been part of our production code by that time for years ...

--
Toon Moene - e-mail: [hidden email] - phone: +31 346 214290
Saturnushof 14, 3738 XG  Maartensdijk, The Netherlands
At home: http://moene.org/~toon/; weather: http://moene.org/~hirlam/
Progress of GNU Fortran: http://gcc.gnu.org/wiki/GFortran#news
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Handling of .and. and .or. expressions

Dominique d'Humières-2
In reply to this post by Thomas Koenig-6
Hi Thomas,

As I said in one of the PRs, I don’t like the warnings triggering with -Wall.

Also I don’t understand what is special with ASSOCIATED, as shown by the following code

MODULE M1
 TYPE T1
   LOGICAL :: T=.FALSE.
 END TYPE T1
CONTAINS
 SUBROUTINE S1(m)
   TYPE(T1), allocatable :: m
   IF (ALLOCATED(m) .AND. m%T) THEN
    WRITE(6,*) "X"
   ENDIF
 END SUBROUTINE
END MODULE

USE M1
 TYPE(T1), allocatable :: m
 CALL S1(m)
 allocate(m)
 CALL S1(m)
 m%t = .true.
 CALL S1(m)
 deallocate(m)
END

In addition I don’t understand the associated warning: in short-circuit evaluation ASSOCIATED(m) (or ALLOCATED(m)) does protect m%T to be accessed.

Cheers,

Dominique

Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Handling of .and. and .or. expressions

Janus Weil-3
In reply to this post by Thomas Koenig-6
Hi Thomas,


>the attached patch introduces the following changes:

thanks a lot for working on this!


>If a logical .and. or .or. expression contains a reference to a
>function
>which is impure and which also does not behave like a pure function
>(i.e. does not have the implicit_pure attribute set), it emits a
>warning with -Wsurprising that the function might not be evaluated.
>(-Wsurprising is enabled by -Wall).

I think you are warning about too many cases here. Warning about an impure function as the *second* operand of the logical operators should be enough, I think. AFAICS there is no danger of the first operand not being evaluated, right?


>It special cases the idiom  if (associated(m) .and. m%t) which
>people appear to use.

I don't fully understand why you do this, but in any case it should not be necessary if you warn about the second operand only.


>And, if there is an expression like   func() .and. flag , it
>reverses the test as an optimization.

I really don't like this part. It actually introduces more problems of the sort that we're trying to warn about ...

Cheers,
Janus



>2018-06-11  Thomas Koenig  <[hidden email]>
>
>         PR fortran/57160
>         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. Special-case
>         if (associated(m) .and. m%t).  If an .and. or .or. expression
>         has a function or a non-function, exchange the operands.
>
>2018-06-11  Thomas Koenig  <[hidden email]>
>
>         PR fortran/57160
>         PR fortran/85599
>         * gfortran.dg/logical_evaluation_1.f90: New test.
>         * gfortran.dg/alloc_comp_default_init_2.f90: Fix code which
>         implicitly depends on short-circuiting.
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Handling of .and. and .or. expressions

Thomas Koenig-6
In reply to this post by Dominique d'Humières-2
Hi Dominique,

> As I said in one of the PRs, I don’t like the warnings triggering with -Wall.

Well, we have a potential problem with code which, at first glance,
looks good but turns out to cause problems.  I think this is what
-Wsurprising is for.

Note that I was trying to be being "nice" by making sure that functions
which are not marked as pure, but behave like a pure function, are
not flagged.

> Also I don’t understand what is special with ASSOCIATED, as shown by the following code

I can easily add ALLOCATED. Makes sense.

> In addition I don’t understand the associated warning: in short-circuit evaluation ASSOCIATED(m) (or ALLOCATED(m)) does protect m%T to be accessed.

That is the whole point: Fortran does not have short circuit evaluation.
Very many people assume that it does (or that everything is always
evaluated), and this causes latent bugs.

If we flag these problems, we do our users a service by pointing out
this kind of error. Of course, this not help if we hide the warning
too deep, which is why I think that -Wall (which includes -Wsurprising)
is appropriate.

Regards

        Thomas
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Handling of .and. and .or. expressions

Thomas Koenig-6
In reply to this post by Janus Weil-3
Am 13.06.2018 um 14:20 schrieb Janus Weil:

> Hi Thomas,
>
>
>> the attached patch introduces the following changes:
>
> thanks a lot for working on this!
>
>
>> If a logical .and. or .or. expression contains a reference to a
>> function
>> which is impure and which also does not behave like a pure function
>> (i.e. does not have the implicit_pure attribute set), it emits a
>> warning with -Wsurprising that the function might not be evaluated.
>> (-Wsurprising is enabled by -Wall).
>
> I think you are warning about too many cases here. Warning about an impure function as the *second* operand of the logical operators should be enough, I think. AFAICS there is no danger of the first operand not being evaluated, right?

Not necessarily.  The middle end, at the moment, may
lack this optimization; at least nobody has so far come
up with a test case that demonstrates otherwise.  On the
other hand, people who know the middle end extremely well have
indicated that they expect optimiazations to happen with
TRUTH_AND_EXPR, so I am far from certaint that this behavior
will continue.

>
>> It special cases the idiom  if (associated(m) .and. m%t) which
>> people appear to use.
>
> I don't fully understand why you do this, but in any case it should not be necessary if you warn about the second operand only.

Well, it is an easy mistake to make, and it is (as above) liable
to break at any time.

Also ASSOCIATED (and ALLOCATED) are pure, so normally I would
not warn on these.

>> And, if there is an expression like   func() .and. flag , it
>> reverses the test as an optimization.
>
> I really don't like this part. It actually introduces more problems of the sort that we're trying to warn about ...

If we actually perform this operation, we will have warned about this
before (with -Wsurprising).

And of course, the compiler can always reverse the order...

Regards

        Thomas
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Handling of .and. and .or. expressions

Steve Kargl
In reply to this post by Thomas Koenig-6
On Mon, Jun 11, 2018 at 09:22:27PM +0200, Thomas Koenig wrote:
>
> Regression-tested (which found one bug in the testsuite).
>
> OK for trunk?
>

I fine with the intent of the patch (see below).

PS: I think there may be some confusion on what IMPURE implies.

> Index: fortran/resolve.c
> ===================================================================
> --- fortran/resolve.c (Revision 261388)
> +++ fortran/resolve.c (Arbeitskopie)
> @@ -3807,7 +3807,43 @@ lookup_uop_fuzzy (const char *op, gfc_symtree *uop
>    return gfc_closest_fuzzy_match (op, candidates);
>  }
>  
> +/* Callback finding an impure function as an operand to an .and. or
> +   .or.  expression.  Remember the last function warned about to
> +   avoid double warnings when recursing.  */
>  
> +static int
> +impure_function_callback (gfc_expr **e, int *walk_subtrees ATTRIBUTE_UNUSED,
> +  void *data)
> +{
> +  gfc_expr *f = *e;
> +  const char *name;
> +  static gfc_expr *last = NULL;
> +  bool *found = (bool *) data;
> +
> +  if (f->expr_type == EXPR_FUNCTION)
> +    {
> +      *found = 1;

Why not true?  *found is declared to be bool.

> +      if (name)
> + gfc_warning (OPT_Wsurprising, "Impure function %qs at %L "
> +     "might not be evaluated", name, &f->where);
> +      else
> + gfc_warning (OPT_Wsurprising, "Impure function at %L "
> +     "might not be evaluated", &f->where);

I think that this can simply be "Function %qs at %L may not be evaluated"

> +      /* Some people code which depends on the short-circuiting that
> + Fortran does not provide, such as

The above seems a little muddled.  I suggest a shorter comment.
"Some programmers assume that Fortran supports short-circuiting in logical
 expression, which can lead to surprising behavior.  For example, in the
 following

> +
> + if (associated(m) .and. m%t) then

 m%t may be evaluated before associated(m).

> + So, warn about this idiom. However, avoid breaking
> + it on purpose.  */
> +
> +      if (op1->expr_type == EXPR_FUNCTION && op1->value.function.isym
> +  && op1->value.function.isym->id == GFC_ISYM_ASSOCIATED)
> + {
> +  gfc_expr *e = op1->value.function.actual->expr;
> +  gfc_expr *en = op1->value.function.actual->next->expr;
> +  if (en == NULL && gfc_check_dependency (e, op2, true))
> +    {
> +      gfc_warning (OPT_Wsurprising, "%qs function call at %L does "
> +   "not guard expression at %L", "ASSOCIATED",
> +   &op1->where, &op2->where);
> +      dont_move = true;
> +    }
> + }
> +
> +      /* A bit of optimization: Transfer if (f(x) .and. flag)
> + into if (flag .and. f(x)), to save evaluation of a

s/transfer/transform

I would also put quotes around the Fortran code.

> + function.  The middle end should be capable of doing
> + this with a TRUTH_AND_EXPR, but it currently does not do
> + so. See PR 85599.  */

The rest looks ok, but I'm not sure if this addresses Janus'
concerns.  

--
Steve
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Handling of .and. and .or. expressions

Dominique d'Humières-2
In reply to this post by Thomas Koenig-6

>> In addition I don’t understand the associated warning: in short-circuit evaluation ASSOCIATED(m) (or ALLOCATED(m)) does protect m%T to be accessed.
>
> That is the whole point: Fortran does not have short circuit evaluation.
> Very many people assume that it does (or that everything is always
> evaluated), and this causes latent bugs.

I don’t understand. Short-circuit evaluation is explicitly allowed by the standard.

Dominique

> Regards
>
> Thomas

Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Handling of .and. and .or. expressions

Adam Hirst
On 13/06/18 21:46, Dominique d'Humières wrote:
> I don’t understand. Short-circuit evaluation is explicitly allowed by the standard.
>
> Dominique
Allowed, but not required, I think is the position.


signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Handling of .and. and .or. expressions

Thomas Koenig-6
In reply to this post by Dominique d'Humières-2
Am 13.06.2018 um 22:46 schrieb Dominique d'Humières:
>
>>> In addition I don’t understand the associated warning: in short-circuit evaluation ASSOCIATED(m) (or ALLOCATED(m)) does protect m%T to be accessed.
>>
>> That is the whole point: Fortran does not have short circuit evaluation.
>> Very many people assume that it does (or that everything is always
>> evaluated), and this causes latent bugs.
>
> I don’t understand. Short-circuit evaluation is explicitly allowed by the standard.

Also legal are:

Short-circuit evaluation in reverse order, parallel evaluation,
evaluation of both expressions in the order they appear or evaluation
of both expressions in reverse order (I think I have all cases there,
but I may have missed one :-)

The problem is that the user should not depend on it, and
a compiler is free to chose whatever is most suited.

Regards

        Thomas
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Handling of .and. and .or. expressions

Janne Blomqvist-3
In reply to this post by Thomas Koenig-6
On Mon, Jun 11, 2018 at 10:22 PM, Thomas Koenig <[hidden email]>
wrote:

> Hello world,
>
> the attached patch introduces the following changes:
>
> If a logical .and. or .or. expression contains a reference to a function
> which is impure and which also does not behave like a pure function
> (i.e. does not have the implicit_pure attribute set), it emits a
> warning with -Wsurprising that the function might not be evaluated.
> (-Wsurprising is enabled by -Wall).
>
> It special cases the idiom  if (associated(m) .and. m%t) which
> people appear to use.
>
> And, if there is an expression like   func() .and. flag , it
> reverses the test as an optimization. The middle end should be
> capable of doing this, but apparently it doesn't, so the front
> end might as well do this.
>

What about more complicated expressions like, say, "func() .and. flag .and
func2() .and. flag2" etc.? Can it move all the function calls to the end?


>
> What it does not do is one part of PR 57160, i.e. warn against
> if (a /= 0 .and. 1/a > 5) which people who are used to C might
> also like to write.
>
> There is already quite some discussion in the PRs, especially 85599,
> where not all people were of the same opinion. Let us see where the
> discussion here leads us.
>


IMHO it's a mistake that the standard refuses to specify the evaluation
strategy in the name of some rather minor hypothetical performance benefit.
Either

a) short-circuiting in left-to-right order

or

b) must evaluate all the arguments in left-to-right order

My preference would be for a) as that is what most languages are doing, but
even b) would be better than leaving it undefined.

Also, there are AFAIU other similar weirdness with impure functions. The
standard allows a compiler to optimize

y = f(x) + f(x)

into

y = 2 * f(x)

even if f is impure, which is totally bonkers. Or even not call f at all,
if the compiler determines that y is not needed.


--
Janne Blomqvist
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Handling of .and. and .or. expressions

Janus Weil-2
In reply to this post by Thomas Koenig-6


Am 13. Juni 2018 22:39:38 MESZ schrieb Thomas Koenig <[hidden email]>:

>>> If a logical .and. or .or. expression contains a reference to a
>>> function
>>> which is impure and which also does not behave like a pure function
>>> (i.e. does not have the implicit_pure attribute set), it emits a
>>> warning with -Wsurprising that the function might not be evaluated.
>>> (-Wsurprising is enabled by -Wall).
>>
>> I think you are warning about too many cases here. Warning about an
>impure function as the *second* operand of the logical operators should
>be enough, I think. AFAICS there is no danger of the first operand not
>being evaluated, right?
>
>Not necessarily.  The middle end, at the moment, may
>lack this optimization; at least nobody has so far come
>up with a test case that demonstrates otherwise.  On the
>other hand, people who know the middle end extremely well have
>indicated that they expect optimiazations to happen with
>TRUTH_AND_EXPR, so I am far from certaint that this behavior
>will continue.

IIUC, TRUTH_AND_EXPR corresponds to C's && operator, which is inherently assymetric and guaranteed to short-circuit. So I don't think the middle-end would ever switch operands here. I hope your ominous "people who know the middle end extremely well" will correct me if I'm wrong :)


>>> It special cases the idiom  if (associated(m) .and. m%t) which
>>> people appear to use.
>>
>> I don't fully understand why you do this, but in any case it should
>not be necessary if you warn about the second operand only.
>
>Well, it is an easy mistake to make, and it is (as above) liable
>to break at any time.

You still haven't stated clearly the rationale for this warning. As I see it, Fortran does not require the compiler to do short-circuiting, but gfortran still does it. So AFAICS there is currently no reason to throw a warning on this case, right?

I don't think we need warnings for hypothetical situations. In fact, ifort might have reason to warn about this case, because it does not do short-circuiting.


>>> And, if there is an expression like   func() .and. flag , it
>>> reverses the test as an optimization.
>>
>> I really don't like this part. It actually introduces more problems
>of the sort that we're trying to warn about ...
>
>If we actually perform this operation, we will have warned about this
>before (with -Wsurprising).

But we do not perform this operation at present, and I really don't think we should.


>And of course, the compiler can always reverse the order...

If you're talking about the middle-end here, I don't think it can (see discussion above).

Cheers,
Janus

Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Handling of .and. and .or. expressions

Janne Blomqvist-3
In reply to this post by Janne Blomqvist-3
On Thu, Jun 14, 2018 at 1:14 PM, Janus Weil <[hidden email]> wrote:

>
>
> Am 14. Juni 2018 11:41:03 MESZ schrieb Janne Blomqvist <
> [hidden email]>:
> >On Thu, Jun 14, 2018 at 11:38 AM, Janus Weil <[hidden email]>
> >wrote:
> >
> >>
> >> Am 14. Juni 2018 10:05:24 MESZ schrieb Janne Blomqvist <
> >> [hidden email]>:
> >>
> >> >Either
> >> >
> >> >a) short-circuiting in left-to-right order
> >> >
> >> >or
> >> >
> >> >b) must evaluate all the arguments in left-to-right order
> >> >
> >> >My preference would be for a) as that is what most languages are
> >doing,
> >>
> >> Well, C/C++ has two different kinds of operators that give you the
> >choice
> >> to specify whether you want short-circuiting or not.
> >
> >Presumably you're thinking of "&" and "|". But to be pedantic, those
> >are
> >bitwise operators, not non-short-circuiting boolean operators
>
> Yes, but they work perfectly fine as logical operators on booleans, don't
> they?
>
>
> >There are however situations where
> >they are not equivalent to hypothetical non-short-circuiting boolean
> >operators!
>
> Could you give a simple example of such a case?
>

With the usual representation of integers, for A=1, B=2, both A and B by
themselves will evaluate as true, A&&B will evaluate to true, but A&B will
evaluate to false.

>>but
> >> >even b) would be better than leaving it undefined.
> >>
> >> Agreed.
> >>
> >> In my opinion the best strategy for gfortran in the current situation
> >> would be to apply short-circuiting whenever it can be proven that it
> >has no
> >> observable consequences. E.g. a function call should not be optimized
> >away
> >> unless it is pure.
> >
> >Hmm, why? I don't see why always evaluating everything is less wrong
> >than
> >short-circuiting?
>
> I'm not saying it is "less wrong", but it can be less efficient. In that
> sense my proposed strategy is a compromise that tries to avoid harmful
> optimizations but still does optimization where it is safe.
>

If the user assumes short-circuiting, then evaluating everything is
surprising. If the user assumes everything will be evaluated,
short-circuiting is surprising. So whatever is done, somebody will think
we're doing it wrong. So in lieu of the standards committee getting their
act together on this issue, I suggest we do whatever is most efficient, and
generate a warning.



--
Janne Blomqvist
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Handling of .and. and .or. expressions

Dominique d'Humières-2
In reply to this post by Thomas Koenig-6
Point taken! However will you warn for

-1<=x .and. x<=1
x>0.0 .and. y>0.0
… ?

If yes, this will be an incredible noise. If no, what about

-1<=x .and. x<=1 .and. asin(x)<1.0
x>0.0 .and. y>0.0 .and. log(x+y)<0.0

Dominique

> Le 14 juin 2018 à 07:15, Thomas Koenig <[hidden email]> a écrit :
>
> Am 13.06.2018 um 22:46 schrieb Dominique d'Humières:
>>>> In addition I don’t understand the associated warning: in short-circuit evaluation ASSOCIATED(m) (or ALLOCATED(m)) does protect m%T to be accessed.
>>>
>>> That is the whole point: Fortran does not have short circuit evaluation.
>>> Very many people assume that it does (or that everything is always
>>> evaluated), and this causes latent bugs.
>> I don’t understand. Short-circuit evaluation is explicitly allowed by the standard.
>
> Also legal are:
>
> Short-circuit evaluation in reverse order, parallel evaluation,
> evaluation of both expressions in the order they appear or evaluation
> of both expressions in reverse order (I think I have all cases there,
> but I may have missed one :-)
>
> The problem is that the user should not depend on it, and
> a compiler is free to chose whatever is most suited.
>
> Regards
>
> Thomas

Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Handling of .and. and .or. expressions

Steve Kargl
In reply to this post by Thomas Koenig-6
On Thu, Jun 14, 2018 at 11:55:45AM +0200, Jakub Jelinek wrote:
>
> I bet gfortran evaluates the side-effects left-to-right and evaluates both
> arguments always, right?
>

Currently, gfortran evaluates a logical expression left-to-right
due to its use of the middle-end's TRUTH_AND_EXPR.  The Fortran
standard does not require this left-to-right evaluation.

Janus found that with '.false. .and. check()' gfortran will not
evaluate check().  This is a valid Fortran optimization in that
'.false.' and '.false. .and. check()' are equivalent expessions
no matter what the return value of check() is.  It does not matter
if check() is PURE or IMPURE.  He then found with the operands
reversed that check() is evaluated.  He traced this behavior to
TRUTH_AND_EXPR, where C allows short-circuiting and it performs
left-to-right evaluation.

I've already provided a sloppy patch in comment #33 of PR85599
that forces gfortran to evaluate both operands.  The patch is
equivalent to writing

tmp1 = .false.
tmp2 = check()
if (tmp1 .and. tmp2) ...

The patch is sloppy because this forced evaulation should be
enabled by -fno-short-circuit (or some such option).

Thomas' patch with a warning is simply meant to help programmers
who aren't familiar with the Fortran standard.  He's trying to
use PUREness as a way to surpress the warning.  A PURE function
by design does not have side-effects so eliminating the evaluation
of PURE function should be okay.  An IMPURE function or an unmarked
function may or may not have side effects.    

The moral of the story:  Don't use functions with side-effects.

--
Steve
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Handling of .and. and .or. expressions

Thomas Koenig-6
In reply to this post by Janne Blomqvist-3
Am 14.06.2018 um 10:38 schrieb Janus Weil:

>> Also, there are AFAIU other similar weirdness with impure functions.
>> The
>> standard allows a compiler to optimize
>>
>> y = f(x) + f(x)
>>
>> into
>>
>> y = 2 * f(x)
>>
>> even if f is impure, which is totally bonkers. Or even not call f at
>> all,
>> if the compiler determines that y is not needed.
> Yes, that is the same kind of craziness. I hope gfortran does not actually do this?

I would vote for this, but currently it is not done unless
-faggressive-function-elimination is specified.

By the way, there is a bit of strangeness about this. People use -Ofast
knowing it will break all sorts of standards for numercial computation,
but they balk at using an optimization that is explicitly permitted
by the standard. Oh well...
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Handling of .and. and .or. expressions

Thomas Koenig-6
In reply to this post by Thomas Koenig-6
Am 14.06.2018 um 11:55 schrieb Jakub Jelinek:
> Or warn users that there is no evaluation ordering between the first and
> second operand, that both operands are evaluated and it is unspecified which
> is evaluated first?  Wouldn't you then just warn all the time?  Even without
> any impure procedures, you could have
> l = associated (m)
> if (l .and. m%t)
> or similar and m%t could be evaluated before l.

It is not the aim of this patch to catch any and all dubious programming
practices. It is the aim of this patch to flag a few well-known errors
that compiler writers use.

> I bet gfortran evaluates the side-effects left-to-right and evaluates both
> arguments always, right?

gfortran hands a TRUTH_AND_EXPR to the middle end. You know better what
this means than I do.
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Handling of .and. and .or. expressions

Thomas Koenig-6
In reply to this post by Dominique d'Humières-2
Hi Dominique,

> Point taken! However will you warn for
>
> -1<=x .and. x<=1
> x>0.0 .and. y>0.0
> … ?

> If yes, this will be an incredible noise. If no, what about
>
> -1<=x .and. x<=1 .and. asin(x)<1.0
> x>0.0 .and. y>0.0 .and. log(x+y)<0.0

The patch can be extended, sure. I do not want to catch
all possible problems on the first try :-)

Regards

        Thomas
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Handling of .and. and .or. expressions

Janne Blomqvist-3
In reply to this post by Janne Blomqvist-3
On Fri, Jun 15, 2018 at 12:22 PM, Janus Weil <[hidden email]> wrote:

> Am 14. Juni 2018 12:40:19 MESZ schrieb Janne Blomqvist <
> [hidden email]>:
> In Fortran, it still feels like functions as such are second-class
> citizens. People seriously advise against using them. Doesn't really help
> the attractivity of the language.


Yes. Back when I followed c.l.f, several experts did advise people to never
use functions unless they were pure (or more or less effectively so, if
they didn't fulfill the standard requirements for purity). Considering that
at least some of those same experts were also part of the Fortran standards
committee, I just find it very strange that, to the best of my knowledge,
no effort to fix this has been done.

IMHO it is completely insane to optimize out impure functions, just for a
> little bit of speedup, but sacrificing compiler-independent results.
>
> I really don't understand why I'm so alone here with this opinion.
>

I would agree with you if there were some substantial majority opinion
among Fortran programmers that all the parts of a logical expression are
always evaluated, contrary to what the standard actually guarantees. But as
we have seen e.g. in the PR's that this patch attempt to fix, some people
actually seem to assume short-circuiting, e.g. by writing code like

a /= 0 .and. b/a > c

or

associated(t) .and. func(t)


--
Janne Blomqvist
Reply | Threaded
Open this post in threaded view
|

Re: [patch, fortran] Handling of .and. and .or. expressions

Steve Kargl
In reply to this post by Janne Blomqvist-3
On Fri, Jun 15, 2018 at 08:27:49PM +0300, Janne Blomqvist wrote:

> On Fri, Jun 15, 2018 at 8:06 PM, Thomas Koenig <[hidden email]>
> wrote:
>
> >
> > What about more complicated expressions like, say, "func() .and. flag .and
> >> func2() .and. flag2" etc.? Can it move all the function calls to the end?
> >>
> >
> > Not at the moment.
> >
> > Like many of the front-end optimizations, this aims for the easy 80%
> > which are relatively easy to achieve.
> >
>
> Come to think about it, is this optimization also done for impure
> functions? E.g. if func() changes the value of flag, then exchanging the
> order they are evaluated might change the result. Of course, this is all
> the fault of the programmer, but still..

It doesn't matter.  The code is invalid.  A compiler can do anything.

  F2018: 10.1.4 Evaluation of operations

  An intrinsic operation requires the values of its operands.

  Execution of a function reference in the logical expression in
  an IF statement (11.1.8.4), the mask expression in a WHERE
  statement (10.2.3.1), or the concurrent-limits and concurrent-steps
  in a FORALL statement (10.2.4) is permitted to define variables in
  the subsidiary action-stmt, where-assignment-stmt, or
  forall-assignment-stmt respectively.  Except in those cases:

   · the evaluation of a function reference shall neither affect
     nor be affected by the evaluation of any other entity within
     the statement;

   · if a function reference causes definition or undefinition of
     an actual argument of the function, that argument or any
     associated entities shall not appear elsewhere in the same
     statement.
>
> But at least for pure functions, this optimization looks Ok.
>

Why is everyone fixated on PURE vs IMPURE functions?  The Fortran
standard allows short circuiting regardless of the pureness of
a function.

  F2018: 10.1.5.4.2 Evaluation of logical intrinsic operations

  Once the interpretation of a logical intrinsic operation is
  established, the processor may evaluate any other expression
  that is logically equivalent, provided that the integrity of
  parentheses in any expression is not violated.

  Two expressions of type logical are logically equivalent if
  their values are equal for all possible values of their
  primaries.

'.false. .and. func()' and 'func() .and. .false.' are logically
equivalent to '.false.'

  F2018: 10.1.7 Evaluation of operands

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

In fact, F2018 NOTE 10.28 describes this exact situation.

  NOTE 10.28
  This principle is most often applicable to logical expressions,
  zero-sized arrays, and zero-length strings, but it applies to
  all expressions.

  For example, in evaluating the expression

      X > Y .OR. L(Z)

  where X, Y, and Z are real and L is a function of type logical,
  the function reference L(Z) need not be evaluated if X is greater
  than Y.

There is no statement about L(Z) being PURE or not.

Thomas' patch is simply trying to alert users to the fact that
their code may not do what they think.  His check for pureness
versus impureness is an attempt to reduce warnings where the
the non-evaluation of pure function cannot change the outcome
of program.

Finally, I think that he can change the warning from

+  if (f->symtree == NULL || f->symtree->n.sym == NULL
+      || !gfc_implicit_pure (f->symtree->n.sym))
+    {
+      if (name)
+ gfc_warning (OPT_Wsurprising, "Impure function %qs at %L "
+     "might not be evaluated", name, &f->where);
+      else
+ gfc_warning (OPT_Wsurprising, "Impure function at %L "
+     "might not be evaluated", &f->where);
+    }

to

+  if (f->symtree == NULL || f->symtree->n.sym == NULL
+      || !gfc_implicit_pure (f->symtree->n.sym))
+    {
+      if (name)
+ gfc_warning (OPT_Wsurprising, "Function %qs at %L "
+     "might not be evaluated", name, &f->where);
+      else
+ gfc_warning (OPT_Wsurprising, "Function at %L "
+     "might not be evaluated", &f->where);
+    }
 
That is, remove the word 'Impure' as it can be misleading.  

--
Steve
123