Re: [patch] libgompd: Add thread handles

[Jakub Jelinek <jakub@redhat.com>]

On Mon, Jul 04, 2022 at 10:34:03PM +0200, Ahmed Sayed Mousse wrote:
> *This patch is the initial implementation of OpenMP-API specs book section
> **20.5.5 with title "Thread Handles".*

Sorry for the delay, have been on vacation.

> *I have fixed the first version after revising the notes on it.*
> 
> *libgomp/ChangeLog
> 
> 2022-07-01  Ahmed Sayed  <ahmedsayedmousse@gmail.com
> <ahmedsayedmousse@gmail.com>>
> *
> 
> ** Makefile.am (libgompd_la_SOURCES): Add ompd-threads.c.*
> 
> ** Makefile.in: Regenerate.*
> 
> ** team.c ( gomp_free_thread ): Called ompd_bp_thread_end ().*
> 
> ** ompd-support.c ( gompd_thread_initial_tls_bias ): New Variable.*
> 
> *  	  (gompd_load): Initialize gompd_thread_initial_tls_bias.*
> 
> ** ompd-threads.c: New File.*

The ChangeLog formatting is wrong, so wouldn't go through the commit
hook checking.  Unclear what part of it is just a fault of your mailer
setting and what is really wrong.  But
There should be just > after gmail.com, not another email address,
all the non-empty lines should be indented by a single tab,
there shouldn't be an extra * at the start of end of lines.
There shouldn't be empty lines in between the different changes, just
between the date/name/email line and the ret.
There shouldn't be spaces after ( or before ).
Instead of Called ompd_bp_thread_end (). say just Call ompd_bp_thread_end.
New variable. rather than New Variable.
The line with (gompd_load) is weirdly extra indented.
New file. rather than New File.

> diff --git a/libgomp/Makefile.am b/libgomp/Makefile.am
> index 6d913a93e7f..23f5bede1bf 100644
> --- a/libgomp/Makefile.am
> +++ b/libgomp/Makefile.am
> @@ -94,7 +94,7 @@ libgomp_la_SOURCES = alloc.c atomic.c barrier.c critical.c env.c error.c \
>  	priority_queue.c affinity-fmt.c teams.c allocator.c oacc-profiling.c \
>  	oacc-target.c ompd-support.c
>  
> -libgompd_la_SOURCES = ompd-init.c ompd-helper.c ompd-icv.c
> +libgompd_la_SOURCES = ompd-init.c ompd-helper.c ompd-icv.c ompd-threads.c
>  
>  include $(top_srcdir)/plugin/Makefrag.am
>  

You've just changed libgompd_la_SOURCES but there are many changes
in the generated file, that means either you didn't use the right
libtool version (1.15.1) or something else wrong is happening.

> diff --git a/libgomp/Makefile.in b/libgomp/Makefile.in
> index 40f896b5f03..8bbc46cca25 100644
> --- a/libgomp/Makefile.in
> +++ b/libgomp/Makefile.in
> @@ -133,21 +133,8 @@ target_triplet = @target@
>  @USE_FORTRAN_TRUE@am__append_7 = openacc.f90
>  subdir = .
>  ACLOCAL_M4 = $(top_srcdir)/aclocal.m4
> -am__aclocal_m4_deps = $(top_srcdir)/../config/acx.m4 \
> -	$(top_srcdir)/../config/ax_count_cpus.m4 \
> -	$(top_srcdir)/../config/depstand.m4 \
> -	$(top_srcdir)/../config/enable.m4 \
> -	$(top_srcdir)/../config/futex.m4 \
> -	$(top_srcdir)/../config/lead-dot.m4 \
> -	$(top_srcdir)/../config/lthostflags.m4 \
> -	$(top_srcdir)/../config/multi.m4 \
> -	$(top_srcdir)/../config/override.m4 \
> -	$(top_srcdir)/../config/tls.m4 \
> -	$(top_srcdir)/../config/toolexeclibdir.m4 \
> -	$(top_srcdir)/../ltoptions.m4 $(top_srcdir)/../ltsugar.m4 \
> -	$(top_srcdir)/../ltversion.m4 $(top_srcdir)/../lt~obsolete.m4 \
> -	$(top_srcdir)/acinclude.m4 $(top_srcdir)/../libtool.m4 \
> -	$(top_srcdir)/../config/cet.m4 \
> +am__aclocal_m4_deps = $(top_srcdir)/acinclude.m4 \
> +	$(top_srcdir)/../libtool.m4 $(top_srcdir)/../config/cet.m4 \
>  	$(top_srcdir)/plugin/configfrag.ac $(top_srcdir)/configure.ac

The above certainly shouldn't be changed.

