Re: [PATCH] OpenMP, libgomp: Add new runtime routine omp_get_mapped_ptr.

[Jakub Jelinek <jakub@redhat.com>]

On Fri, Mar 04, 2022 at 03:47:31PM +0100, Marcel Vollweiler wrote:
> libgomp/ChangeLog:
> 
> 	* libgomp.map: Added omp_get_mapped_ptr.
> 	* libgomp.texi: Tagged omp_get_mapped_ptr as supported.
> 	* omp.h.in: Added omp_get_mapped_ptr.
> 	* omp_lib.f90.in: Added interface for omp_get_mapped_ptr.
> 	* omp_lib.h.in: Likewise.
> 	* target.c (omp_get_mapped_ptr): Added implementation of
> 	omp_get_mapped_ptr.
> 	* testsuite/libgomp.c-c++-common/get-mapped-ptr-1.c: New test.
> 	* testsuite/libgomp.c-c++-common/get-mapped-ptr-2.c: New test.
> 	* testsuite/libgomp.c-c++-common/get-mapped-ptr-3.c: New test.
> 	* testsuite/libgomp.c-c++-common/get-mapped-ptr-4.c: New test.
> 	* testsuite/libgomp.fortran/get-mapped-ptr-1.f90: New test.
> 	* testsuite/libgomp.fortran/get-mapped-ptr-2.f90: New test.
> 	* testsuite/libgomp.fortran/get-mapped-ptr-3.f90: New test.
> 	* testsuite/libgomp.fortran/get-mapped-ptr-4.f90: New test.
> 
> diff --git a/libgomp/libgomp.map b/libgomp/libgomp.map
> index 2ac5809..00a4858 100644
> --- a/libgomp/libgomp.map
> +++ b/libgomp/libgomp.map
> @@ -224,6 +224,7 @@ OMP_5.1 {
>  	omp_set_teams_thread_limit_8_;
>  	omp_get_teams_thread_limit;
>  	omp_get_teams_thread_limit_;
> +	omp_get_mapped_ptr;
>  } OMP_5.0.2;

I think it is too late for this to be targetted for GCC 12, and
for GCC 13 it will need to go into OMP_5.1.1 symver.

