From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 67595 invoked by alias); 18 Dec 2018 12:36:10 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 67572 invoked by uid 89); 18 Dec 2018 12:36:09 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-9.2 required=5.0 tests=BAYES_50,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=fond, Hx-languages-length:3096, GOMP_VERSION, 4626 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 18 Dec 2018 12:36:06 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3922813268C; Tue, 18 Dec 2018 12:36:05 +0000 (UTC) Received: from tucnak.zalov.cz (ovpn-117-214.ams2.redhat.com [10.36.117.214]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A2F7F67142; Tue, 18 Dec 2018 12:36:04 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.15.2/8.15.2) with ESMTP id wBICa2i6016023; Tue, 18 Dec 2018 13:36:02 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.15.2/8.15.2/Submit) id wBICa0Kl016022; Tue, 18 Dec 2018 13:36:00 +0100 Date: Tue, 18 Dec 2018 12:36:00 -0000 From: Jakub Jelinek To: Thomas Schwinge Cc: Chung-Lin Tang , gcc-patches@gcc.gnu.org Subject: Re: [PATCH 1/6, OpenACC, libgomp] Async re-work, interfaces Message-ID: <20181218123600.GJ23305@tucnak> Reply-To: Jakub Jelinek References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2018-12/txt/msg01293.txt.bz2 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