[Bug fortran/48955] New: [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary

[Bug fortran/48955] New: [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955

           Summary: [4.6/4.7 Regression] Wrong result for array assignment
                    due to missing temporary
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: critical
          Priority: P3
         Component: fortran
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: burnus@gcc.gnu.org
                CC: pault@gcc.gnu.org, tkoenig@gcc.gnu.org


As reported on IRC.

The following program should print:

-10.0000 -20.0000 -20.0000 -10.0000  10.0000  20.0000  20.0000  10.0000 
-10.0000 -20.0000 -20.0000 -10.0000  10.0000  20.0000  20.0000  10.0000 


It does so with gfortran 4.3.4, 4.4.0, 4.5.3, ifort, NAG. However, using
gfortran 4.6.0, 4.7.0 (and for that matter 4.1.2) the first line is wrong:

-10.0000 -20.0000 -25.0000 -17.5000   6.2500  18.1250  24.0625  10.0000
-10.0000 -20.0000 -20.0000 -10.0000  10.0000  20.0000  20.0000  10.0000


program ala
   implicit none

   integer, parameter  :: n = 8
   real, dimension(n) :: v0, v1

   v0 = [-10.0, -10., -10., -10., 10., 10., 10., 10.]
   v1 = v0

   v1(2:n-1) = 0.5*(v1(1:n-2) + v1(3:n) + 2.0*v1(2:n-1))
   write(*,'(8(F8.4,1X))') v1
   v1 = v0
   v1(2:n-1) = 0.5*(v0(1:n-2) + v0(3:n) + 2.0*v0(2:n-1))
   write(*,'(8(F8.4,1X))') v1
end program ala


First regression hunting shows:
  Works: 2010-07-16-r162255
  Fails: 2010-08-28-r163612

[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955

Tobias Burnus <burnus at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |4.3.4, 4.4.0, 4.5.3
   Target Milestone|---                         |4.6.1
      Known to fail|                            |4.1.2, 4.6.0, 4.7.0

--- Comment #1 from Tobias Burnus <burnus at gcc dot gnu.org> 2011-05-10 21:10:10 UTC ---
Works: 2010-08-02  r162824  cca7236e5fabb3791d494683d1f53f3c09d545e5
Fails: 2010-08-02  r162829  644e9dad11b9ba317bd11726569b5d8bc648950f

Thus, the culprit is:

New Revision: 162829
URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=162829

2010-08-02  Thomas Koenig  <tkoenig@gcc.gnu.org>

    PR fortran/45159
    * depencency.c (gfc_dep_resolver):  Fix logic for when a loop
    can be reversed.

2010-08-02  Thomas Koenig  <tkoenig@gcc.gnu.org>

    PR fortran/45159
    * gfortran.dg/dependency_29.f90:  New test.

[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955

--- Comment #2 from Tobias Burnus <burnus at gcc dot gnu.org> 2011-05-11 09:06:30 UTC ---
The obvious problem is that there is no temporary needed for either of the
variable expressions on the right hand side - only for their combination.

Thus, one needs to check whether as some point whether one part of the RHS
expression is GFC_DEP_FORWARD while another part is GFC_DEP_BACKWARD.

My impression is that one needs an additional argument to
  gfc_check_dependency
of the type "gfc_reverse *reverse" and two local variables like
  gfc_reverse reverse1, reverse2;
which one passes to gfc_check_dependency/gfc_dep_resolver, and - if "reverse"
is not NULL, propagates it to the caller. Then, for operators, the reverse
setting can be compared.

[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary

[pault at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955

Paul Thomas <pault at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011.05.11 09:25:24
         AssignedTo|unassigned at gcc dot       |pault at gcc dot gnu.org
                   |gnu.org                     |
     Ever Confirmed|0                           |1

[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P4
                 CC|                            |jakub at gcc dot gnu.org

[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary

[tkoenig at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955

--- Comment #3 from Thomas Koenig <tkoenig at gcc dot gnu.org> 2011-05-11 19:00:06 UTC ---
Hmm... I wonder if this does the trick?  It fixes the test case,
and passes all regression tests... Paul, what do you think?

Index: dependency.c
===================================================================
--- dependency.c        (Revision 173389)
+++ dependency.c        (Arbeitskopie)
@@ -1822,7 +1822,6 @@ gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gf
                  /* Inhibit loop reversal if dependence not compatible.  */
                  if (reverse && reverse[n] != GFC_REVERSE_NOT_SET
                        && this_dep != GFC_DEP_EQUAL
-                       && this_dep != GFC_DEP_BACKWARD
                        && this_dep != GFC_DEP_NODEP)
                    {
                      reverse[n] = GFC_CANNOT_REVERSE;

[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary

[pault at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955

--- Comment #4 from Paul Thomas <pault at gcc dot gnu.org> 2011-05-11 19:57:21 UTC ---
Created attachment 24229
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24229
A fix for the PR

This fixes the problem in two steps:
(i) It reverts r162289; and
(ii) It adds the correct initialization for loop.reverse[] in
gfc_trans_assignment_1.  This was implemented incorrectly in the fix for
PR24524 (in spite of the correct comment in dependency.c!) and removed at
sometime, I do not know why.

****sigh****

Bootstraps and regtests on x86_64/FC9.  I will add a testcase and submit
formally tomorrow night.  There are probably other assignements, such as in FOR
and WHERE blocks that could do with reversing being enabled.  I'll put thinking
cap on after I have fixed this PR.

Paul

[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955

--- Comment #5 from Tobias Burnus <burnus at gcc dot gnu.org> 2011-05-16 07:27:07 UTC ---
Submitted patch: http://gcc.gnu.org/ml/fortran/2011-05/msg00090.html
It fixes the test case of comment 0, but (cf. review comment) it does not
handle a modified version.

[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary

[paul.richard.thomas at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955

--- Comment #6 from paul.richard.thomas at gmail dot com <paul.richard.thomas at gmail dot com> 2011-05-16 12:48:32 UTC ---
Indeed - I just need to find the time to sort out the logic.
Structurally the patch is OK.

Cheers

Paul

On Mon, May 16, 2011 at 9:56 AM, burnus at gcc dot gnu.org
<gcc-bugzilla@gcc.gnu.org> wrote:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955
>
> --- Comment #5 from Tobias Burnus <burnus at gcc dot gnu.org> 2011-05-16 07:27:07 UTC ---
> Submitted patch: http://gcc.gnu.org/ml/fortran/2011-05/msg00090.html
> It fixes the test case of comment 0, but (cf. review comment) it does not
> handle a modified version.
>
> --
> Configure bugmail: http://gcc.gnu.org/bugzilla/userprefs.cgi?tab=email
> ------- You are receiving this mail because: -------
> You are on the CC list for the bug.
> You are the assignee for the bug.
>

[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary

[tkoenig at netcologne dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955

--- Comment #7 from tkoenig at netcologne dot de <tkoenig at netcologne dot de> 2011-05-16 18:10:03 UTC ---
Hi Paul,

> Indeed - I just need to find the time to sort out the logic.
> Structurally the patch is OK.

I think the logic could be as follows: You could have two flags, one
FORWARD_ALLOWED and one BACKWARD_ALLOWED.

Initialize both flags to true.

Run through all the references which could introduce a dependency.
If there is a GFC_EQUAL dependency, do nothing.  If there is a
GFC_FORWARD dependency, set BACKWARD_ALLOW to false.  If there
is a GFC_BACKWARD dependency, set FORWARD_ALLOW to false.

When selecting the loop direction:

- If FORWARD_ALLOW is set, select a forward loop; else
     If BACKWARD_ALLOW is set, select a forward loop;
        else mark this dimension as needing a temporary.

Does this sound OK?

    Thomas

[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary

[tkoenig at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955

--- Comment #8 from Thomas Koenig <tkoenig at gcc dot gnu.org> 2011-05-21 15:12:47 UTC ---
Created attachment 24320
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24320
Tentative patch

Paul,

what do you think of this approach?  It fixes the test case, and
passes regression-testing.

[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary

[pault at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955

--- Comment #9 from Paul Thomas <pault at gcc dot gnu.org> 2011-05-22 18:28:18 UTC ---
Created attachment 24332
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24332
A fix for the PR

This uses the same basic idea as Thomas' patch but is based on the original
logic

[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary

[tkoenig at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955

--- Comment #10 from Thomas Koenig <tkoenig at gcc dot gnu.org> 2011-05-23 20:01:10 UTC ---
Hi Paul,

just two questions, for my understanding:

With your patch, what is the difference between GFC_CAN_REVERSE
and GFC_REVERSE_NOT_SET?

And why do you initialize loop.reverse close to line 6052 and
close to line 6070 of trans-expr.c?  Is something in between
changing this?

Regards

Thomas

[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary

[paul.richard.thomas at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955

--- Comment #11 from paul.richard.thomas at gmail dot com <paul.richard.thomas at gmail dot com> 2011-05-24 09:43:32 UTC ---
Dear Thomas,


> With your patch, what is the difference between GFC_CAN_REVERSE
> and GFC_REVERSE_NOT_SET?

Perhaps GFC_REVERSE_ENABLED and GFC_REVERSE_INHIBITED would be better.

>
> And why do you initialize loop.reverse close to line 6052 and
> close to line 6070 of trans-expr.c?  Is something in between
> changing this?

Pure cock-up, I think.  Clearly we only need one instance of this.
However, when I monitored the beviour of the code in dependency.c, I
noticed that the reversing was disabled and so added the second block,
whilst failing to notice the first.  Either I screwed up or your last
question is pertinent.  I will look this afternoon.

Many thanks

Paul

PS I will post the patch this afternoon.

[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary

[pault at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955

--- Comment #12 from Paul Thomas <pault at gcc dot gnu.org> 2011-05-26 18:19:40 UTC ---
Author: pault
Date: Thu May 26 18:19:36 2011
New Revision: 174302

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=174302
Log:
2011-05-26  Paul Thomas  <pault@gcc.gnu.org>
        Thomas Koenig  <tkoenig@gcc.gnu.org>

    PR fortran/48955
    * trans-expr.c (gfc_trans_assignment_1): GFC_REVERSE_NOT_SET
    changed to GFC_ENABLE_REVERSE.
    * trans-array.c (gfc_init_loopinfo): GFC_CANNOT_REVERSE changed
    to GFC_INHIBIT_REVERSE.
    * gfortran.h : Enum gfc_reverse is now GFC_ENABLE_REVERSE,
    GFC_FORWARD_SET, GFC_REVERSE_SET and GFC_INHIBIT_REVERSE.
    * dependency.c (gfc_dep_resolver): Change names for elements of
    gfc_reverse as necessary. Change the logic so that forward
    dependences are remembered as well as backward ones. When both
    have appeared, force a temporary.

2011-05-26  Thomas Koenig  <tkoenig@gcc.gnu.org>

    PR fortran/48955
    * gfortran.dg/dependency_40.f90 : New test.


Added:
    trunk/gcc/testsuite/gfortran.dg/dependency_40.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/dependency.c
    trunk/gcc/fortran/gfortran.h
    trunk/gcc/fortran/trans-array.c
    trunk/gcc/fortran/trans-expr.c
    trunk/gcc/testsuite/ChangeLog

[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary

[pault at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955

--- Comment #13 from Paul Thomas <pault at gcc dot gnu.org> 2011-05-26 20:49:11 UTC ---
Author: pault
Date: Thu May 26 20:49:07 2011
New Revision: 174308

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=174308
Log:
2011-05-26  Paul Thomas  <pault@gcc.gnu.org>
        Thomas Koenig  <tkoenig@gcc.gnu.org>

    PR fortran/48955
    * trans-expr.c (gfc_trans_assignment_1): GFC_REVERSE_NOT_SET
    changed to GFC_ENABLE_REVERSE.
    * trans-array.c (gfc_init_loopinfo): GFC_CANNOT_REVERSE changed
    to GFC_INHIBIT_REVERSE.
    * gfortran.h : Enum gfc_reverse is now GFC_ENABLE_REVERSE,
    GFC_FORWARD_SET, GFC_REVERSE_SET and GFC_INHIBIT_REVERSE.
    * dependency.c (gfc_dep_resolver): Change names for elements of
    gfc_reverse as necessary. Change the logic so that forward
    dependences are remembered as well as backward ones. When both
    have appeared, force a temporary.

2011-05-26  Thomas Koenig  <tkoenig@gcc.gnu.org>

    PR fortran/48955
    * gfortran.dg/dependency_40.f90 : New test.


Added:
    branches/gcc-4_6-branch/gcc/testsuite/gfortran.dg/dependency_40.f90
Modified:
    branches/gcc-4_6-branch/gcc/fortran/ChangeLog
    branches/gcc-4_6-branch/gcc/fortran/dependency.c
    branches/gcc-4_6-branch/gcc/fortran/gfortran.h
    branches/gcc-4_6-branch/gcc/fortran/trans-array.c
    branches/gcc-4_6-branch/gcc/fortran/trans-expr.c
    branches/gcc-4_6-branch/gcc/testsuite/ChangeLog

[Bug fortran/48955] [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary

[pault at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955

Paul Thomas <pault at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #14 from Paul Thomas <pault at gcc dot gnu.org> 2011-05-26 21:18:23 UTC ---
Fixed on trunk and 4.6

Paul