Patchwork [fortran,4.6/4.7,Regression] Fix PR 48955

login
register
mail settings
Submitter Thomas Koenig
Date May 22, 2011, 3:32 p.m.
Message ID <4DD92C77.5090107@netcologne.de>
Download mbox | patch
Permalink /patch/96755/
State New
Headers show

Comments

Thomas Koenig - May 22, 2011, 3:32 p.m.
Hello world,

the attached patch fixes PR 48955, a wrong-code regression for 4.6 and 
4.7, including the test case from 
http://gcc.gnu.org/ml/fortran/2011-05/msg00093.html .  It follows the 
outline from http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955#c7 .

Regression-tested. OK for trunk?

	Thomas

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

         PR fortran/48955
         * gfortran.h (gfc_reverse):  Change to struct with two boolean
         variables, forward_ok and backward_ok.
         * trans-expr.c (gfc_trans_assignment_1):  Initialize using
         new gfc_reverse struct.
         * trans-array.c (gfc_init_loopinfo):  Likewise.
         (gfc_trans_scalarized_loop):  Use new gfc_reverse.
         * dependency.c (gfc_dep_resovler):  Use new gfc_reverse.
         If a forward dependency is found, a backward dependency is
         no longer possible and vice versa.

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

         PR fortran/48955
         * gfortran.dg/dependency_40.f90:  New test.
! { dg-do run }
! PR 48955 - missing array temporary when there was both a forward
! and a backward dependency.
! Test case slightly modified from the original one by Kacper Kowalik.
program ala
   implicit none

   integer, parameter  :: n = 6
   real, dimension(n), parameter :: result = [1.,10.,30.,90.,270., 243.];
   real, dimension(n) :: v0, v1
   character(len=80) :: line1, line2

   v0 = [1.0, 3.0, 9.0, 27.0, 81.0, 243.0]
   v1 = v0

   v1(2:n-1) = v1(1:n-2) + v1(3:n)
   if (any(v1 /= result)) call abort
   v1 = v0
   v1(2:n-1) = v0(1:n-2) + v0(3:n)
   if (any(v1 /= result)) call abort

   v1 = v0
   v1(2:n-1) = v1(3:n) + v1(1:n-2)
   if (any(v1 /= result)) call abort
   v1 = v0
   v1(2:n-1) = v0(3:n) + v0(1:n-2)
   if (any(v1 /= result)) call abort

end program ala
Paul Richard Thomas - May 22, 2011, 6:48 p.m.
Dear Thomas,

I am sorry for the long silence on this PR.  I have been up to my
eyeballs in daytime work.

I have posted a simpler alternative on the PR that uses your
suggestion that forward and backward dependences need to to be
recorded to get this right.

I believe that it's OK but have only now had the opportunity to put it
on to regtest.

Cheers

Paul

On Sun, May 22, 2011 at 5:32 PM, Thomas Koenig <tkoenig@netcologne.de> wrote:
> Hello world,
>
> the attached patch fixes PR 48955, a wrong-code regression for 4.6 and 4.7,
> including the test case from
> http://gcc.gnu.org/ml/fortran/2011-05/msg00093.html .  It follows the
> outline from http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48955#c7 .
>
> Regression-tested. OK for trunk?
>
>        Thomas
>
> 2011-05-22  Thomas Koenig  <tkoenig@gcc.gnu.org>
>
>        PR fortran/48955
>        * gfortran.h (gfc_reverse):  Change to struct with two boolean
>        variables, forward_ok and backward_ok.
>        * trans-expr.c (gfc_trans_assignment_1):  Initialize using
>        new gfc_reverse struct.
>        * trans-array.c (gfc_init_loopinfo):  Likewise.
>        (gfc_trans_scalarized_loop):  Use new gfc_reverse.
>        * dependency.c (gfc_dep_resovler):  Use new gfc_reverse.
>        If a forward dependency is found, a backward dependency is
>        no longer possible and vice versa.
>
> 2011-05-22  Thomas Koenig  <tkoenig@gcc.gnu.org>
>
>        PR fortran/48955
>        * gfortran.dg/dependency_40.f90:  New test.
>
>

Patch

