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

[Jakub Jelinek <jakub at redhat dot com>]

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,
> <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?)

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