>  am__configure_deps = $(am__aclocal_m4_deps) $(CONFIGURE_DEPENDENCIES) \
>  	$(ACLOCAL_M4)
> @@ -233,7 +220,8 @@ am_libgomp_la_OBJECTS = alloc.lo atomic.lo barrier.lo critical.lo \
>  	affinity-fmt.lo teams.lo allocator.lo oacc-profiling.lo \
>  	oacc-target.lo ompd-support.lo $(am__objects_1)
>  libgomp_la_OBJECTS = $(am_libgomp_la_OBJECTS)
> -am_libgompd_la_OBJECTS = ompd-init.lo ompd-helper.lo ompd-icv.lo
> +am_libgompd_la_OBJECTS = ompd-init.lo ompd-helper.lo ompd-icv.lo \
> +	ompd-threads.lo
>  libgompd_la_OBJECTS = $(am_libgompd_la_OBJECTS)
>  AM_V_P = $(am__v_P_@AM_V@)
>  am__v_P_ = $(am__v_P_@AM_DEFAULT_V@)

The above yes.

> @@ -485,7 +473,6 @@ dvidir = @dvidir@
>  enable_shared = @enable_shared@
>  enable_static = @enable_static@
>  exec_prefix = @exec_prefix@
> -get_gcc_base_ver = @get_gcc_base_ver@
>  host = @host@
>  host_alias = @host_alias@
>  host_cpu = @host_cpu@
> @@ -501,10 +488,8 @@ libtool_VERSION = @libtool_VERSION@
>  link_gomp = @link_gomp@
>  localedir = @localedir@
>  localstatedir = @localstatedir@
> -lt_host_flags = @lt_host_flags@
>  mandir = @mandir@
>  mkdir_p = @mkdir_p@
> -multi_basedir = @multi_basedir@
>  offload_additional_lib_paths = @offload_additional_lib_paths@
>  offload_additional_options = @offload_additional_options@
>  offload_plugins = @offload_plugins@
> @@ -514,6 +499,7 @@ pdfdir = @pdfdir@
>  prefix = @prefix@
>  program_transform_name = @program_transform_name@
>  psdir = @psdir@
> +runstatedir = @runstatedir@
>  sbindir = @sbindir@
>  sharedstatedir = @sharedstatedir@
>  srcdir = @srcdir@

The above shouldn't be changed.

> @@ -583,7 +569,7 @@ libgomp_la_SOURCES = alloc.c atomic.c barrier.c critical.c env.c \
>  	oacc-async.c oacc-plugin.c oacc-cuda.c priority_queue.c \
>  	affinity-fmt.c teams.c allocator.c oacc-profiling.c \
>  	oacc-target.c ompd-support.c $(am__append_7)
> -libgompd_la_SOURCES = ompd-init.c ompd-helper.c ompd-icv.c
> +libgompd_la_SOURCES = ompd-init.c ompd-helper.c ompd-icv.c ompd-threads.c
>  
>  # Nvidia PTX OpenACC plugin.
>  @PLUGIN_NVPTX_TRUE@libgomp_plugin_nvptx_version_info = -version-info $(libtool_VERSION)
> @@ -801,6 +787,7 @@ distclean-compile:
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/ompd-icv.Plo@am__quote@
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/ompd-init.Plo@am__quote@
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/ompd-support.Plo@am__quote@
> +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/ompd-threads.Plo@am__quote@
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/ordered.Plo@am__quote@
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/parallel.Plo@am__quote@
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/priority_queue.Plo@am__quote@

The above should.

> --- a/libgomp/aclocal.m4
> +++ b/libgomp/aclocal.m4

This shouldn't change at all.
> --- a/libgomp/configure
> +++ b/libgomp/configure

Neither should this.

> --- a/libgomp/ompd-support.c
> +++ b/libgomp/ompd-support.c
> @@ -33,6 +33,8 @@ const unsigned short gompd_sizeof_gomp_thread_handle
>    __attribute__ ((used)) OMPD_SECTION = 0;
>  #endif
>  
> +unsigned long gompd_thread_initial_tls_bias __attribute__ ((used));
> +
>  /* Get offset of the member m in struct t.  */
>  #define gompd_get_offset(t, m) \
>    const unsigned short gompd_access_##t##_##m __attribute__ ((used)) \
> @@ -67,6 +69,9 @@ gompd_load (void)
>    gompd_state |= OMPD_ENABLED;
>    ompd_dll_locations = &ompd_dll_locations_array[0];
>    ompd_dll_locations_valid ();
> +
> +  gompd_thread_initial_tls_bias = (unsigned long) ((char *) &gomp_tls_data
> +  						   - (char *) pthread_self ());

