Re: [PATCH] libgompd: Add OMPD support and global ICV functions

[Jakub Jelinek <jakub@redhat.com>]

On Mon, May 16, 2022 at 07:35:17PM +0200, Mohamed Atef via Gcc-patches wrote:
> libgomp/ChangeLog
> 
> 2022-05-15 Mohamed Atef <mohamedatef1698@gmail.com>
> 
> *config/darwin/plugin-suffix.h (SONAME_SUFFIX): Remove ()s.
> *config/hpux/plugin-suffix.h (SONAME_SUFFIX): Remove ()s.
> *config/posix/plugin-suffix.h (SONAME_SUFFIX): Remove ()s.
> *configure: Regenerate.
> *Makefile.am (toolexeclib_LTLIBRARIES): Add libgompd.la.
> (libgompd_la_LDFLAGS, libgompd_la_DEPENDENCIES,
> libgompd_la_LINK,libgompd_la_SOURCES, libgompd_version_dep,
> libgompd_version_script, libgompd.ver-sun, libgompd.ver,
> libgompd_version_info): New.
> *Makefile.in: Regenerate.
> *aclocal.m4: Regenerate.
> *env.c: Include ompd-support.h.
> (parse_debug): New function.
> (gompd_enabled): New Variable.
> (initialize_env): Call gompd_load.
> (initialize_env): Call parse_debug.
> *team.c: Include ompd-support.h.
> (gomp_team_start): Call ompd_bp_parallel_begin.
> (gomp_team_end): Call ompd_bp_parallel_end.
> (gomp_thread_start): Call ompd_bp_thread_start.
> *libgomp.map: ADD OMP_5.0.3 symbol versions.

Add rather than ADD

> *libgompd.map: New.
> *omp-tools.h.in: New.
> *ompd-types.h.in: New.
> *ompd-support.h: New.
> *ompd-support.c: New.
> *ompd-helper.h: New.
> *ompd-helper.c: New.
> *ompd-init.c: New.
> *ompd-icv.c: New.
> *configure.ac (AC_CONFIG_FILES): Add omp-tools.h and ompd-types.h.

Almost ok, minor comments below.
As I said earlier, as this is just partial implementation of the
OMPD, I think it would be better to commit it to a git branch but
already in the upstream repository, say devel/omp/ompd branch.

Do you have a FSF Copyright assignment on file or do you want to
submit this under DCO (https://gcc.gnu.org/dco.html)?  If the latter,
we need Signed-off-by line in your final patch mail and also in the
commit message.
The commit message should include a one line summary, then
empty line, then explanation what the patch is, followed by properly
formatted ChangeLog entry (the above is badly mangled by your mailer),
there should be 2 spaces around the name etc. and after the ChangeLog
entry the Signed-of-by line if you use DCO.
More details in https://gcc.gnu.org/gitwrite.html
In Authenticated access there is a link to a form to request write
access to the repository, please file it and write me as the sponsor.
Then follow the rest of gitwrite to e.g. do contrib/gcc-git-customization.sh
and then you can git gcc-verify to verify whether the ChangeLog entry in
your commit log is ok before you push it.

> --- /dev/null
> +++ b/libgomp/libgompd.map
> @@ -0,0 +1,23 @@
> +OMPD_5.1 {

Shouldn't this be OMPD_5.0 ?
The functions were already in OpenMP 5.0, weren't they?

> +  global:
> +    ompd_initialize;
> +    ompd_get_api_version;
> +    ompd_get_version_string;
> +    ompd_process_initialize;
> +    ompd_device_initialize;
> +    ompd_rel_address_space_handle;
> +    ompd_finalize;
> +    ompd_enumerate_icvs;
> +    ompd_get_icv_from_scope;
> +    ompd_get_icv_string_from_scope;
> +    ompd_get_thread_handle;
> +    ompd_get_thread_in_parallel;
> +    ompd_rel_thread_handle;
> +    ompd_thread_handle_compare;
> +    ompd_get_thread_id;
> +    ompd_get_device_from_thread;
> +    ompd_bp_thread_begin;
> +    ompd_bp_thread_end;

Why is ompd_bp_thread_{begin,end} here?  I thought they were
in libgomp.so.*, not in libgompd.so.*

> +  local:
> +    *;
> +};

> +  /* NO cuda for now.  */

We support other kinds of offloading, so better just say
  /* No offloading support for now.  */
or so.

> +  if (device == OMPD_DEVICE_KIND_HOST)
> +    {
> +      switch (icv_id)
> +	{
> +	  case gompd_icv_cancellation_var:
> +	    return
> +	      gompd_get_cancellation ((ompd_address_space_handle_t *) handle,
> +				      icv_value);
> +	  case gompd_icv_max_task_priority_var:
> +	    return gompd_get_max_task_priority ((ompd_address_space_handle_t *)
> +						handle, icv_value);
> +	  case gompd_icv_stacksize_var:
> +	    return gompd_get_stacksize ((ompd_address_space_handle_t *) handle,
> +					icv_value);

You use that overly long (ompd_address_space_handle_t *) handle in all the
spots and it causes various formatting issues.  Wouldn't it be better
to add some temporary variable above the switch
      ompd_address_space_handle_t *whatever
	= (ompd_address_space_handle_t *) handle;
(for some good choice of name) and then just use it instead of
(ompd_address_space_handle_t *) handle which would allow always writing
	    return gompd_whatever (whatever, icv_value);
and not the uglier return with call on the next line, or even worse ( on
a different line from the function name?

> +ompd_rc_t
> +ompd_finalize ()

Please use (void) instead of ()

> +void
> +gompd_load ()

Likewise.

> +void __attribute__ ((noipa))
> +ompd_dll_locations_valid ()

Likewise (and several times more).

	Jakub