From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id D77F33858424 for ; Fri, 29 Jul 2022 11:23:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D77F33858424 Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-183-fvWfeyeEONqu-VtGfCZkQA-1; Fri, 29 Jul 2022 07:23:03 -0400 X-MC-Unique: fvWfeyeEONqu-VtGfCZkQA-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 0E2B4803301; Fri, 29 Jul 2022 11:23:03 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.41]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A2B0818ECB; Fri, 29 Jul 2022 11:23:02 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 26TBMxOf234973 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Fri, 29 Jul 2022 13:23:00 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 26TBMxPQ234972; Fri, 29 Jul 2022 13:22:59 +0200 Date: Fri, 29 Jul 2022 13:22:59 +0200 From: Jakub Jelinek To: Ahmed Sayed Mousse Cc: gcc-patches@gcc.gnu.org Subject: Re: [patch] libgompd: Add thread handles Message-ID: Reply-To: Jakub Jelinek References: MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-10.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 Jul 2022 11:23:11 -0000 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 > > * > > ** 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