| Message ID | 4d45b5da-5e28-7d1f-84ac-7aacbbc1733f@netcologne.de |
|---|---|
| State | New |
| Headers | show |
Hello, Le 16/07/2016 à 15:38, Thomas Koenig a écrit : > Hello world, > > this fixes PR 71902. The recent fix for PR 71783 introduced a > performance and code size regression a string temporary was created for > the test case when it was not actually needed. > > I also took the opportunity of renaming the misnomed temporary > variable. > > Regression-tested. > > OK for trunk? > > Do we actually want to backport this? Technically, it is a regression, > but people are not likely to notice much. > It is not an ICE, neither a code correctness issue as far as I can see, so I would rather not backport. comments below. > p3.diff > > Index: dependency.c > =================================================================== > --- dependency.c (Revision 238223) > +++ dependency.c (Arbeitskopie) > @@ -54,6 +54,8 @@ enum gfc_dependency > static gfc_dependency check_section_vs_section (gfc_array_ref *, > gfc_array_ref *, int); > > +static gfc_dependency dep_ref (gfc_ref *, gfc_ref *, gfc_reverse *); > + > /* Returns 1 if the expr is an integer constant value 1, 0 if it is not or > def if the value could not be determined. */ > > @@ -1316,14 +1318,34 @@ gfc_check_dependency (gfc_expr *expr1, gfc_expr *e > return 0; > } > > - if (identical) > - return 1; > - > /* Identical and disjoint ranges return 0, > overlapping ranges return 1. */ > if (expr1->ref && expr2->ref) > - return gfc_dep_resolver (expr1->ref, expr2->ref, NULL); > + { > + gfc_dependency dep; > + dep = dep_ref (expr1->ref, expr2->ref, NULL); > + switch (dep) > + { > + case GFC_DEP_EQUAL: > + return identical; > > + case GFC_DEP_FORWARD: > + return 0; > + This is doubtfull, but not worse than before I guess. > + case GFC_DEP_BACKWARD: > + return 1; > + > + case GFC_DEP_OVERLAP: > + return 1; > + > + case GFC_DEP_NODEP: > + return 0; > + > + case GFC_DEP_ERROR: > + return 0; Can we put a gcc_unreachable here instead? > + } > + } > + > return 1; > > case EXPR_FUNCTION: > @@ -2052,11 +2074,44 @@ ref_same_as_full_array (gfc_ref *full_ref, gfc_ref > 2 : array references are overlapping but reversal of one or > more dimensions will clear the dependency. > 1 : array references are overlapping. > - 0 : array references are identical or not overlapping. */ > + 0 : array references are identical or can be handled in a forward loop. */ > > int > gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gfc_reverse *reverse) > { > + enum gfc_dependency dep; > + dep = dep_ref (lref, rref, reverse); > + switch (dep) > + { > + case GFC_DEP_EQUAL: > + return 0; > + > + case GFC_DEP_FORWARD: > + return 0; > + > + case GFC_DEP_BACKWARD: > + return 2; > + > + case GFC_DEP_OVERLAP: > + return 1; > + > + case GFC_DEP_NODEP: > + return 0; > + > + case GFC_DEP_ERROR: > + return 0; > + same here, make it unreachable please. OK with this and the other unreachable change above. Thanks, Mikael > + default: > + gcc_unreachable(); > + } > +} > + > +/* Compare two references, returning an enum gfc_dependency depending on the > + "worst" type of dependency found. */ > + > +static gfc_dependency > +dep_ref (gfc_ref *lref, gfc_ref *rref, gfc_reverse *reverse) > +{ > int n; > int m; > gfc_dependency fin_dep;
Hi Mikael, >> Do we actually want to backport this? Technically, it is a regression, >> but people are not likely to notice much. >> > It is not an ICE, neither a code correctness issue as far as I can see, > so I would rather not backport. Fine with me. >> >> + case GFC_DEP_FORWARD: >> + return 0; > This is doubtfull, but not worse than before I guess. 0 in this case means that you need no array temporary. This is fine. >> + case GFC_DEP_BACKWARD: >> + return 1; >> + >> + case GFC_DEP_OVERLAP: >> + return 1; >> + >> + case GFC_DEP_NODEP: >> + return 0; >> + >> + case GFC_DEP_ERROR: >> + return 0; > Can we put a gcc_unreachable here instead? Unfortunately not. The original code (before I lifted out the functionality) sometimes had GFC_DEP_ERROR at the end of the function, which was then removed by return fin_dep == GFC_DEP_OVERLAP; (In other words, if you put a gcc_unreachable there, you get regressions.). I can add a comment that this is intentional. >> + > same here, make it unreachable please. > > OK with this and the other unreachable change above. So, OK with a comment why this appears? Or should I simply rename GFC_DEP_ERROR to GFC_DEP_NODEPFOUND to make this a bit clearer? Regards Thomas
Hi Thomas, > So, OK with a comment why this appears? Or should I simply > rename GFC_DEP_ERROR to GFC_DEP_NODEPFOUND to make this a bit > clearer? I recommend the latter. Reporting an error should be done only when an error occurred, but no dependency detected does not feel like an error. Let's reserve GFC_DEP_ERROR for real error cases (that may occur in the future). With the latter the patch is ok for me. In fact, was I thinking about doing something similar to the gfc_dependendy routines. (Note, I have no reviewer priviliges, so this is just a vote). Regards, Andre
Le 17/07/2016 à 18:21, Thomas Koenig a écrit : > Hi Mikael, > >>> Do we actually want to backport this? Technically, it is a regression, >>> but people are not likely to notice much. >>> >> It is not an ICE, neither a code correctness issue as far as I can see, >> so I would rather not backport. > > Fine with me. > >>> >>> + case GFC_DEP_FORWARD: >>> + return 0; > >> This is doubtfull, but not worse than before I guess. > > 0 in this case means that you need no array temporary. This is fine. > >>> + case GFC_DEP_BACKWARD: >>> + return 1; >>> + >>> + case GFC_DEP_OVERLAP: >>> + return 1; >>> + >>> + case GFC_DEP_NODEP: >>> + return 0; >>> + >>> + case GFC_DEP_ERROR: >>> + return 0; >> Can we put a gcc_unreachable here instead? > > Unfortunately not. The original code (before I lifted out the > functionality) sometimes had GFC_DEP_ERROR at the end of the > function, which was then removed by > > return fin_dep == GFC_DEP_OVERLAP; > That is very strange, there is an assert just a few lines before, that fin_dep != GFC_DEP_ERROR. The only case I can see where GFC_DEP_ERROR could be returned after your change is the REF_SUBSTRING case, but then it wouldn't work either without substring...
Am 18.07.2016 um 20:58 schrieb Mikael Morin: >> Unfortunately not. The original code (before I lifted out the >> functionality) sometimes had GFC_DEP_ERROR at the end of the >> function, which was then removed by >> >> return fin_dep == GFC_DEP_OVERLAP; >> > That is very strange, there is an assert just a few lines before, that > fin_dep != GFC_DEP_ERROR. This is not the only return statement. For example, look at @@ -2215,7 +2268,7 @@ gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gf /* Exactly matching and forward overlapping ranges don't cause a dependency. */ if (fin_dep < GFC_DEP_BACKWARD) - return 0; + return fin_dep; A GFC_DEP_ERROR could 'escape' here. I didn't change the logic (at least not intentionally), I just made it more visible. Regards Thomas
Le 18/07/2016 à 22:20, Thomas Koenig a écrit : > Am 18.07.2016 um 20:58 schrieb Mikael Morin: > >>> Unfortunately not. The original code (before I lifted out the >>> functionality) sometimes had GFC_DEP_ERROR at the end of the >>> function, which was then removed by >>> >>> return fin_dep == GFC_DEP_OVERLAP; >>> >> That is very strange, there is an assert just a few lines before, that >> fin_dep != GFC_DEP_ERROR. > > This is not the only return statement. > > For example, look at > > @@ -2215,7 +2268,7 @@ gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gf > /* Exactly matching and forward overlapping ranges don't cause a > dependency. */ > if (fin_dep < GFC_DEP_BACKWARD) > - return 0; > + return fin_dep; > > A GFC_DEP_ERROR could 'escape' here. > Indeed, I missed that one. Then handle the GFC_DEP_ERROR here. Or initialize fin_dep with GFC_DEP_NODEP at the beginning, as you prefer. OK with either (and the unreachable assertions). Having an invalid enum value equals to zero helps diagnosing uninitialized values, so I prefer keeping the GFC_DEP_ERROR value separate from GFC_DEP_NODEP, GFC_DEP_NODEPFOUND, or any other case.
Index: dependency.c =================================================================== --- dependency.c (Revision 238223) +++ dependency.c (Arbeitskopie) @@ -54,6 +54,8 @@ enum gfc_dependency static gfc_dependency check_section_vs_section (gfc_array_ref *, gfc_array_ref *, int); +static gfc_dependency dep_ref (gfc_ref *, gfc_ref *, gfc_reverse *); + /* Returns 1 if the expr is an integer constant value 1, 0 if it is not or def if the value could not be determined. */ @@ -1316,14 +1318,34 @@ gfc_check_dependency (gfc_expr *expr1, gfc_expr *e return 0; } - if (identical) - return 1; - /* Identical and disjoint ranges return 0, overlapping ranges return 1. */ if (expr1->ref && expr2->ref) - return gfc_dep_resolver (expr1->ref, expr2->ref, NULL); + { + gfc_dependency dep; + dep = dep_ref (expr1->ref, expr2->ref, NULL); + switch (dep) + { + case GFC_DEP_EQUAL: + return identical; + case GFC_DEP_FORWARD: + return 0; + + case GFC_DEP_BACKWARD: + return 1; + + case GFC_DEP_OVERLAP: + return 1; + + case GFC_DEP_NODEP: + return 0; + + case GFC_DEP_ERROR: + return 0; + } + } + return 1; case EXPR_FUNCTION: @@ -2052,11 +2074,44 @@ ref_same_as_full_array (gfc_ref *full_ref, gfc_ref 2 : array references are overlapping but reversal of one or more dimensions will clear the dependency. 1 : array references are overlapping. - 0 : array references are identical or not overlapping. */ + 0 : array references are identical or can be handled in a forward loop. */ int gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gfc_reverse *reverse) { + enum gfc_dependency dep; + dep = dep_ref (lref, rref, reverse); + switch (dep) + { + case GFC_DEP_EQUAL: + return 0; + + case GFC_DEP_FORWARD: + return 0; + + case GFC_DEP_BACKWARD: + return 2; + + case GFC_DEP_OVERLAP: + return 1; + + case GFC_DEP_NODEP: + return 0; + + case GFC_DEP_ERROR: + return 0; + + default: + gcc_unreachable(); + } +} + +/* Compare two references, returning an enum gfc_dependency depending on the + "worst" type of dependency found. */ + +static gfc_dependency +dep_ref (gfc_ref *lref, gfc_ref *rref, gfc_reverse *reverse) +{ int n; int m; gfc_dependency fin_dep; @@ -2079,21 +2134,21 @@ gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gf /* The two ranges can't overlap if they are from different components. */ if (lref->u.c.component != rref->u.c.component) - return 0; + return GFC_DEP_NODEP; break; case REF_SUBSTRING: /* Substring overlaps are handled by the string assignment code if there is not an underlying dependency. */ - return (fin_dep == GFC_DEP_OVERLAP) ? 1 : 0; + return fin_dep; case REF_ARRAY: if (ref_same_as_full_array (lref, rref)) - return 0; + return GFC_DEP_EQUAL; if (ref_same_as_full_array (rref, lref)) - return 0; + return GFC_DEP_EQUAL; if (lref->u.ar.dimen != rref->u.ar.dimen) { @@ -2104,7 +2159,7 @@ gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gf fin_dep = gfc_full_array_ref_p (lref, NULL) ? GFC_DEP_EQUAL : GFC_DEP_OVERLAP; else - return 1; + return GFC_DEP_OVERLAP; break; } @@ -2148,7 +2203,7 @@ gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gf /* If any dimension doesn't overlap, we have no dependency. */ if (this_dep == GFC_DEP_NODEP) - return 0; + return GFC_DEP_NODEP; /* Now deal with the loop reversal logic: This only works on ranges and is activated by setting @@ -2215,7 +2270,7 @@ gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gf /* Exactly matching and forward overlapping ranges don't cause a dependency. */ if (fin_dep < GFC_DEP_BACKWARD) - return 0; + return fin_dep; /* Keep checking. We only have a dependency if subsequent references also overlap. */ @@ -2233,7 +2288,7 @@ gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gf /* Assume the worst if we nest to different depths. */ if (lref || rref) - return 1; + return GFC_DEP_OVERLAP; - return fin_dep == GFC_DEP_OVERLAP; + return fin_dep; } Index: frontend-passes.c =================================================================== --- frontend-passes.c (Revision 238223) +++ frontend-passes.c (Arbeitskopie) @@ -185,7 +185,7 @@ realloc_string_callback (gfc_code **c, int *walk_s current_code = c; inserted_block = NULL; changed_statement = NULL; - n = create_var (expr2, "trim"); + n = create_var (expr2, "realloc_string"); co->expr2 = n; return 0; }
Hello world, this fixes PR 71902. The recent fix for PR 71783 introduced a performance and code size regression a string temporary was created for the test case when it was not actually needed. I also took the opportunity of renaming the misnomed temporary variable. Regression-tested. OK for trunk? Do we actually want to backport this? Technically, it is a regression, but people are not likely to notice much. Regards Thomas 2016-07-16 Thomas Koenig <tkoenig@gcc.gnu.org> PR fortran/71902 * dependency.c (gfc_check_dependency): Use dep_ref. Handle case if identical is true and two array element references differ. (gfc_dep_resovler): Move most of the code to dep_ref. (dep_ref): New function. * frontend-passes.c (realloc_string_callback): Name temporary variable "realloc_string". 2016-07-16 Thomas Koenig <tkoenig@gcc.gnu.org> PR fortran/71902 * gfortran.dg/dependency_47.f90: New test.