Re: [PATCH] libgompd: add OMPD support, libgompd initialization and global ICVs functions

[Jakub Jelinek <jakub@redhat.com>]

On Sun, Mar 20, 2022 at 11:33:10AM +0200, Mohamed Atef via Gcc-patches wrote:
> hello,
>    I know it's too much.
> we fixed the functions' names that are not part of the standard form ompd_
> * prefix to gompd_

Sorry for the delay, have been busy with GCC 12.  Now that it branched, I
can look at this again.  Will try to do timely reviews now that GCC 13 is in
stage1.

> > 2022-03-15  Mohamed Atef  <mohamedatef1698@gmail.com>

Note, you'll need to update the ChangeLog entry for the changes in the
patch.

> --- a/libgomp/Makefile.am
> +++ b/libgomp/Makefile.am
> @@ -32,13 +32,21 @@ libgomp.ver: $(top_srcdir)/libgomp.map
>  	$(EGREP) -v '#(#| |$$)' $< | \
>  	  $(PREPROCESS) -P -include config.h - > $@ || (rm -f $@ ; exit 1)
>  
> +libgompd.ver: $(top_srcdir)/libgompd.map
> +	$(EGREP) -v '#(#| |$$)' $< | \
> +		$(PREPROCESS) -P -include config.h - > $@ || (rm -f $@ ; exit 1)

Why the different indentation from the entry you've copied it from?
Better stay consistent.

> +libgompd.ver-sun : libgompd.ver \
> +		$(top_srcdir)/../contrib/make_sunver.pl \
> +		$(libgompd_la_OBJECTS) $(libgompd_la_LIBADD)
> +	perl $(top_srcdir)/../contrib/make_sunver.pl \
> +		libgompd.ver \
> +		$(libgompd_la_OBJECTS:%.lo=.libs/%.o) \
> +		`echo $(libgompd_la_LIBADD) | \
> +			sed 's,/\([^/.]*\)\.la,/.libs/\1.a,g'` \
> +		> $@ || (rm -f $@ ; exit 1)

Likewise.

> --- a/libgomp/env.c
> +++ b/libgomp/env.c
> @@ -33,6 +33,7 @@
>  #ifndef LIBGOMP_OFFLOADED_ONLY
>  #include "libgomp_f.h"
>  #include "oacc-int.h"
> +#include "ompd-support.h"
>  #include <ctype.h>
>  #include <stdlib.h>
>  #include <stdio.h>
> @@ -1483,6 +1484,7 @@ initialize_env (void)
>  	= thread_limit_var > INT_MAX ? UINT_MAX : thread_limit_var;
>      }
>    parse_int_secure ("GOMP_DEBUG", &gomp_debug_var, true);
> +  gompd_load ();

See below.

> +#ifndef _OMP_TOOLS_H
> +#define _OMP_TOOLS_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +extern const char **ompd_dll_locations;
> +
> +#ifdef __ELF__
> +#define ompd_dll_locations_valid() \
> +  __asm__ __volatile__ (".globl ompd_dll_locations_valid\n\t" \
> +			"ompd_dll_locations_valid:")
> +#else
> +extern void ompd_dll_locations_valid (void);
> +#endif /* __ELF__ */

In the public omp-tools.h, ompd_dll_locations_valid should be declared
just as
extern void ompd_dll_locations_valid (void);
(well,
extern void ompd_dll_locations_valid (void) __GOMPD_NOTHROW;
and there should be
#ifdef __cplusplus
extern "C" {
# define __GOMPD_NOTHROW throw ()
#else
# define __GOMPD_NOTHROW __attribute__((__nothrow__))
#endif
earlier.  The fact that we want the former macro if possible is an
implementation detail we shouldn't expose to users in a public header.
So that #define belongs to some non-public header, ideally using some
helper macro because you use it many times later.

> +ompd_rc_t ompd_initialize (ompd_word_t, const ompd_callbacks_t *);

Similarly add __GOMPD_NOTHROW to this and other declarations.

> +  if (current + 1 >= gompd_last_icv_var || next_id == NULL
> +    || next_icv_name == NULL || next_scope == NULL || more == NULL)

The general formatting rule is if all the whole if/while condition
fits on one line, use just one line, otherwise put each ||/&& part
on a separate line.  So
  if (whatever || whatever2 || whatever3)
if short or
  if (whatever
      || whatever2
      || whatever3
      || whatever4
      || whatever5)
if too long.

> +void
> +gompd_load ()
> +{
> +  const char *ompd_env_var = getenv ("OMP_DEBUG");
> +  if (ompd_env_var == NULL || strcmp (ompd_env_var, "enabled"))
> +    return;

If you look at other similar env var handling, you can notice that
we accept leading and/or trailing whitespace and use case insensitive
comparisons for strings.  See e.g. env.c (parse_target_offload).
IMHO the OMP_DEBUG env var parsing should be done in env.c too next
to other env vars in a similar way how other vars are handled and
gompd_load only called if OMPD is enabled.

> +#ifndef __ELF__
> +/* Dummy functions. they shoud not be optimized.  */
> +
> +void __attribute__ ((noinline))

noinline isn't strong enough, it should use noipa attribute
here and in the similar spots.

> +
> +void gompd_load ();

We want internal functions like this (note, (void) rather than (),
this is C) to be marked hidden if possible, libgomp.h uses
#ifdef HAVE_ATTRIBUTE_VISIBILITY
# pragma GCC visibility push(hidden)
#endif
...
#ifdef HAVE_ATTRIBUTE_VISIBILITY
# pragma GCC visibility pop
#endif

	Jakub