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

[Jakub Jelinek]

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 (>mem_map, );
>gomp_mutex_unlock (>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.

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

[Thomas Schwinge]

Hi!

On Thu, 12 Nov 2015 11:11:33 +0100, Jakub Jelinek  wrote:
> On Thu, Nov 05, 2015 at 10:54:42PM +0100, Martin Jambor wrote:
> > --- 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.

I'm confused -- didn't we agree that we don't need to maintain backwards
compatibility in the libgomp <-> plugins interface?  (Nathan?)  As far as
I remember, the argument was that libgomp and all its plugins will always
be built from the same source tree, so will be compatible with each
other, "by definition"?

(We do need, and have, versioning between GCC proper and libgomp
interfaces.)


> > --- 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 (>mem_map, );
> >gomp_mutex_unlock (>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.

I like them, because they help during development, and for getting
higher-quality bug reports from users, and they serve as source code
documentation.  Of course, I understand your -- I suppose -- performance
worries.  Does such an NULL checking assert -- hopefully marked as
"unlikely" -- cause any noticeable overhead, though?


> 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.

That's one of the issues that I'm working on resolving with my
"Forwarding -foffload=[...] from the driver (compile-time) to libgomp
(run-time)" patch,
.
In such a case (no GOMP_offload_register_ver call for HSA), HSA
offloading would not be considered (not "enabled") in libgomp.  (It'll be
two more weeks before I can make progress with that patch; will be
attending SuperComputing 2015 next week -- anyone else will be there,
too?)

> 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 

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

[Nathan Sidwell]

On 11/12/15 08:21, Thomas Schwinge wrote:

Hi!




so there is a way to deal with it, but you need to adjust all plugins.


I'm confused -- didn't we agree that we don't need to maintain backwards
compatibility in the libgomp <-> plugins interface?  (Nathan?)


Indeed, no need to deal with version skew between libgomp and its plugins.

On 07/24/15 12:30, Jakub Jelinek wrote:

And I'd say that we don't really need to maintain support for mixing libgomp
from one GCC version and libgomp plugins from another version, worst case
there should be some GOMP_OFFLOAD_get_version function that libgomp could
use to verify it is talking to the right version of the plugin and
completely ignore it if it gives wrong version.




(We do need, and have, versioning between GCC proper and libgomp
interfaces.)


Yes. (For avoidance of doubt)

nathan

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

[Jakub Jelinek]

On Thu, Nov 12, 2015 at 02:21:56PM +0100, Thomas Schwinge wrote:
> > > --- 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.
> 
> I'm confused -- didn't we agree that we don't need to maintain backwards
> compatibility in the libgomp <-> plugins interface?  (Nathan?)  As far as
> I remember, the argument was that libgomp and all its plugins will always
> be built from the same source tree, so will be compatible with each
> other, "by definition"?
> 
> (We do need, and have, versioning between GCC proper and libgomp
> interfaces.)

I've mentioned the GOMP_VERSION check in there, and that all the plugins
would need to be modified, which was I think not shown in the patch.

> > 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.
> 
> That's one of the issues that I'm working on resolving with my
> "Forwarding -foffload=[...] from the driver (compile-time) to libgomp
> (run-time)" patch,
> .
> In such a case (no GOMP_offload_register_ver call for HSA), HSA
> offloading would not be considered (not "enabled") in libgomp.  (It'll be
> two more weeks before I can make progress with that patch; will be
> attending SuperComputing 2015 next week -- anyone else will be there,
> too?)

You are aware of my objections to that and what does it do with later dlopened
libraries.

> > 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).
> 
> What about adopting the "tagging" scheme that we added for
> libgomp/oacc-parallel.c:GOACC_parallel_keyed?  With support for other
> offloading schemes being added one by one, isn't it quite likely that the
> interface will need to be adjusted for each of them, because
> more/different data will have to be transmitted from GCC proper to
> libgomp?

Perhaps something similar, but certainly not as varargs, that is a really
bad idea.  Instead of the long * array I've talked about perhaps use void **,
and put in unkeyed num_teams and thread_limit as the first two arguments
thereof, then add keyed arguments in there, terminate by 0.

Jakub