>  GOMP_1.0 {
> diff --git a/libgomp/libgomp.texi b/libgomp/libgomp.texi
> index 161a423..c163b56 100644
> --- a/libgomp/libgomp.texi
> +++ b/libgomp/libgomp.texi
> @@ -314,7 +314,7 @@ The OpenMP 4.5 specification is fully supported.
>  @item @code{omp_target_is_accessible} runtime routine @tab N @tab
>  @item @code{omp_target_memcpy_async} and @code{omp_target_memcpy_rect_async}
>        runtime routines @tab N @tab
> -@item @code{omp_get_mapped_ptr} runtime routine @tab N @tab
> +@item @code{omp_get_mapped_ptr} runtime routine @tab Y @tab
>  @item @code{omp_calloc}, @code{omp_realloc}, @code{omp_aligned_alloc} and
>        @code{omp_aligned_calloc} runtime routines @tab Y @tab
>  @item @code{omp_alloctrait_key_t} enum: @code{omp_atv_serialized} added,
> diff --git a/libgomp/omp.h.in b/libgomp/omp.h.in
> index 89c5d65..18d0152 100644
> --- a/libgomp/omp.h.in
> +++ b/libgomp/omp.h.in
> @@ -282,6 +282,7 @@ extern int omp_target_memcpy_rect (void *, const void *, __SIZE_TYPE__, int,
>  extern int omp_target_associate_ptr (const void *, const void *, __SIZE_TYPE__,
>  				     __SIZE_TYPE__, int) __GOMP_NOTHROW;
>  extern int omp_target_disassociate_ptr (const void *, int) __GOMP_NOTHROW;
> +extern void *omp_get_mapped_ptr (const void *, int) __GOMP_NOTHROW;
>  
>  extern void omp_set_affinity_format (const char *) __GOMP_NOTHROW;
>  extern __SIZE_TYPE__ omp_get_affinity_format (char *, __SIZE_TYPE__)
> diff --git a/libgomp/omp_lib.f90.in b/libgomp/omp_lib.f90.in
> index daf40dc..506f15c 100644
> --- a/libgomp/omp_lib.f90.in
> +++ b/libgomp/omp_lib.f90.in
> @@ -835,6 +835,15 @@
>            end function omp_target_disassociate_ptr
>          end interface
>  
> +        interface
> +          function omp_get_mapped_ptr (ptr, device_num) bind(c)
> +            use, intrinsic :: iso_c_binding, only : c_ptr, c_int
> +            type(c_ptr) :: omp_get_mapped_ptr
> +            type(c_ptr), value :: ptr
> +            integer(c_int), value :: device_num
> +          end function omp_get_mapped_ptr
> +        end interface
> +
>  #if _OPENMP >= 201811
>  !GCC$ ATTRIBUTES DEPRECATED :: omp_get_nested, omp_set_nested
>  #endif
> diff --git a/libgomp/omp_lib.h.in b/libgomp/omp_lib.h.in
> index ff857a4..0f48510 100644
> --- a/libgomp/omp_lib.h.in
> +++ b/libgomp/omp_lib.h.in
> @@ -416,3 +416,12 @@
>            integer(c_int), value :: device_num
>          end function omp_target_disassociate_ptr
>        end interface
> +
> +      interface
> +        function omp_get_mapped_ptr (ptr, device_num) bind(c)
> +          use, intrinsic :: iso_c_binding, only : c_ptr, c_int
> +          type(c_ptr) :: omp_get_mapped_ptr
> +          type(c_ptr), value :: ptr
> +          integer(c_int), value :: device_num
> +        end function omp_get_mapped_ptr
> +      end interface
> diff --git a/libgomp/target.c b/libgomp/target.c
> index 9017458..735d70b 100644
> --- a/libgomp/target.c
> +++ b/libgomp/target.c
> @@ -3665,6 +3665,49 @@ omp_target_disassociate_ptr (const void *ptr, int device_num)
>    return ret;
>  }
>  
> +void *
> +omp_get_mapped_ptr (const void *ptr, int device_num)
> +{
> +  if (device_num < 0 || device_num > omp_get_num_devices ())
> +    return NULL;
> +
> +  if (device_num == omp_get_initial_device ())
> +    return (void*)ptr;

Space before * and space after )

> +  struct gomp_device_descr *devicep = resolve_device (device_num);
> +  if (devicep == NULL)
> +    return NULL;
> +
> +  if (!(devicep->capabilities & GOMP_OFFLOAD_CAP_OPENMP_400)
> +      || devicep->capabilities & GOMP_OFFLOAD_CAP_SHARED_MEM)
> +    return (void*)ptr;

Likewise.
> +
> +  gomp_mutex_lock (&devicep->lock);
> +
> +  struct splay_tree_s *mem_map = &devicep->mem_map;
> +  struct splay_tree_key_s cur_node;
> +  void *ret = NULL;
> +  uintptr_t offset = 0;

offset should be moved to the only place that defines it.
> +
> +  cur_node.host_start = (uintptr_t) ptr;
> +  cur_node.host_end = cur_node.host_start;
> +  splay_tree_key n = gomp_map_0len_lookup (mem_map, &cur_node);
> +
> +  if (n && n->host_start == cur_node.host_start)
> +    {
> +      ret = (void*) n->tgt->tgt_start + n->tgt_offset;
> +    }

Single statement body, so without {}s and reindented, space before *.
> +  else if (n)
> +    {
> +      offset = cur_node.host_start - n->host_start;
      uintptr_t offset = cur_node.host_start - n->host_start;

> +      ret = (void*) n->tgt->tgt_start + n->tgt_offset + offset;

Space before *.

Though, looking at this more, what is the point of the first if?
The second if would compute offset = 0...

Also, void * arithmetics is a GNU extension, maybe better use char *.

> +  if (omp_get_mapped_ptr (q, -1) != NULL)
> +    __builtin_abort ();

When you do include stdlib.h, what is the point of using __builtin_abort ?
Just use abort then.

	Jakub