Re: [RFC] Device-specific OpenMP target arguments

[Jakub Jelinek <jakub at redhat dot com>]

On Fri, Nov 13, 2015 at 07:40:27PM +0100, Martin Jambor wrote:
> the patch below is is an untested trunk-only implementation of device
> specific GOMP_target_ext arguments, which was proposed to me by Jakub
> today on IRC.  I'm sending this patch to make sure I understood the
> details well.  Nevertheless, I will be commiting a tested and working
> version to the hsa branch shortly.

Well, the details aren't really in the patch though.

> --- a/gcc/builtin-types.def
> +++ b/gcc/builtin-types.def
> @@ -556,9 +556,9 @@ DEF_FUNCTION_TYPE_9 (BT_FN_VOID_OMPFN_PTR_OMPCPYFN_LONG_LONG_BOOL_UINT_PTR_INT,
>  		     BT_PTR_FN_VOID_PTR_PTR, BT_LONG, BT_LONG,
>  		     BT_BOOL, BT_UINT, BT_PTR, BT_INT)
>  
> -DEF_FUNCTION_TYPE_10 (BT_FN_VOID_INT_OMPFN_SIZE_PTR_PTR_PTR_UINT_PTR_INT_INT,
> -		      BT_VOID, BT_INT, BT_PTR_FN_VOID_PTR, BT_SIZE, BT_PTR,
> -		      BT_PTR, BT_PTR, BT_UINT, BT_PTR, BT_INT, BT_INT)
> +DEF_FUNCTION_TYPE_9 (BT_FN_VOID_INT_OMPFN_SIZE_PTR_PTR_PTR_UINT_PTR_PTR,
> +		     BT_VOID, BT_INT, BT_PTR_FN_VOID_PTR, BT_SIZE, BT_PTR,
> +		     BT_PTR, BT_PTR, BT_UINT, BT_PTR, BT_PTR)

A _9 entry should be groupped together with other _9 entries, so no empty
line in between.

> diff --git a/gcc/fortran/types.def b/gcc/fortran/types.def
> index a37e856..279d055 100644
> --- a/gcc/fortran/types.def
> +++ b/gcc/fortran/types.def
> @@ -221,9 +221,9 @@ DEF_FUNCTION_TYPE_9 (BT_FN_VOID_OMPFN_PTR_OMPCPYFN_LONG_LONG_BOOL_UINT_PTR_INT,
>  		     BT_PTR_FN_VOID_PTR_PTR, BT_LONG, BT_LONG,
>  		     BT_BOOL, BT_UINT, BT_PTR, BT_INT)
>  
> -DEF_FUNCTION_TYPE_10 (BT_FN_VOID_INT_OMPFN_SIZE_PTR_PTR_PTR_UINT_PTR_INT_INT,
> -		      BT_VOID, BT_INT, BT_PTR_FN_VOID_PTR, BT_SIZE, BT_PTR,
> -		      BT_PTR, BT_PTR, BT_UINT, BT_PTR, BT_INT, BT_INT)
> +DEF_FUNCTION_TYPE_9 (BT_FN_VOID_INT_OMPFN_SIZE_PTR_PTR_PTR_UINT_PTR_PTR,
> +		     BT_VOID, BT_INT, BT_PTR_FN_VOID_PTR, BT_SIZE, BT_PTR,
> +		     BT_PTR, BT_PTR, BT_UINT, BT_PTR, BT_PTR)

Likewise.

> --- a/libgomp/libgomp.h
> +++ b/libgomp/libgomp.h
> @@ -876,7 +876,7 @@ struct gomp_device_descr
>    void *(*dev2host_func) (int, void *, const void *, size_t);
>    void *(*host2dev_func) (int, void *, const void *, size_t);
>    void *(*dev2dev_func) (int, void *, const void *, size_t);
> -  void (*run_func) (int, void *, void *);
> +  void (*run_func) (int, void *, void *, void **);

There is now async_run_func too, which needs similar treatment.

> --- a/libgomp/target.c
> +++ b/libgomp/target.c
> @@ -1373,7 +1373,8 @@ GOMP_target (int device, void (*fn) (void *), const void *unused,
>        thr->place = old_thr.place;
>        thr->ts.place_partition_len = gomp_places_list_len;
>      }
> -  devicep->run_func (devicep->target_id, fn_addr, (void *) tgt_vars->tgt_start);
> +  devicep->run_func (devicep->target_id, fn_addr, (void *) tgt_vars->tgt_start,
> +		     NULL);
>    gomp_free_thread (thr);
>    *thr = old_thr;
>    gomp_unmap_vars (tgt_vars, true);
> @@ -1383,6 +1384,11 @@ GOMP_target (int device, void (*fn) (void *), const void *unused,
>     and several arguments have been added:
>     FLAGS is a bitmask, see GOMP_TARGET_FLAG_* in gomp-constants.h.
>     DEPEND is array of dependencies, see GOMP_task for details.
> +   ARGS is a pointer to an array consisting of NUM_TEAMS, THREAD_LIMIT and a
> +   variable number of device-specific arguments, which always take two elements
> +   where the first specifies the type and the second the actual value.  The
> +   last element of the array is a single NULL.

I wouldn't document that it has to be always a pair, simply always a new
pointer in the array encodes:
a) a few bits describing what offloading target this is for
   (enum offload_target_type or some other constant, but needs to be defined
   in gomp-constants.h; offload_target_type has the advantage that it exists
   alrady, and disadvantage that it already now contains gaps, for now
   we really need some value describing that it applies to all offloading
   targets, then host fallback, XeonPhi, NVidia, HSA (but with enough room
   for future growth))
b) a few bits describing the opcode
c) the remaining bits could be used to hold shorter values; say if you
   have a bool value (or some set of bools), they could go inline, or
   say some integer with limited range (say 16 bits at most), or perhaps
   on 64-bit hosts even full integer (and on 32-bit hosts the argument
   would go in the next pointer instead)
And this pointer would be followed by optional arguments, as specific to the
opcode.
Though, if we want other plugins to be able to easily skip opcodes for
other offloading targets, perhaps there should be also bits describing
how many arguments the opcode has.

> --- a/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp
> +++ b/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp
> @@ -501,7 +501,8 @@ GOMP_OFFLOAD_dev2dev (int device, void *dst_ptr, const void *src_ptr,
>  }
>  
>  extern "C" void
> -GOMP_OFFLOAD_run (int device, void *tgt_fn, void *tgt_vars)
> +GOMP_OFFLOAD_run (int device, void *tgt_fn, void *tgt_vars,
> +		  void **args __attribute__((unused)))
>  {
>    TRACE ("(tgt_fn = %p, tgt_vars = %p)", tgt_fn, tgt_vars);

This plugin is written in C++, so you can just use void **/* args */ or
void ** instead.

	Jakub