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

[Bernhard Reutner-Fischer <rep.dot.nop@gmail.com>]

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,