Re: [PATCH] libgompd: Fix sizes in OMPD support and add local ICVs finctions.

[Jakub Jelinek <jakub@redhat.com>]

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