Patchwork [Fortran] Fix dependency checking regression for 64-bit

login
register
mail settings
Submitter Thomas Koenig
Date Aug. 3, 2010, 4:45 p.m.
Message ID <1280853926.25237.8.camel@linux-fd1f.site>
Download mbox | patch
Permalink /patch/60780/
State New
Headers show

Comments

Thomas Koenig - Aug. 3, 2010, 4:45 p.m.
Hello world,

the attached patch fixes dependency checking for 64-bit systems.  What
used to happen was that the second and third expressions for indices
were converted to 8-byte integers, but not the first one.  This caused
gfc_dep_compare_expr to fail.  Incidentially, it worked with -m32.

The non-conversion of the first expression was introduced some way back
(the test case works for gfortran-4.4.1) so this is indeed a regression.

Regression-tested on x86_64-unknown-linux-gnu.  OK?  Also OK for 4.5 if
we need it?  (I haven't checked yet).

	Thomas

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

	PR fortran/45159
	* dependency.c (gfc_deb_compare_expr):  Remove any integer
	conversion functions to larger types from both arguments.
	Remove handling these functions futher down.

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

	PR fortran/45159
	* gfortran.dg/dependency_30.f90:  New test.
Jack Howarth - Aug. 3, 2010, 9:06 p.m.
On Tue, Aug 03, 2010 at 06:45:26PM +0200, Thomas Koenig wrote:
> Hello world,
> 
> the attached patch fixes dependency checking for 64-bit systems.  What
> used to happen was that the second and third expressions for indices
> were converted to 8-byte integers, but not the first one.  This caused
> gfc_dep_compare_expr to fail.  Incidentially, it worked with -m32.
> 
> The non-conversion of the first expression was introduced some way back
> (the test case works for gfortran-4.4.1) so this is indeed a regression.
> 
> Regression-tested on x86_64-unknown-linux-gnu.  OK?  Also OK for 4.5 if
> we need it?  (I haven't checked yet).

Confirmed that this eliminates unnecessary temporaries on
x86_64-apple-darwin10 for nf.f90. The benchmark
shows a marginal improvement from...

          nf      3.97       10000     30.53      20  0.0959

without the patch to...

          nf      3.72       10000     29.86      14  0.0927

with the patch.
          Jack
> 
> 	Thomas
> 
> 2010-08-03  Thomas Koenig  <tkoenig@gcc.gnu.org>
> 
> 	PR fortran/45159
> 	* dependency.c (gfc_deb_compare_expr):  Remove any integer
> 	conversion functions to larger types from both arguments.
> 	Remove handling these functions futher down.
> 
> 2010-08-03  Thomas Koenig  <tkoenig@gcc.gnu.org>
> 
> 	PR fortran/45159
> 	* gfortran.dg/dependency_30.f90:  New test.
> 

> Index: dependency.c
> ===================================================================
> --- dependency.c	(Revision 162829)
> +++ dependency.c	(Arbeitskopie)
> @@ -180,7 +180,45 @@ gfc_dep_compare_expr (gfc_expr *e1, gfc_expr *e2)
>    gfc_actual_arglist *args1;
>    gfc_actual_arglist *args2;
>    int i;
> +  gfc_expr *n1, *n2;
>  
> +  n1 = NULL;
> +  n2 = NULL;
> +
> +  /* Remove any integer conversion functions to larger types.  */
> +  if (e1->expr_type == EXPR_FUNCTION && e1->value.function.isym
> +      && e1->value.function.isym->id == GFC_ISYM_CONVERSION
> +      && e1->ts.type == BT_INTEGER)
> +    {
> +      args1 = e1->value.function.actual;
> +      if (args1->expr->ts.type == BT_INTEGER
> +	  && e1->ts.kind > args1->expr->ts.kind)
> +	n1 = args1->expr;
> +    }
> +
> +  if (e2->expr_type == EXPR_FUNCTION && e2->value.function.isym
> +      && e2->value.function.isym->id == GFC_ISYM_CONVERSION
> +      && e2->ts.type == BT_INTEGER)
> +    {
> +      args2 = e2->value.function.actual;
> +      if (args2->expr->ts.type == BT_INTEGER
> +	  && e2->ts.kind > args2->expr->ts.kind)
> +	n2 = args2->expr;
> +    }
> +
> +  if (n1 != NULL)
> +    {
> +      if (n2 != NULL)
> +	return gfc_dep_compare_expr (n1, n2);
> +      else
> +	return gfc_dep_compare_expr (n1, e2);
> +    }
> +  else
> +    {
> +      if (n2 != NULL)
> +	return gfc_dep_compare_expr (e1, n2);
> +    }
> +  
>    if (e1->expr_type == EXPR_OP
>        && (e1->value.op.op == INTRINSIC_UPLUS
>  	  || e1->value.op.op == INTRINSIC_PARENTHESES))
> @@ -328,20 +366,6 @@ gfc_dep_compare_expr (gfc_expr *e1, gfc_expr *e2)
>  	 argument lists.  */
>        switch (e1->value.function.isym->id)
>  	{
> -	case GFC_ISYM_CONVERSION:
> -	  /* Handle integer extensions specially, as __convert_i4_i8
> -	     is not only "constant" but also "unary" and "increasing".  */
> -	  if (args1 && !args1->next
> -	      && args2 && !args2->next
> -	      && e1->ts.type == BT_INTEGER
> -	      && args1->expr->ts.type == BT_INTEGER
> -	      && e1->ts.kind > args1->expr->ts.kind
> -	      && e2->ts.type == e1->ts.type
> -	      && e2->ts.kind == e1->ts.kind
> -	      && args2->expr->ts.type == args1->expr->ts.type
> -	      && args2->expr->ts.kind == args2->expr->ts.kind)
> -	    return gfc_dep_compare_expr (args1->expr, args2->expr);
> -	  break;
>  
>  	case GFC_ISYM_REAL:
>  	case GFC_ISYM_LOGICAL:

> subroutine foo(a,b,i,j,k,n)
>   implicit none
>   integer, intent(in) :: i, j, k, n
>   real, dimension(n) :: a,b
>   a(k:i-1) = a(i:j)
> !  b(k+1:i) = b(j-k:k-1)
> end subroutine foo
Tobias Burnus - Aug. 3, 2010, 9:24 p.m.
Thomas Koenig wrote:
> Regression-tested on x86_64-unknown-linux-gnu.  OK?  Also OK for 4.5 if
> we need it?  (I haven't checked yet).

The actual patch looks fine, however, the test case looks incomplete: No 
reference to the PR - and it is completely unclear what you are testing 
for - and ICE? Or for dependencies - if the latter, then you miss a 
check for that! OK if you fix the testcase.

Tobias

> 2010-08-03  Thomas Koenig<tkoenig@gcc.gnu.org>
>
> 	PR fortran/45159
> 	* dependency.c (gfc_deb_compare_expr):  Remove any integer
> 	conversion functions to larger types from both arguments.
> 	Remove handling these functions futher down.
>
> 2010-08-03  Thomas Koenig<tkoenig@gcc.gnu.org>
>
> 	PR fortran/45159
> 	* gfortran.dg/dependency_30.f90:  New test.
>
Thomas Koenig - Aug. 3, 2010, 10:04 p.m.
Tobias Burnus wrote:
> The actual patch looks fine, however, the test case looks incomplete:
> No 
> reference to the PR - and it is completely unclear what you are
> testing 
> for - and ICE? Or for dependencies - if the latter, then you miss a 
> check for that! OK if you fix the testcase.

Thanks for the quick review, and for spotting the faulty test case!

Here's the test case as I committed it:

! { do-do compile }
! { dg-options "-Warray-temporaries" }
! PR 45159 - make sure no temporary is created for this.
subroutine foo(a,b,i,j,k,n)
  implicit none
  integer, intent(in) :: i, j, k, n
  real, dimension(n) :: a,b
  a(k:i-1) = a(i:j)
end subroutine foo

	Thomas

Sende          fortran/ChangeLog
Sende          fortran/dependency.c
Sende          testsuite/ChangeLog
Hinzufügen     testsuite/gfortran.dg/dependency_30.f90
Übertrage Daten ....
Revision 162848 übertragen.

Patch

Index: dependency.c
===================================================================
--- dependency.c	(Revision 162829)
+++ dependency.c	(Arbeitskopie)
@@ -180,7 +180,45 @@  gfc_dep_compare_expr (gfc_expr *e1, gfc_expr *e2)
   gfc_actual_arglist *args1;
   gfc_actual_arglist *args2;
   int i;
+  gfc_expr *n1, *n2;
 
+  n1 = NULL;
+  n2 = NULL;
+
+  /* Remove any integer conversion functions to larger types.  */
+  if (e1->expr_type == EXPR_FUNCTION && e1->value.function.isym
+      && e1->value.function.isym->id == GFC_ISYM_CONVERSION
+      && e1->ts.type == BT_INTEGER)
+    {
+      args1 = e1->value.function.actual;
+      if (args1->expr->ts.type == BT_INTEGER
+	  && e1->ts.kind > args1->expr->ts.kind)
+	n1 = args1->expr;
+    }
+
+  if (e2->expr_type == EXPR_FUNCTION && e2->value.function.isym
+      && e2->value.function.isym->id == GFC_ISYM_CONVERSION
+      && e2->ts.type == BT_INTEGER)
+    {
+      args2 = e2->value.function.actual;
+      if (args2->expr->ts.type == BT_INTEGER
+	  && e2->ts.kind > args2->expr->ts.kind)
+	n2 = args2->expr;
+    }
+
+  if (n1 != NULL)
+    {
+      if (n2 != NULL)
+	return gfc_dep_compare_expr (n1, n2);
+      else
+	return gfc_dep_compare_expr (n1, e2);
+    }
+  else
+    {
+      if (n2 != NULL)
+	return gfc_dep_compare_expr (e1, n2);
+    }
+  
   if (e1->expr_type == EXPR_OP
       && (e1->value.op.op == INTRINSIC_UPLUS
 	  || e1->value.op.op == INTRINSIC_PARENTHESES))
@@ -328,20 +366,6 @@  gfc_dep_compare_expr (gfc_expr *e1, gfc_expr *e2)
 	 argument lists.  */
       switch (e1->value.function.isym->id)
 	{
-	case GFC_ISYM_CONVERSION:
-	  /* Handle integer extensions specially, as __convert_i4_i8
-	     is not only "constant" but also "unary" and "increasing".  */
-	  if (args1 && !args1->next
-	      && args2 && !args2->next
-	      && e1->ts.type == BT_INTEGER
-	      && args1->expr->ts.type == BT_INTEGER
-	      && e1->ts.kind > args1->expr->ts.kind
-	      && e2->ts.type == e1->ts.type
-	      && e2->ts.kind == e1->ts.kind
-	      && args2->expr->ts.type == args1->expr->ts.type
-	      && args2->expr->ts.kind == args2->expr->ts.kind)
-	    return gfc_dep_compare_expr (args1->expr, args2->expr);
-	  break;
 
 	case GFC_ISYM_REAL:
 	case GFC_ISYM_LOGICAL: