[hsa 2/12] Modifications to libgomp proper

[Thomas Schwinge]

Hi!

On Thu, 12 Nov 2015 11:11:33 +0100, Jakub Jelinek <jakub@redhat.com> 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 (&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.

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,
<http://news.gmane.org/find-root.php?message_id=%3C87mvve95af.fsf%40schwinge.name%3E>.
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 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?


Grüße
 Thomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 472 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20151112/acfd52d8/attachment.sig>