Re: [hsa 2/10] Modifications to libgomp proper

[Jakub Jelinek <jakub at redhat dot com>]

On Mon, Dec 07, 2015 at 12:19:57PM +0100, Martin Jambor wrote:
> +/* Flag set when the subsequent element in the device-specific argument
> +   values.  */
> +#define GOMP_TARGET_ARG_SUBSEQUENT_PARAM	(1 << 7)
> +
> +/* Bitmask to apply to a target argument to find out the value identifier.  */
> +#define GOMP_TARGET_ARG_ID_MASK			(((1 << 8) - 1) << 8)
> +/* Target argument index of NUM_TEAMS.  */
> +#define GOMP_TARGET_ARG_NUM_TEAMS		(1 << 8)
> +/* Target argument index of THREAD_LIMIT.  */
> +#define GOMP_TARGET_ARG_THREAD_LIMIT		(2 << 8)

I meant that these two would be just special, passed as the first two
pointers in the array, without the markup.  Because, otherwise you either
need to use GOMP_TARGET_ARG_SUBSEQUENT_PARAM for these always, or for 32-bit
arches and for 64-bit ones shift often at runtime.  Having the markup even
for them is perhaps cleaner, but less efficient, so if you really want to go
that way, please make sure you handle it properly for 32-bit pointers
architectures though.  num_teams or thread_limit could be > 32767 or >
65535.

> -static void
> -gomp_target_fallback_firstprivate (void (*fn) (void *), size_t mapnum,
> -				   void **hostaddrs, size_t *sizes,
> -				   unsigned short *kinds)
> +static void *
> +gomp_target_unshare_firstprivate (size_t mapnum, void **hostaddrs,
> +				  size_t *sizes, unsigned short *kinds)
>  {
>    size_t i, tgt_align = 0, tgt_size = 0;
>    char *tgt = NULL;
> @@ -1281,7 +1282,7 @@ gomp_target_fallback_firstprivate (void (*fn) (void *), size_t mapnum,
>        }
>    if (tgt_align)
>      {
> -      tgt = gomp_alloca (tgt_size + tgt_align - 1);
> +      tgt = gomp_malloc (tgt_size + tgt_align - 1);

I don't like using gomp_malloc here, either copy/paste the function, or
create separate inline functions for the two loops, one for the first loop
which returns you tgt_align and tgt_size, and another for the stuff after
the allocation.  Then you can use those two inline functions to implement
both gomp_target_fallback_firstprivate which will use alloca, and
gomp_target_unshare_firstprivate which will use gomp_malloc instead.

> @@ -1356,6 +1377,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.

Note, here you document NUM_TEAMS and THREAD_LIMIT as special values, not
encoded.

> @@ -1473,6 +1508,7 @@ GOMP_target_data (int device, const void *unused, size_t mapnum,
>    struct gomp_device_descr *devicep = resolve_device (device);
>  
>    if (devicep == NULL
> +      || (devicep->capabilities & GOMP_OFFLOAD_CAP_SHARED_MEM)
>        || !(devicep->capabilities & GOMP_OFFLOAD_CAP_OPENMP_400))

Would be nice to have some consistency in the order of capabilities checks.
Usually you check SHARED_MEM after OPENMP_400, so perhaps do it this way
here too.

> @@ -1741,23 +1784,38 @@ gomp_target_task_fn (void *data)
>  
>        if (ttask->state == GOMP_TARGET_TASK_FINISHED)
>  	{
> -	  gomp_unmap_vars (ttask->tgt, true);
> +	  if (ttask->tgt)
> +	    gomp_unmap_vars (ttask->tgt, true);
>  	  return false;
>  	}

This doesn't make sense.  For the GOMP_OFFLOAD_CAP_SHARED_MEM case, unless
you want to run the free (ttask->firstprivate_copies); as a task, you
shouldn't be queing the target task again for further execution, instead
it should just be handled like a finished task at that point.  The reason
why for XeonPhi or PTX gomp_target_task_fn is run with
GOMP_TARGET_TASK_FINISHED state is that gomp_unmap_vars can perform IO
actions and doing it with the tasking lock held is highly undesirable.
>  
> -      void *fn_addr = gomp_get_target_fn_addr (devicep, ttask->fn);
> -      ttask->tgt
> -	= gomp_map_vars (devicep, ttask->mapnum, ttask->hostaddrs, NULL,
> -			 ttask->sizes, ttask->kinds, true,
> -			 GOMP_MAP_VARS_TARGET);
> +      bool shared_mem;
> +      if (devicep->capabilities & GOMP_OFFLOAD_CAP_SHARED_MEM)
> +	{
> +	  shared_mem = true;
> +	  ttask->tgt = NULL;
> +	  ttask->firstprivate_copies
> +	    = gomp_target_unshare_firstprivate (ttask->mapnum, ttask->hostaddrs,
> +						ttask->sizes, ttask->kinds);
> +	}
> +      else
> +	{
> +	  shared_mem = false;
> +	  ttask->tgt = gomp_map_vars (devicep, ttask->mapnum, ttask->hostaddrs,
> +				      NULL, ttask->sizes, ttask->kinds, true,
> +				      GOMP_MAP_VARS_TARGET);
> +	}
>        ttask->state = GOMP_TARGET_TASK_READY_TO_RUN;
>  
>        devicep->async_run_func (devicep->target_id, fn_addr,
> -			       (void *) ttask->tgt->tgt_start, (void *) ttask);
> +			       shared_mem ? ttask->hostaddrs
> +			       : (void *) ttask->tgt->tgt_start,
> +			       ttask->args, (void *) ttask);

Replace the shared_mem bool variable with a void *arg or some other name and just
set it to ttask->hostaddrs in the then case and ttask->tgt->tgt_start
otherwise?

> --- a/libgomp/task.c
> +++ b/libgomp/task.c
> @@ -581,6 +581,7 @@ GOMP_PLUGIN_target_task_completion (void *data)
>        gomp_mutex_unlock (&team->task_lock);
>      }
>    ttask->state = GOMP_TARGET_TASK_FINISHED;
> +  free (ttask->firstprivate_copies);
>    gomp_target_task_completion (team, task);
>    gomp_mutex_unlock (&team->task_lock);
>  }

So, this function should have a special case for the SHARED_MEM case, handle
it closely to say how GOMP_taskgroup_end handles the finish_cancelled:
case.  Just note that the target task is missing from certain queues at that
point.

	Jakub