This should be done only when GOMP_NEEDS_THREAD_HANDLE is not defined.
Otherwise gompd_thread_initial_tls_bias should be initialized to some magic
value (that isn't otherwise possible) that ompd-threads.c will handle as
request not to use the tls bias and instead read struct gomp_thread's 
handle member.
Looking at your patch later, you already use sizeof the handle == 0 as
sign of that, so just wrap the above store in
#if defined(LIBGOMP_USE_PTHREADS) && !defined(GOMP_NEEDS_THREAD_HANDLE)

> --- /dev/null
> +++ b/libgomp/ompd-threads.c
> @@ -0,0 +1,216 @@
> +#include "ompd-helper.h"
> +
> +ompd_rc_t
> +ompd_get_thread_in_parallel (ompd_parallel_handle_t *parallel_handle,
> +			     int thread_num,
> +			     ompd_thread_handle_t **thread_handle)
> +{
> +
> +  if (parallel_handle == NULL)
> +    return ompd_rc_stale_handle;
> +  CHECK (parallel_handle->ah);
> +
> +  ompd_address_space_context_t *context = parallel_handle->ah->context;
> +  ompd_rc_t ret;
> +
> +  ompd_word_t team_size_var = 1;
> +  if (parallel_handle->th.address)
> +    gompd_get_team_size(parallel_handle, &team_size_var);

Space before (

> +/* The ompd_get_thread_handle function that maps a native thread to an
> +   OMPD thread handle.  */
> +
> +ompd_rc_t
> +ompd_get_thread_handle (ompd_address_space_handle_t *handle,
> +			ompd_thread_id_t kind, ompd_size_t sizeof_thread_id,
> +			const void *thread_id,
> +			ompd_thread_handle_t **thread_handle)
> +{
> +  CHECK (handle);
> +  if (kind != OMPD_THREAD_ID_PTHREAD)
> +    return ompd_rc_unsupported;
> +
> +  ompd_address_space_context_t *context = handle->context;
> +  ompd_thread_context_t *tcontext;
> +  ompd_rc_t ret;
> +
> +  ret = callbacks->get_thread_context_for_thread_id (context, kind,
> +						     sizeof_thread_id,
> +						     thread_id, &tcontext);
> +  CHECK_RET (ret);
> +
> +  ompd_size_t temp_symbol_size, symbol_size;
> +  ompd_address_t temp_symbol_addr, symbol_addr = {OMPD_SEGMENT_UNSPECIFIED, 0};
> +
> +  GET_VALUE (context, NULL, "gompd_sizeof_gomp_thread", symbol_size,
> +	     temp_symbol_size, target_sizes.sizeof_short, 1, ret,
> +	     temp_symbol_addr);
> +
> +  GET_VALUE (context, tcontext, "gomp_tls_data", symbol_addr.address,
> +	     temp_symbol_addr.address, symbol_size, 1, ret, symbol_addr);
> +
> +  ret = callbacks->alloc_memory (sizeof (ompd_thread_handle_t),
> +				 (void **) thread_handle);
> +
> +  CHECK_RET (ret);
> +
> +  (*thread_handle)->ah = handle;
> +  (*thread_handle)->th = symbol_addr;
> +  (*thread_handle)->thread_context = tcontext;
> +  return ret;
> +}
> +
> +
> +ompd_rc_t
> +ompd_rel_thread_handle (ompd_thread_handle_t *thread_handle)
> +{
> +  if (thread_handle == NULL)
> +    return ompd_rc_stale_handle;
> +
> +  ompd_rc_t ret;
> +  ret = callbacks->free_memory ((void *) thread_handle);
> +  if (ret != ompd_rc_ok)
> +  return ret;
> +
> +  return ompd_rc_ok;
> +}
> +
> +
> +/* return -1, 0 or 1 for thread_handle_1 <, == or > thread_handle_2.  */

Capital R, i.e. Return

> +ompd_rc_t
> +ompd_thread_handle_compare (ompd_thread_handle_t *thread_handle_1,
> +			    ompd_thread_handle_t *thread_handle_2,
> +			    int	*cmp_value )

No space before )
> +{
> +
> +  if (thread_handle_1 == NULL || thread_handle_2 == NULL)
> +    return ompd_rc_stale_handle;
> +  if (cmp_value == NULL)
> +    return ompd_rc_bad_input;
> +  if (thread_handle_1->ah->kind != thread_handle_2->ah->kind)
> +    return ompd_rc_bad_input;
> +
> +  *cmp_value = thread_handle_1->th.address - thread_handle_2->th.address;

This looks incorrect.  address is I believe ompd_addr_t, 64-bit unsigned
integer, you subtract 2 64-bit integers and store the difference into
int, usually 32-bit.  Whether that compares < 0, or > 0, or == 0 is a
lottery.
Furthermore, you document -1, 0, 1, not < 0, 0, > 0.

So, better do
  if (thread_handle_1->th.address < thread_handle_2->th.address)
    *cmp_value = -1;
  else if (thread_handle_1->th.address > thread_handle_2->th.address)
    *cmp_value = 1;
  else
    *cmp_value = 0;

> +  return ompd_rc_ok;
> +}
> +
> +
> +ompd_rc_t
> +ompd_get_thread_id (ompd_thread_handle_t *thread_handle, ompd_thread_id_t kind,
> +		    ompd_size_t sizeof_thread_id, void *thread_id)
> +{
> +  if (kind != OMPD_THREAD_ID_PTHREAD)
> +    return ompd_rc_unsupported;
> +  if (thread_id == NULL)
> +    return ompd_rc_bad_input;
> +  if (thread_handle == NULL)
> +    return ompd_rc_stale_handle;
> +
> +  CHECK (thread_handle->ah);
> +  ompd_address_space_context_t *context = thread_handle->ah->context;
> +
> +  ompd_rc_t ret;
> +  ompd_address_t taddr = thread_handle->th;
> +  ompd_address_t temp_symbol_addr, symbol_addr = {OMPD_SEGMENT_UNSPECIFIED, 0};
> +  ompd_size_t temp_symbol_size, symbol_size;
> +  ompd_word_t temp_offset, offset;
> +
> +  GET_VALUE (context, NULL, "gompd_sizeof_gomp_thread_handle", symbol_size,
> +	     temp_symbol_size, target_sizes.sizeof_short, 1, ret, symbol_addr);
> +
> +  if (symbol_size == 0)
> +    goto use_tls_bias;
> +
> +  if (sizeof_thread_id != symbol_size)
> +    return ompd_rc_bad_input;
> +
> +  GET_VALUE (context, NULL, "gompd_access_gomp_thread_handle", offset,
> +	     temp_offset, target_sizes.sizeof_short, 1, ret, symbol_addr);
> +  taddr.address += offset;
> +
> +  ret = callbacks->read_memory (context, NULL, &taddr, symbol_size, thread_id);
> +  return ret;
> +
> +use_tls_bias:

I don't see the need to use goto and label here, just do
  if (symbol_size == 0)
    {
      TLS bias handling
    }
  else
    {
      thread handle handling
    }
> +
> +  GET_VALUE (context, NULL, "gompd_thread_initial_tls_bias", offset, temp_offset,
> +	     target_sizes.sizeof_long, 1, ret, symbol_addr);
> +
> +  ret = callbacks->symbol_addr_lookup (context, NULL,"gomp_tls_data",
> +				       &symbol_addr, NULL);
> +  ret = callbacks->device_to_host (context, &temp_symbol_addr.address,
> +				   target_sizes.sizeof_long_long, 1,
> +				   &symbol_addr.address);
> +  CHECK_RET (ret);
> +
> +  taddr.address = symbol_addr.address + offset;
> +  ret = callbacks->read_memory (context, NULL, &taddr,
> +				target_sizes.sizeof_long_long, thread_id);
> +  return ret;
> +}
> +
> +
> +/* OMPD doesn't support GPUs for now.  */
> +ompd_rc_t ompd_get_device_from_thread (ompd_thread_handle_t *thread_handle,
> +				       ompd_address_space_handle_t **device)
> +{
> +  if (thread_handle == NULL)
> +    return ompd_rc_stale_handle;
> +  return ompd_rc_unsupported;
> +}
> diff --git a/libgomp/team.c b/libgomp/team.c
> index d53246961b7..8e18fd6af63 100644
> --- a/libgomp/team.c
> +++ b/libgomp/team.c
> @@ -77,6 +77,7 @@ gomp_thread_start (void *xdata)
>    void *local_data;
>  
>    ompd_bp_thread_begin ();
> +
>  #if defined HAVE_TLS || defined USE_EMUTLS
>    thr = &gomp_tls_data;
>  #else
> @@ -313,6 +314,9 @@ gomp_free_thread (void *arg __attribute__((unused)))
>        gomp_end_task ();
>        free (task);
>      }
> +
> +  ompd_bp_thread_end ();
> +

No need for the empty line after the call/macro.
>  }
>  
>  /* Launch a team.  */
> diff --git a/libgomp/testsuite/Makefile.in b/libgomp/testsuite/Makefile.in
> index 048844f0a40..76cd09b0faf 100644
> --- a/libgomp/testsuite/Makefile.in
> +++ b/libgomp/testsuite/Makefile.in

This file shouldn't be changed at all.

	Jakub