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

[Jakub Jelinek <jakub@redhat.com>]

On Thu, Nov 05, 2015 at 10:54:42PM +0100, Martin Jambor wrote:
> The patch below contains all changes to libgomp files.  First, it adds
> a new constant identifying HSA devices and a structure that is shared
> between libgomp and the compiler when kernels from kernels are invoked
> via dynamic parallelism.
> 
> Second it modifies the GOMP_target_41 function so that it also can take
> kernel attributes (essentially the grid dimension) as a parameter and
> pass it on the HSA libgomp plugin.  Because we do want HSAIL
> generation to gracefully fail and use host fallback in that case, the
> same function calls the host implementation if it cannot map the
> requested function to an accelerated one or of a new callback
> can_run_func indicates there is a problem.
> 
> We need a new hook because we use it to check for linking errors which
> we cannot do when incrementally loading registered images.  And we
> want to handle linking errors, so that when we cannot emit HSAIL for a
> function called from a kernel (possibly in a different compilation
> unit), we also resort to host fallback.
> 
> Last but not least, the patch removes data remapping when the selected
> device is capable of sharing memory with the host.

The patch clearly is not against current trunk, there is no GOMP_target_41
function, the GOMP_target_ext function has extra arguments, etc.

> diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
> index 9c8b1fb..0ad42d2 100644
> --- a/libgomp/libgomp.h
> +++ b/libgomp/libgomp.h
> @@ -876,7 +876,8 @@ 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 *, const void *);

Adding arguments to existing plugin methods is a plugin ABI incompatible
change.  We now have:
  DLSYM (version);
  if (device->version_func () != GOMP_VERSION)
    {
      err = "plugin version mismatch";
      goto fail;
    }
so there is a way to deal with it, but you need to adjust all plugins.
See below anyway.

> --- a/libgomp/oacc-host.c
> +++ b/libgomp/oacc-host.c
> @@ -123,7 +123,8 @@ host_host2dev (int n __attribute__ ((unused)),
>  }
>  
>  static void
> -host_run (int n __attribute__ ((unused)), void *fn_ptr, void *vars)
> +host_run (int n __attribute__ ((unused)), void *fn_ptr, void *vars,
> +	  const void* kern_launch __attribute__ ((unused)))

This is C, space before * not after it.
>  {
>    void (*fn)(void *) = (void (*)(void *)) fn_ptr;

> --- a/libgomp/target.c
> +++ b/libgomp/target.c
> @@ -1248,7 +1248,12 @@ gomp_get_target_fn_addr (struct gomp_device_descr *devicep,
>        splay_tree_key tgt_fn = splay_tree_lookup (&devicep->mem_map, &k);
>        gomp_mutex_unlock (&devicep->lock);
>        if (tgt_fn == NULL)
> -	gomp_fatal ("Target function wasn't mapped");
> +	{
> +	  if (devicep->capabilities & GOMP_OFFLOAD_CAP_SHARED_MEM)
> +	    return NULL;
> +	  else
> +	    gomp_fatal ("Target function wasn't mapped");
> +	}
>  
>        return (void *) tgt_fn->tgt_offset;
>      }
> @@ -1276,6 +1281,7 @@ GOMP_target (int device, void (*fn) (void *), const void *unused,
>      return gomp_target_fallback (fn, hostaddrs);
>  
>    void *fn_addr = gomp_get_target_fn_addr (devicep, fn);
> +  assert (fn_addr);

I must say I really don't like putting asserts into libgomp, in production
it is after all not built with -D_NDEBUG.  But this shows a worse problem,
if you have GCC 5 compiled OpenMP code, of course there won't be HSA
offloaded copy, but if you try to run it on a box with HSA offloading
enabled, you can run into this assertion failure.
Supposedly the old APIs (GOMP_target, GOMP_target_update, GOMP_target_data)
should treat GOMP_OFFLOAD_CAP_SHARED_MEM capable devices as unconditional
device fallback?

> @@ -1297,7 +1304,7 @@ GOMP_target (int device, void (*fn) (void *), const void *unused,
>  void
>  GOMP_target_41 (int device, void (*fn) (void *), size_t mapnum,
>  		void **hostaddrs, size_t *sizes, unsigned short *kinds,
> -		unsigned int flags, void **depend)
> +		unsigned int flags, void **depend, const void *kernel_launch)

GOMP_target_ext has different arguments, you get the num_teams and
thread_limit clauses values in there already (if known at compile time or
before entering target region; 0 stands for implementation defined choice,
-1 for unknown before GOMP_target_ext).
Plus I must say I really don't like the addition of HSA specific argument
to the API, it is unclean and really doesn't scale, when somebody adds
support for another offloading target, would we add again another argument?
Can't use the same one, because one could have configured both HSA and that
other kind offloading at the same time and which one is picked would be only
a runtime decision, based on env vars of omp_set_default_device etc.
num_teams/thread_limit, as runtime arguments, you already get on the trunk.
For compile time decided values, those should go into some data section
and be somehow attached to what fn is translated into in the AVL tree (which
you really don't need to use for variables on GOMP_OFFLOAD_CAP_SHARED_MEM
obviously, but can still use for the kernels, and populate during
registration of the offloading region).

>  {
>    struct gomp_device_descr *devicep = resolve_device (device);
>  
> @@ -1312,8 +1319,16 @@ GOMP_target_41 (int device, void (*fn) (void *), size_t mapnum,
>  	gomp_task_maybe_wait_for_dependencies (depend);
>      }
>  
> +  void *fn_addr = NULL;
> +  bool host_fallback = false;
>    if (devicep == NULL
> -      || !(devicep->capabilities & GOMP_OFFLOAD_CAP_OPENMP_400))
> +      || !(devicep->capabilities & GOMP_OFFLOAD_CAP_OPENMP_400)
> +      || !(fn_addr = gomp_get_target_fn_addr (devicep, fn))
> +      || (devicep->can_run_func && !devicep->can_run_func (fn_addr)))
> +    host_fallback = true;
> +
> +  if (host_fallback
> +      || devicep->capabilities & GOMP_OFFLOAD_CAP_SHARED_MEM)

The part below is now in a different function.

>      {
>        size_t i, tgt_align = 0, tgt_size = 0;
>        char *tgt = NULL;
> @@ -1343,15 +1358,20 @@ GOMP_target_41 (int device, void (*fn) (void *), size_t mapnum,
>  		tgt_size = tgt_size + sizes[i];
>  	      }
>  	}
> -      gomp_target_fallback (fn, hostaddrs);
> -      return;
> -    }
>  

	Jakub