| Message ID | 1280853926.25237.8.camel@linux-fd1f.site |
|---|---|
| State | New |
| Headers | show |
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
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. >
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.
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: