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 <mohamedatef1...@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
