Bug 48955 - [4.6/4.7 Regression] Wrong result for array assignment due to missing temporary
[4.6/4.7 Regression] Wrong result for array assignment due to missing temporary
Status: RESOLVED FIXED
Product: gcc
Classification: Unclassified
Component: fortran
4.7.0
: P4 critical
: 4.6.1
Assigned To: Paul Thomas
: wrong-code
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-10 20:10 UTC by Tobias Burnus
Modified: 2011-05-26 21:18 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work: 4.3.4, 4.4.0, 4.5.3
Known to fail: 4.1.2, 4.6.0, 4.7.0
Last reconfirmed: 2011-05-11 09:25:24


Attachments
A fix for the PR (1.44 KB, patch)
2011-05-11 19:57 UTC, Paul Thomas
Details | Diff
Tentative patch (3.61 KB, patch)
2011-05-21 15:12 UTC, Thomas Koenig
Details | Diff
A fix for the PR (4.27 KB, patch)
2011-05-22 18:28 UTC, Paul Thomas
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tobias Burnus 2011-05-10 20:10:32 UTC
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
Comment 1 Tobias Burnus 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.
Comment 2 Tobias Burnus 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.
Comment 3 Thomas Koenig 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;
Comment 4 Paul Thomas 2011-05-11 19:57:21 UTC
Created attachment 24229 [details]
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
Comment 5 Tobias Burnus 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.
Comment 6 paul.richard.thomas@gmail.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.
>
Comment 7 tkoenig@netcologne.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
Comment 8 Thomas Koenig 2011-05-21 15:12:47 UTC
Created attachment 24320 [details]
Tentative patch

Paul,

what do you think of this approach?  It fixes the test case, and
passes regression-testing.
Comment 9 Paul Thomas 2011-05-22 18:28:18 UTC
Created attachment 24332 [details]
A fix for the PR

This uses the same basic idea as Thomas' patch but is based on the original logic
Comment 10 Thomas Koenig 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
Comment 11 paul.richard.thomas@gmail.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.
Comment 12 Paul Thomas 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
Comment 13 Paul Thomas 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
Comment 14 Paul Thomas 2011-05-26 21:18:23 UTC
Fixed on trunk and 4.6

Paul