On Fri, Dec 14, 2018 at 06:52:20PM +0100, Thomas Schwinge wrote:
> > --- a/include/gomp-constants.h
> > +++ b/include/gomp-constants.h
>
> > @@ -199,7 +200,7 @@ enum gomp_map_kind
> > /* Versions of libgomp and device-specific plugins. GOMP_VERSION
> > should be incremented whenever an ABI-incompatible change is introduced
> > to the plugin interface defined in libgomp/libgomp.h. */
> > -#define GOMP_VERSION 1
> > +#define GOMP_VERSION 2
> > #define GOMP_VERSION_NVIDIA_PTX 1
> > #define GOMP_VERSION_INTEL_MIC 0
> > #define GOMP_VERSION_HSA 0
>
> OK, I think -- but I'm never quite sure whether we do need to increment
> "GOMP_VERSION" when only doing libgomp-internal libgomp-plugin changes,
> which don't affect the user/GCC side?
>
> GCC encodes "GOMP_VERSION" in "GOMP_offload_register_ver" calls
> synthesized by "mkoffload": "GOMP_VERSION_PACK (/* LIB */ GOMP_VERSION,
> /* DEV */ GOMP_VERSION_NVIDIA_PTX)", and then at run time libgomp checks
> in "GOMP_offload_register_ver", so that we don't try to load offloading
> code with an _old_ libgomp that has been compiled with/for the _new_
> version. (Right?)
To me it looks wrong to tie two different things in the same version number.
Just because we are changing something in the libgomp vs. the corresponding
plugin APIs doesn't mean we need to rebuild all binaries and libraries that
have offloading code in it.
So, IMHO GOMP_VERSION should be bumped only if we do a change that requires
the offloading data to be changed, and either have an additional internal
version to make sure that the plugin are kept in sync with libgomp, or just
figure that out because dlsym will fail on some of the new symbols in the
plugin.
> > --- a/libgomp/libgomp.map
> > +++ b/libgomp/libgomp.map
> > @@ -458,7 +462,6 @@ GOMP_PLUGIN_1.0 {
> > GOMP_PLUGIN_debug;
> > GOMP_PLUGIN_error;
> > GOMP_PLUGIN_fatal;
> > - GOMP_PLUGIN_async_unmap_vars;
> > GOMP_PLUGIN_acc_thread;
> > };
>
> I think that's fine, but highlighting this again for Jakub, in case
> there's an issue with removing a symbol from the libgomp-plugin
> interface.
I'd prefer not to remove symbols from libgomp.so.*. You can
do a gomp_fatal in it.
>
>
> > --- a/libgomp/libgomp-plugin.h
> > +++ b/libgomp/libgomp-plugin.h
>
> > +/* Opaque type to represent plugin-dependent implementation of an
> > + OpenACC asynchronous queue. */
> > +struct goacc_asyncqueue;
> > +
> > +/* Used to keep a list of active asynchronous queues. */
> > +struct goacc_asyncqueue_list
> > +{
> > + struct goacc_asyncqueue *aq;
> > + struct goacc_asyncqueue_list *next;
> > +};
> > +
> > +typedef struct goacc_asyncqueue *goacc_aq;
> > +typedef struct goacc_asyncqueue_list *goacc_aq_list;
>
> I'm not too fond of such "syntactic sugar" typedefs, but if that's fine
> for Jakub to have in libgomp, then I won't object.
If it helps with making sure the formatting of the code isn't too ugly,
yes, otherwise no.
> I'd be in favor then of "typedef struct N *N" or "typedef struct N *N_t"
Please avoid *_t, that is reserved in POSIX.
Jakub
