[PATCH,Fortran 1/2] Add uop/name helpers

[PATCH,Fortran 1/2] Add uop/name helpers

[Bernhard Reutner-Fischer]

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*);
 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.  */
-- 
2.33.0

[PATCH,Fortran 2/2] Fix write_omp_udr for user-operator REDUCTIONs

[Bernhard Reutner-Fischer]

From: Bernhard Reutner-Fischer <aldot@gcc.gnu.org>

Due to a typo a user operator used in a reduction was not found in the
symtree so would have been written multiple times (in theory).

E.g. user operator ".add." was looked up as ".ad" instead of "add".

For gcc-11 branch and earlier one would
-         memcpy (name, udr->name, len - 1);
+         memcpy (name, udr->name + 1, len - 1);

but for gcc-12 we have an appropriate helper already.
Jakub, please take care of non-trunk branches if you want it fixed
there.

Cc: Jakub Jelinek <jakub@redhat.com>

gcc/fortran/ChangeLog:

2017-11-16  Bernhard Reutner-Fischer  <aldot@gcc.gnu.org>

	* module.c (write_omp_udr): Use gfc_get_name_from_uop.
---
 gcc/fortran/module.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/gcc/fortran/module.c b/gcc/fortran/module.c
index 1328414e4f7..90ab9e275f3 100644
--- a/gcc/fortran/module.c
+++ b/gcc/fortran/module.c
@@ -6021,12 +6021,8 @@ write_omp_udr (gfc_omp_udr *udr)
 	return;
       else
 	{
-	  gfc_symtree *st;
-	  size_t len = strlen (udr->name + 1);
-	  char *name = XALLOCAVEC (char, len);
-	  memcpy (name, udr->name, len - 1);
-	  name[len - 1] = '\0';
-	  st = gfc_find_symtree (gfc_current_ns->uop_root, name);
+	  const char *name = gfc_get_name_from_uop (udr->name);
+	  gfc_symtree *st = gfc_find_symtree (gfc_current_ns->uop_root, name);
 	  /* If corresponding user operator is private, don't write
 	     the UDR.  */
 	  if (st != NULL)
-- 
2.33.0

Re: [PATCH,Fortran 1/2] Add uop/name helpers

[Jakub Jelinek]

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

Re: [PATCH,Fortran 1/2] Add uop/name helpers

[Bernhard Reutner-Fischer]

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,

Re: [PATCH,Fortran 1/2] Add uop/name helpers

[Bernhard Reutner-Fischer]

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,