Index: trans-expr.c
===================================================================
--- trans-expr.c	(Revision 173754)
+++ trans-expr.c	(Arbeitskopie)
@@ -6068,9 +6068,14 @@  gfc_trans_assignment_1 (gfc_expr * expr1, gfc_expr
 
       /* Calculate the bounds of the scalarization.  */
       gfc_conv_ss_startstride (&loop);
+
       /* Enable loop reversal.  */
       for (n = 0; n < loop.dimen; n++)
-	loop.reverse[n] = GFC_REVERSE_NOT_SET;
+	{
+	  loop.reverse[n].forward_ok = true;
+	  loop.reverse[n].backward_ok = true;
+	}
+
       /* Resolve any data dependencies in the statement.  */
       gfc_conv_resolve_dependencies (&loop, lss, rss);
       /* Setup the scalarizing loops.  */
Index: trans-array.c
===================================================================
--- trans-array.c	(Revision 173754)
+++ trans-array.c	(Arbeitskopie)
@@ -2244,7 +2244,8 @@  gfc_init_loopinfo (gfc_loopinfo * loop)
   for (n = 0; n < GFC_MAX_DIMENSIONS; n++)
     {
       loop->order[n] = n;
-      loop->reverse[n] = GFC_CANNOT_REVERSE;
+      loop->reverse[n].forward_ok = true;
+      loop->reverse[n].backward_ok = false;
     }
 
   loop->ss = gfc_ss_terminator;
@@ -2925,7 +2926,8 @@  gfc_trans_scalarized_loop_end (gfc_loopinfo * loop
     }
   else
     {
-      bool reverse_loop = (loop->reverse[n] == GFC_REVERSE_SET)
+      bool reverse_loop = loop->reverse[n].backward_ok
+	                     && !loop->reverse[n].forward_ok
 			     && (loop->temp_ss == NULL);
 
       loopbody = gfc_finish_block (pbody);
Index: gfortran.h
===================================================================
--- gfortran.h	(Revision 173754)
+++ gfortran.h	(Arbeitskopie)
@@ -576,12 +576,10 @@  typedef enum
 }
 gfc_fcoarray;
 
-typedef enum
+typedef struct
 {
-  GFC_REVERSE_NOT_SET,
-  GFC_REVERSE_SET,
-  GFC_CAN_REVERSE,
-  GFC_CANNOT_REVERSE
+  bool forward_ok;
+  bool backward_ok;
 }
 gfc_reverse;
 
Index: dependency.c
===================================================================
--- dependency.c	(Revision 173754)
+++ dependency.c	(Arbeitskopie)
@@ -1807,34 +1807,35 @@  gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gf
 
 	      /* Now deal with the loop reversal logic:  This only works on
 		 ranges and is activated by setting
-				reverse[n] == GFC_CAN_REVERSE
+		     reverse[n].backward_ok = true
 		 The ability to reverse or not is set by previous conditions
 		 in this dimension.  If reversal is not activated, the
 		 value GFC_DEP_BACKWARD is reset to GFC_DEP_OVERLAP.  */
 	      if (rref->u.ar.dimen_type[n] == DIMEN_RANGE
-		    && lref->u.ar.dimen_type[n] == DIMEN_RANGE)
+		  && lref->u.ar.dimen_type[n] == DIMEN_RANGE)
 		{
-		  /* Set reverse if backward dependence and not inhibited.  */
-		  if (reverse && reverse[n] != GFC_CANNOT_REVERSE)
-		    reverse[n] = (this_dep == GFC_DEP_BACKWARD) ?
-			         GFC_REVERSE_SET : reverse[n];
+		  if (reverse)
+		    {
+		      if (this_dep == GFC_DEP_FORWARD
+			  || this_dep == GFC_DEP_OVERLAP
+			  || this_dep == GFC_DEP_ERROR)
+			reverse[n].backward_ok = false;
 
-		  /* 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)
+		      if (this_dep == GFC_DEP_BACKWARD
+			  || this_dep == GFC_DEP_OVERLAP
+			  || this_dep == GFC_DEP_ERROR)
+			reverse[n].forward_ok = false;
+
+		      if (!reverse[n].forward_ok && !reverse[n].backward_ok)
+			this_dep = GFC_DEP_OVERLAP;
+		    }
+		  else
 		    {
-	              reverse[n] = GFC_CANNOT_REVERSE;
-		      if (this_dep != GFC_DEP_FORWARD)
+		      /* If no intention of reversing, convert backward
+			 dependence to overlap.  */
+		      if (this_dep == GFC_DEP_BACKWARD)
 			this_dep = GFC_DEP_OVERLAP;
 		    }
-
-		  /* If no intention of reversing or reversing is explicitly
-		     inhibited, convert backward dependence to overlap.  */
-		  if (this_dep == GFC_DEP_BACKWARD
-		      && (reverse == NULL || reverse[n] == GFC_CANNOT_REVERSE))
-		    this_dep = GFC_DEP_OVERLAP;
 		}
 
 	      /* Overlap codes are in order of priority.  We only need to