| Message ID | 20211028235259.1411169-1-rep.dot.nop@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Fortran,1/2] Add uop/name helpers | expand |
On Fri, Oct 29, 2021 at 01:52:58AM +0200, Bernhard Reutner-Fischer wrote: > From: Bernhard Reutner-Fischer <aldot@gcc.gnu.org> > > Introduce a helper to construct a user operator from a name and the > reverse operation, i.e. a helper to construct a name from a user > operator. > > Cc: Jakub Jelinek <jakub@redhat.com> > > gcc/fortran/ChangeLog: > > 2017-10-29 Bernhard Reutner-Fischer <aldot@gcc.gnu.org> > > * gfortran.h (gfc_get_uop_from_name, gfc_get_name_from_uop): Declare. > * symbol.c (gfc_get_uop_from_name, gfc_get_name_from_uop): Define. > * module.c (load_omp_udrs): Use them. > --- > gcc/fortran/gfortran.h | 2 ++ > gcc/fortran/module.c | 21 +++------------------ > gcc/fortran/symbol.c | 21 +++++++++++++++++++++ > 3 files changed, 26 insertions(+), 18 deletions(-) > > diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h > index 9378b4b8a24..afe9f2354ee 100644 > --- a/gcc/fortran/gfortran.h > +++ b/gcc/fortran/gfortran.h > @@ -3399,6 +3399,8 @@ void gfc_delete_symtree (gfc_symtree **, const char *); > gfc_symtree *gfc_get_unique_symtree (gfc_namespace *); > gfc_user_op *gfc_get_uop (const char *); > gfc_user_op *gfc_find_uop (const char *, gfc_namespace *); > +const char *gfc_get_uop_from_name (const char*); > +const char *gfc_get_name_from_uop (const char*); Formatting, space between char and *. > --- a/gcc/fortran/symbol.c > +++ b/gcc/fortran/symbol.c > @@ -3044,6 +3044,27 @@ gfc_find_uop (const char *name, gfc_namespace *ns) > return (st == NULL) ? NULL : st->n.uop; > } > > +/* Given a name return a string usable as user operator name. */ > +const char * > +gfc_get_uop_from_name (const char* name) { Formatting, space before * rather than after it, { should go on next line. Similarly later. But most importantly, I really don't like these helpers at all, they unnecessarily allocate memory of the remaining duration of compilation, and the second one even uses heap for temporary. Can't you just fix the real bug and keep the code as it was otherwise (with XALLOCAVEC etc.)? And, there should be a testcase... Jakub
On Fri, 29 Oct 2021 13:13:52 +0200 Jakub Jelinek <jakub@redhat.com> wrote: > On Fri, Oct 29, 2021 at 01:52:58AM +0200, Bernhard Reutner-Fischer wrote: > > From: Bernhard Reutner-Fischer <aldot@gcc.gnu.org> > > > > Introduce a helper to construct a user operator from a name and the > > reverse operation, i.e. a helper to construct a name from a user > > operator. > > > > Cc: Jakub Jelinek <jakub@redhat.com> > > > > gcc/fortran/ChangeLog: > > > > 2017-10-29 Bernhard Reutner-Fischer <aldot@gcc.gnu.org> > > > > * gfortran.h (gfc_get_uop_from_name, gfc_get_name_from_uop): Declare. > > * symbol.c (gfc_get_uop_from_name, gfc_get_name_from_uop): Define. > > * module.c (load_omp_udrs): Use them. > > --- > > gcc/fortran/gfortran.h | 2 ++ > > gcc/fortran/module.c | 21 +++------------------ > > gcc/fortran/symbol.c | 21 +++++++++++++++++++++ > > 3 files changed, 26 insertions(+), 18 deletions(-) > > > > diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h > > index 9378b4b8a24..afe9f2354ee 100644 > > --- a/gcc/fortran/gfortran.h > > +++ b/gcc/fortran/gfortran.h > > @@ -3399,6 +3399,8 @@ void gfc_delete_symtree (gfc_symtree **, const char *); > > gfc_symtree *gfc_get_unique_symtree (gfc_namespace *); > > gfc_user_op *gfc_get_uop (const char *); > > gfc_user_op *gfc_find_uop (const char *, gfc_namespace *); > > +const char *gfc_get_uop_from_name (const char*); > > +const char *gfc_get_name_from_uop (const char*); > > Formatting, space between char and *. > > > --- a/gcc/fortran/symbol.c > > +++ b/gcc/fortran/symbol.c > > @@ -3044,6 +3044,27 @@ gfc_find_uop (const char *name, gfc_namespace *ns) > > return (st == NULL) ? NULL : st->n.uop; > > } > > > > +/* Given a name return a string usable as user operator name. */ > > +const char * > > +gfc_get_uop_from_name (const char* name) { > > Formatting, space before * rather than after it, { should go on next line. > Similarly later. Fixed the formatting. Sorry for my sloppiness.. > > But most importantly, I really don't like these helpers at all, they > unnecessarily allocate memory of the remaining duration of compilation, > and the second one even uses heap for temporary. Where do they allocate memory that remains during the rest of compilation? If we end up emitting the thing, then we will have it put into the stringpool anyway, so how's that bad? I did delete the allocated buffer after ht_lookup_with_hash copied the content so the temporary buffer in gfc_get_name_from_uop does not leak AFAICS. I can easily switch the second one to use XALLOCAVEC if you'd accept that? Ok? > Can't you just fix the real bug and keep the code as it was otherwise > (with XALLOCAVEC etc.)? > And, there should be a testcase... I tried to construct a testcase yesterday but failed. I took udr10.f90 and experimented with not using a derived type (because DERIVED || CLASS bypasses the failure to lookup st). I tried to move the module out to its own source to no avail and gave up late at night. Unrelated note: One thing that looked odd to my untrained eyes was in e.g. udr10.f90 where you write: !$omp parallel do reduction(+:j) reduction(.localadd.:k) do i = 1, 100 j = j .localadd. dl(i) k = k + dl(i * 2) which may of course be correct (even if + would be implemented by e.g. a twice_i procedure (and not addme like the user operator) but i'd have assumed the reduction oper to match the target var in the region, like !$omp parallel do reduction(.localadd.:j) reduction(+:k) But i admittedly know nothing about openmp syntax so it's certainly fine as written? PS: you have at least declare-simd-3.f90:! { dg-do compile { target { lp64 && { ! lp64 } } } } declare-target-2.f90:! { dg-do compile { target { lp64 && { ! lp64 } } } } and i think later on ! { dg-do compile { target skip-all-targets } } was added, presumably for this very purpose. thanks,
On Fri, 29 Oct 2021 19:15:13 +0200 Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote: > > But most importantly, I really don't like these helpers at all, they > > unnecessarily allocate memory of the remaining duration of compilation, > > and the second one even uses heap for temporary. > I can easily switch the second one to use XALLOCAVEC if you'd accept > that? Ok? const char * gfc_get_name_from_uop (const char *name) { gcc_assert (name[0] == '.'); const size_t len = strlen (name) - 1; gcc_assert (len > 1); gcc_assert (name[len] == '.'); const char *ret = gfc_get_string ("%.*s", len - 1, name + 1); return ret; } if that's deemed portable enough nowadays? There seem to be preexisting users in the tree so should be ok. We can make these checking asserts or remove them altogether of course. Would that be acceptable? thanks, > > > Can't you just fix the real bug and keep the code as it was otherwise > > (with XALLOCAVEC etc.)? > > And, there should be a testcase... > > I tried to construct a testcase yesterday but failed. > I took udr10.f90 and experimented with not using a derived type > (because DERIVED || CLASS bypasses the failure to lookup st). > I tried to move the module out to its own source to no avail and gave up > late at night. > > Unrelated note: > One thing that looked odd to my untrained eyes was in e.g. udr10.f90 > where you write: > !$omp parallel do reduction(+:j) reduction(.localadd.:k) > do i = 1, 100 > j = j .localadd. dl(i) > k = k + dl(i * 2) > > which may of course be correct (even if + would be implemented by e.g. > a twice_i procedure (and not addme like the user operator) but i'd have > assumed the reduction oper to match the target var in the region, like > !$omp parallel do reduction(.localadd.:j) reduction(+:k) > But i admittedly know nothing about openmp syntax so it's certainly fine > as written? > > PS: you have at least > declare-simd-3.f90:! { dg-do compile { target { lp64 && { ! lp64 } } } } > declare-target-2.f90:! { dg-do compile { target { lp64 && { ! lp64 } } } } > > and i think later on > ! { dg-do compile { target skip-all-targets } } > was added, presumably for this very purpose. > > thanks,
diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h index 9378b4b8a24..afe9f2354ee 100644 --- a/gcc/fortran/gfortran.h +++ b/gcc/fortran/gfortran.h @@ -3399,6 +3399,8 @@ void gfc_delete_symtree (gfc_symtree **, const char *); gfc_symtree *gfc_get_unique_symtree (gfc_namespace *); gfc_user_op *gfc_get_uop (const char *); gfc_user_op *gfc_find_uop (const char *, gfc_namespace *); +const char *gfc_get_uop_from_name (const char*); +const char *gfc_get_name_from_uop (const char*); void gfc_free_symbol (gfc_symbol *&); void gfc_release_symbol (gfc_symbol *&); gfc_symbol *gfc_new_symbol (const char *, gfc_namespace *); diff --git a/gcc/fortran/module.c b/gcc/fortran/module.c index 7b98ba539d6..1328414e4f7 100644 --- a/gcc/fortran/module.c +++ b/gcc/fortran/module.c @@ -5027,7 +5027,7 @@ load_omp_udrs (void) while (peek_atom () != ATOM_RPAREN) { const char *name = NULL, *newname; - char *altname; + const char *altname = NULL; gfc_typespec ts; gfc_symtree *st; gfc_omp_reduction_op rop = OMP_REDUCTION_USER; @@ -5054,15 +5054,8 @@ load_omp_udrs (void) else if (strcmp (p, ".neqv.") == 0) rop = OMP_REDUCTION_NEQV; } - altname = NULL; if (rop == OMP_REDUCTION_USER && name[0] == '.') - { - size_t len = strlen (name + 1); - altname = XALLOCAVEC (char, len); - gcc_assert (name[len] == '.'); - memcpy (altname, name + 1, len - 1); - altname[len - 1] = '\0'; - } + altname = gfc_get_name_from_uop (name); newname = name; if (rop == OMP_REDUCTION_USER) newname = find_use_name (altname ? altname : name, !!altname); @@ -5074,15 +5067,7 @@ load_omp_udrs (void) continue; } if (altname && newname != altname) - { - size_t len = strlen (newname); - altname = XALLOCAVEC (char, len + 3); - altname[0] = '.'; - memcpy (altname + 1, newname, len); - altname[len + 1] = '.'; - altname[len + 2] = '\0'; - name = gfc_get_string ("%s", altname); - } + name = altname = gfc_get_uop_from_name (newname); st = gfc_find_symtree (gfc_current_ns->omp_udr_root, name); gfc_omp_udr *udr = gfc_omp_udr_find (st, &ts); if (udr) diff --git a/gcc/fortran/symbol.c b/gcc/fortran/symbol.c index 289d85734bd..900ab49c478 100644 --- a/gcc/fortran/symbol.c +++ b/gcc/fortran/symbol.c @@ -3044,6 +3044,27 @@ gfc_find_uop (const char *name, gfc_namespace *ns) return (st == NULL) ? NULL : st->n.uop; } +/* Given a name return a string usable as user operator name. */ +const char * +gfc_get_uop_from_name (const char* name) { + gcc_assert (name[0] != '.'); + return gfc_get_string (".%s.", name); +} + +/* Given a user operator name return a string usable as name. */ +const char * +gfc_get_name_from_uop (const char* name) { + gcc_assert (name[0] == '.'); + const size_t len = strlen (name) - 1; + gcc_assert (len > 1); + gcc_assert (name[len] == '.'); + char *buffer = XNEWVEC (char, len); + memcpy (buffer, name + 1, len - 1); + buffer[len - 1] = '\0'; + const char *ret = gfc_get_string ("%s", buffer); + XDELETEVEC (buffer); + return ret; +} /* Update a symbol's common_block field, and take care of the associated memory management. */
From: Bernhard Reutner-Fischer <aldot@gcc.gnu.org> Introduce a helper to construct a user operator from a name and the reverse operation, i.e. a helper to construct a name from a user operator. Cc: Jakub Jelinek <jakub@redhat.com> gcc/fortran/ChangeLog: 2017-10-29 Bernhard Reutner-Fischer <aldot@gcc.gnu.org> * gfortran.h (gfc_get_uop_from_name, gfc_get_name_from_uop): Declare. * symbol.c (gfc_get_uop_from_name, gfc_get_name_from_uop): Define. * module.c (load_omp_udrs): Use them. --- gcc/fortran/gfortran.h | 2 ++ gcc/fortran/module.c | 21 +++------------------ gcc/fortran/symbol.c | 21 +++++++++++++++++++++ 3 files changed, 26 insertions(+), 18 deletions(-)