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.133.124]) by sourceware.org (Postfix) with ESMTPS id B2840382C5F9 for ; Thu, 16 Jun 2022 12:22:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B2840382C5F9 Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-574-rilqHyuPOqihv5GJFewlDQ-1; Thu, 16 Jun 2022 08:22:25 -0400 X-MC-Unique: rilqHyuPOqihv5GJFewlDQ-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id E81243802AC2; Thu, 16 Jun 2022 12:22:24 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.239]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 9DB4B1121314; Thu, 16 Jun 2022 12:22:24 +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 25GCMLlx1813670 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Thu, 16 Jun 2022 14:22:22 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 25GCMLkd1813669; Thu, 16 Jun 2022 14:22:21 +0200 Date: Thu, 16 Jun 2022 14:22:20 +0200 From: Jakub Jelinek To: Mohamed Atef Cc: gcc-patches@gcc.gnu.org, Mohamed Sayed , ahmedsayedmousse@gmail.com, youssef.magdy.775@gmail.com, aya.nashaat99@gmail.com Subject: Re: [PATCH] libgompd: Fix sizes in OMPD support and add local ICVs finctions. Message-ID: Reply-To: Jakub Jelinek References: MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 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.8 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, T_SCC_BODY_TEXT_LINE 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: Thu, 16 Jun 2022 12:22:31 -0000 On Fri, Jun 10, 2022 at 05:56:37PM +0200, Mohamed Atef wrote: In the subject line, there is typo: finctions. should be functions. > libgomp/ChangeLog > > 2022-06-10 Mohamed Atef > > * ompd-helper.h (DEREFERENCE, ACCESS_VALUE): New macros. > * ompd-helper.c (gompd_get_nthread, gompd_get_thread_limit, > gomp_get_run_shed, gompd_get_run_sched_chunk_size, > gompd_get_default_device, gompd_get_dynamic, > gompd_get_max_active_levels, gompd_get_proc_bind, > gompd_is_final, gompd_is_implicit, gompd_get_team_size): defined. Always start with capital letter after : , but also Define is used to describe new macros, not functions, for new functions usually one writes New functions. > * ompd-icv.c (ompd_get_icv_from_scope): call the previous fincions, Call functions > thread_handle, task_handle and parallel handle: New variable. When you describe the changes within some entity, in this case ompd_get_icv_from_scope, you already don't use the format whatever: What kind of change, so just write Call the above functions. Add thread_handle, task_handle and parallel_handle variables. Fix format in ashandle definition. > Fix format in ashandle definition. > * ompd-init.c: call GET_VALUE with sizeof_short for gompd_state. The changed entity is ompd_process_initialize. So * ompd-init.c (ompd_process_initialize): Use sizeof_short instead of sizeof_long_long in GET_VALUE argument. ? > * ompd-support.h (gompd_state): size of short instead of long. Change type from __UINT64_TYPE__ to unsigned short. > (GOMPD_FOREACH_ACCESS): Add > gompd_access (gomp_task, kind) > gompd_access (gomp_task, final_task) > gompd_access (gomp_team, nthreads) Better Add entries for gomp_task kind and final_task and gomp_team nthreads. > * ompd-support.c: Define > gompd_get_offset > gompd_get_sizeof_member > gompd_get_size. * ompd-support.c (gompd_get_offset, gompd_get_sizeof_member, gompd_get_size): Define. > (gompd_load): Remove gompd_init_access, > gompd_init_sizeof_members, gompd_init_sizes > define gompd_access_gomp_thread_handle with __UINT16_TYPE__. > --- a/libgomp/ompd-helper.c > +++ b/libgomp/ompd-helper.c > @@ -256,6 +256,350 @@ gompd_stringize_gompd_enabled (ompd_address_space_handle_t *ah, > > /* End of global ICVs functions. */ > > +/* Get per thread ICVs. */ > + > +ompd_rc_t > +gompd_get_nthread (ompd_thread_handle_t *thread_handle, > + ompd_word_t *nthreads_var) > +{ > + /* gomp_thread->task->gomp_task_icv.nthreads_var. */ > + if (thread_handle == NULL) > + return ompd_rc_stale_handle; > + if (nthreads_var == NULL) > + return ompd_rc_bad_input; > + CHECK (thread_handle->ah); > + > + ompd_word_t res = 0; > + ompd_address_t symbol_addr = thread_handle->th; > + ompd_word_t temp_offset; > + ompd_address_t temp_sym_addr; > + ompd_addr_t temp_addr; > + ompd_address_space_context_t *context = thread_handle->ah->context; > + ompd_thread_context_t *t_context = thread_handle->thread_context; > + ompd_rc_t ret; > + /* gomp_thread->task. */ > + ACCESS_VALUE (context, t_context, "gompd_access_gomp_thread_task", > + temp_offset, 1, ret, symbol_addr, temp_sym_addr, temp_addr); > + /* gomp_thread->task->task_icv. */ > + ACCESS_VALUE (context, t_context, "gompd_access_gomp_task_icv", temp_offset, > + 1, ret, symbol_addr, temp_sym_addr, temp_addr); > + /* gomp_thread->task->task_icv.nthreads_var. */ > + ACCESS_VALUE (context, t_context, "gompd_access_gomp_task_icv_nthreads_var", > + temp_offset, 0, ret, symbol_addr, temp_sym_addr, temp_addr); > + DEREFERENCE (context, t_context, symbol_addr, target_sizes.sizeof_long_long, > + 1, res, ret, 0); > + *nthreads_var = res; > + return ompd_rc_ok; > +} > + > +ompd_rc_t > +gompd_get_default_device (ompd_thread_handle_t *thread_handle, > + ompd_word_t *defalut_device_var) s/defalut/default/ > + ACCESS_VALUE (context, NULL, "gompd_access_gomp_task_icv", temp_offset, > + 1, ret, symbol_addr, temp_sym_addr, temp_addr); > + /* gomp_task->task_icv.thred_limit_var. */ s/thred/thread/ > + > +/* get per parallel handle ICVs. */ Capital letter - Get > @@ -130,6 +136,28 @@ ompd_get_icv_from_scope (void *handle, ompd_scope_t scope, ompd_icv_id_t icv_id, > return gompd_get_throttled_spin_count (ashandle, icv_value); > case gompd_icv_managed_threads_var: > return gompd_get_managed_threads (ashandle, icv_value); > + case gompd_icv_nthreads_var: > + return gompd_get_nthread (thread_handle, icv_value); > + case gompd_icv_default_device_var: > + return gompd_get_default_device (thread_handle, icv_value); > + case gompd_icv_dyn_var: > + return gompd_get_dynamic (thread_handle, icv_value); > + case gompd_icv_thread_limit_var: > + return gompd_get_thread_limit (task_handle, icv_value); > + case gompd_icv_run_sched_chunk_size: > + return gompd_get_run_sched_chunk_size (task_handle, icv_value); > + case gompd_icv_run_sched_var: > + return gompd_get_run_sched (task_handle, icv_value); > + case gompd_icv_max_active_levels_var: > + return gompd_get_max_active_levels (task_handle, icv_value); > + case gompd_icv_bind_var: > + return gompd_get_proc_bind (task_handle, icv_value); > + case gompd_icv_final_task_var: > + return gompd_is_final (task_handle, icv_value); > + case gompd_icv_implicit_task_var: > + return gompd_is_implicit (task_handle, icv_value); > + case gompd_icv_team_size_var: > + return gompd_get_team_size (parallel_handle, icv_value); The above is badly formatted or your mailer replaced tab characters with spaces (you can see it in the patch much better, but if you git diff, it should be marked red as well). > --- a/libgomp/ompd-support.c > +++ b/libgomp/ompd-support.c > @@ -20,46 +20,58 @@ > > #include "ompd-support.h" > > -#define gompd_declare_access(t, m) __UINT64_TYPE__ gompd_access_##t##_##m; > - GOMPD_FOREACH_ACCESS (gompd_declare_access) > -#undef gompd_declare_access > - > -#define gompd_declare_sizeof_members(t, m) \ > - __UINT64_TYPE__ gompd_sizeof_##t##_##m; > - GOMPD_FOREACH_ACCESS (gompd_declare_sizeof_members) > -#undef gompd_declare_sizeof_members > - > -#define gompd_declare_sizes(t) __UINT64_TYPE__ gompd_sizeof_##t; > - GOMPD_SIZES (gompd_declare_sizes) > -#undef gompd_declare_sizes > +#ifdef __ELF__ > +/* Get offset of the member m in struct t. */ > +#define gompd_get_offset(t, m) \ > + const __UINT16_TYPE__ gompd_access_##t##_##m __attribute__ ((used)) \ > + __attribute__ ((section ("OMPD"))) \ > + = (__UINT16_TYPE__) offsetof (struct t, m); > + GOMPD_FOREACH_ACCESS (gompd_get_offset) Here 2 things. I think it is better to use unsigned short type rather than __UINT16_TYPE__, the API has sizeof_short, not sizeof___UINT16_TYPE__. And, it is a maintainance nightmare to duplicate everything, one copy under #ifdef __ELF__ and one in #else part, with the only difference being the added __attribute__ ((section ("OMPD"))). I'd suggest just #ifdef __ELF__ #define OMPD_SECTION __attribute__ ((section ("OMPD"))) #else #define OMPD_SECTION #endif and use OMPD_SECTION unconditionally in all those macros. > > const char **ompd_dll_locations = NULL; > -__UINT64_TYPE__ gompd_state; > +__UINT16_TYPE__ gompd_state; unsigned short above too. > > void > gompd_load (void) > { > - /* Get the offset of the struct members. */ > - #define gompd_init_access(t, m) \ > - gompd_access_##t##_##m = (__UINT64_TYPE__) & (((struct t *) NULL)->m); > - GOMPD_FOREACH_ACCESS (gompd_init_access); > - #undef gompd_init_access > - > - /* Get sizeof members. */ > - > - #define gompd_init_sizeof_members(t, m) \ > - gompd_sizeof_##t##_##m = sizeof (((struct t *) NULL)->m); > - GOMPD_FOREACH_ACCESS (gompd_init_sizeof_members); > - #undef gompd_declare_sizeof_members > - > - #define gompd_init_sizes(t) gompd_sizeof_##t = sizeof (struct t); > - GOMPD_SIZES (gompd_init_sizes) > - #undef gompd_init_sizes > - > #ifdef GOMP_NEEDS_THREAD_HANDLE > - __UINT64_TYPE__ gompd_access_gomp_thread_handle > - = (__UINT64_TYPE__) & (((struct gomp_thread *) NULL)->handle); > - __UINT64_TYPE__ gompd_sizeof_gomp_thread_handle > + __UINT16_TYPE__ gompd_access_gomp_thread_handle > + = (__UINT16_TYPE__) & (((struct gomp_thread *) NULL)->handle); > + __UINT16_TYPE__ gompd_sizeof_gomp_thread_handle > = sizeof (((struct gomp_thread *) NULL)->handle); > #endif This still means gompd_access_gomp_thread_handle and gompd_access_gomp_thread_handle are automatic variables and libgompd won't see them. So, they still should be defined at file scope and ideally initialized there too. One possibility is to only include struct gomp_thread, handle entry in the macro which is iterated through conditionally, by doing #ifdef GOMP_NEEDS_THREAD_HANDLE #define gompd_thread_handle_access gompd_access (gomp_thread, handle) #else #define gompd_thread_handle_access #endif and gompd_thread_handle_access next to other gompd_access lines. As I wrote in another mail, one possibility for the #ifndef GOMP_NEEDS_THREAD_HANDLE case would be still to define the gompd_{access,sizeof}_thread_handle vars but ensure they will be 0, 0 in that case and then use sizeof 0 as a hint for libgompd that thread handle isn't available. > gomp_debug (2, "OMP OMPD active\n"); > diff --git a/libgomp/ompd-support.h b/libgomp/ompd-support.h > index 39d55161132..5d7e5c24e3f 100644 > --- a/libgomp/ompd-support.h > +++ b/libgomp/ompd-support.h > @@ -67,7 +67,7 @@ > #endif > > void gompd_load (void); > -extern __UINT64_TYPE__ gompd_state; > +extern __UINT16_TYPE__ gompd_state; Again, unsigned short. > #define OMPD_ENABLED 0x1 > > @@ -83,7 +83,10 @@ extern __UINT64_TYPE__ gompd_state; > gompd_access (gomp_thread_pool, threads) \ > gompd_access (gomp_thread, ts) \ > gompd_access (gomp_team_state, team_id) \ > - gompd_access (gomp_task, icv) > + gompd_access (gomp_task, icv) \ > + gompd_access (gomp_task, kind) \ > + gompd_access (gomp_task, final_task) \ > + gompd_access (gomp_team, nthreads) > > #define GOMPD_SIZES(gompd_size) \ > gompd_size (gomp_thread) \ Jakub