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 <ahmedsayedmou...@gmail.com
> <ahmedsayedmou...@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
