From patchwork Mon Oct 7 18:41:04 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Thomas Schwinge X-Patchwork-Id: 1172943 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-510413-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="uQ5ShmWe"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 46n8TB4jBmz9sPT for ; Tue, 8 Oct 2019 05:41:32 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:in-reply-to:references:date:message-id :mime-version:content-type; q=dns; s=default; b=jiYbWVcgP4E2EsYf 2wY+mn+NdaOYDPMf8trlStmcRFeoRqFDdxcBJIQyCotkKq5lhNAtdqEAHMVobUDO trCxZhwLOyPlc+52TrCeLVARmbMrHa3xUyZDFz1PDZymKAm0lH0ZlS56hyMexoPy XUPwL+JbT3BXejIR33Kg0WLNDq8= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:in-reply-to:references:date:message-id :mime-version:content-type; s=default; bh=1nFFV/nJ52iIIywmtO0ryf LUK8M=; b=uQ5ShmWe8QBdHwpPPJZuHpeVyCwHF/FLAk9nKeZnF3AL5A7t4/+IrN 5JPYSnuZgfOyw8pOhKsqq0ie1SRo3f5+eE/ygaosQ8uBvbyWT+epfUA/7cOFuzhW 8du/VcDgloxrt/naLvsUnpAK3AjXRarzijdd4oeQyOgGt4yQnTs3M= Received: (qmail 59452 invoked by alias); 7 Oct 2019 18:41:23 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 59435 invoked by uid 89); 7 Oct 2019 18:41:23 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.3 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, KAM_STOCKGEN, SPF_PASS autolearn=ham version=3.3.1 spammy=sk:changel, sk:ChangeL, who's, whos X-HELO: esa4.mentor.iphmx.com Received: from esa4.mentor.iphmx.com (HELO esa4.mentor.iphmx.com) (68.232.137.252) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 07 Oct 2019 18:41:17 +0000 IronPort-SDR: 6M5yq5DYovlBnKqH1MgledOo86LN4HU96k1+GvmMGOQ9CSCrhyNHVnV0FbfwFKl0QuhwUTCKr5 7cFEc50JivedlW0e6Sg1FPVSJ+ucHKxe5+TLbyIxGRWj42MvBbMVBnffHziljVWCDAs3U8ah2B OxSjNv/jkIeo1Z7wFDbFGbXCVwxqt5oJd9ugtkzC3nUnlrIAHe3d7GGgGReEoHZJEB2Da7Vi/D TDSzl3bMjPu4qA27uSbqaZHGUHXGaTtBIQ3jKZlW4UyLI4XUpF0DUMv0jO75cR9zw2Th5se82g evY= Received: from orw-gwy-01-in.mentorg.com ([192.94.38.165]) by esa4.mentor.iphmx.com with ESMTP; 07 Oct 2019 10:41:16 -0800 IronPort-SDR: 2IqZ71nsax+Ke8oSnSkp+Tr0edyjpVlple5g/KRufCHNsdgZjeXbMdktIEKe8UAo8uf8SRkMIi /myu6WR3o4oVkBeWw6Mx2aT5KhpWvROR09j6IvQeI+dF+FFADWMCMPLI7u5UDGvIp1sJulJEwP hMM+inZVoFMlatx0PAaA7ZyiHtzvoz2BY51/8W6SbZ/SVjpUDVw12uyMKj6VCS4f0HjPziDCxX 7qEjyqpZ3b4CYL5JD/ul4VBxKZsO7/pTo/C6NYdeDxXuQ0fjlGojicvpoXGARqNVrFOmeKmfEG Uv4= From: Thomas Schwinge To: Jakub Jelinek CC: , Frederik Harwath Subject: Add OpenACC 2.6 `acc_get_property' support In-Reply-To: References: User-Agent: Notmuch/0.29.1+93~g67ed7df (https://notmuchmail.org) Emacs/26.1 (x86_64-pc-linux-gnu) Date: Mon, 7 Oct 2019 20:41:04 +0200 Message-ID: <87imp01jr3.fsf@euler.schwinge.homeip.net> MIME-Version: 1.0 Hi Jakub! On 2018-12-03T16:51:14+0000, "Maciej W. Rozycki" wrote: > Add generic support for the OpenACC 2.6 `acc_get_property' and > `acc_get_property_string' routines [...] ..., which allow for user code to query the implementation for stuff like: > OpenACC vendor: GNU > OpenACC name: GOMP > OpenACC driver: 1.0 > > with the host driver and output like: > > OpenACC vendor: Nvidia > OpenACC total memory: 12651462656 > OpenACC free memory: 12202737664 > OpenACC name: TITAN V > OpenACC driver: 9.1 > > with the NVPTX driver. (With 's%OpenACC%Device%', as I understand this; see my comment below.) Before Frederik starts working on integrating this into GCC trunk, do you (Jakub) agree with the libgomp plugin interface changes as implemented by Maciej? For example, top-level 'GOMP_OFFLOAD_get_property' function in 'struct gomp_device_descr' instead of stuffing this into its 'acc_dispatch_t openacc'. (I never understood why the OpenACC functions need to be segregated like they are.) For reference I'm attaching the latest version of the patch, that is the commit from openacc-gcc-9-branch, plus a small fix-up. And, some first remarks (or, "thinking aloud", not a conclusive review) while I have a look at this myself: > --- a/include/gomp-constants.h > +++ b/include/gomp-constants.h > @@ -215,10 +215,24 @@ enum gomp_map_kind > #define GOMP_DEVICE_NVIDIA_PTX 5 > #define GOMP_DEVICE_INTEL_MIC 6 > #define GOMP_DEVICE_HSA 7 > +#define GOMP_DEVICE_CURRENT 8 This is used for 'acc_device_current', relevant only for 'acc_get_property', to return "the value of the property for the current device". This should probably use a more special (negative?) value instead of eight, so that when additional real device types are added later on, we can just add them with increasing numbers, and keep the scanning code simple. (Use of 'acc_device_current' as an argument to other functions taking an 'acc_device_t' is undefined, and should be rejected with 'gomp_fatal'?) Also, 'acc_device_current' is a libgomp-internal thing (doesn't interface with the compiler proper), so strictly speaking 'GOMP_DEVICE_CURRENT' isn't needed in 'include/gomp-constants.h'. But probably still a good idea to list it there, in this canonical place, to keep the several lists of device types coherent. (I have not checked how 'acc_device_current' is actually implemented in the following.) > +/* Device property codes. Keep in sync with > + libgomp/{openacc.h,openacc.f90,openacc_lib.h}:acc_device_property_t > + as well as libgomp/libgomp-plugin.h. */ Same thing, libgomp-internal, not sure whether to list these here? > +/* Start from 1 to catch uninitialized use. */ Hmm, not sure about that either. Don't think we're generally doing that? (But I see PGI have 'acc_property_none = 0', oh well.) > +#define GOMP_DEVICE_PROPERTY_MEMORY 1 > +#define GOMP_DEVICE_PROPERTY_FREE_MEMORY 2 > +#define GOMP_DEVICE_PROPERTY_NAME 0x10001 > +#define GOMP_DEVICE_PROPERTY_VENDOR 0x10002 > +#define GOMP_DEVICE_PROPERTY_DRIVER 0x10003 > + > +/* Internal property mask to tell numeric and string values apart. */ > +#define GOMP_DEVICE_PROPERTY_STRING_MASK 0x10000 > --- a/libgomp/plugin/plugin-nvptx.c > +++ b/libgomp/plugin/plugin-nvptx.c > +union gomp_device_property_value > +GOMP_OFFLOAD_get_property (int n, int prop) > +{ > + union gomp_device_property_value propval = { .val = 0 }; > + > + pthread_mutex_lock (&ptx_dev_lock); > + > + if (!nvptx_init () || n >= nvptx_get_num_devices ()) > + { > + pthread_mutex_unlock (&ptx_dev_lock); > + return propval; > + } Isn't it implicit that 'get_num_devices' has been called while loading the plugin, so we don't have to do any initialization that here? (But I may be misremembering that.) > + switch (prop) > + { > + case GOMP_DEVICE_PROPERTY_MEMORY: > + { > + size_t total_mem; > + CUdevice dev; > + > + CUDA_CALL_ERET (propval, cuDeviceGet, &dev, n); Isn't that already known as 'ptx_devices[n]'? (Likewise elsewhere.) > + CUDA_CALL_ERET (propval, cuDeviceTotalMem, &total_mem, dev); > + propval.val = total_mem; > + } > + break; > + case GOMP_DEVICE_PROPERTY_NAME: > + { > + static char name[256]; > + CUdevice dev; > + > + CUDA_CALL_ERET (propval, cuDeviceGet, &dev, n); > + CUDA_CALL_ERET (propval, cuDeviceGetName, name, sizeof (name), dev); > + propval.ptr = name; > + } > + break; Uh, that's not thread-safe, is it? From a quick look at OpenACC 2.7, that doesn't specify what exactly to return here (that is, which "class" of memory; who's the "owner" of the memory object). (So, returning a 'malloc'ed object would be a memory leak, for example.) Best would probably be to return some 'const char *' living in read-only program memory. (But that likely is not available, otherwise I suppose Maciej would have done that.) Otherwise, perhaps make this 'name' a property of 'struct ptx_device' in the nvptx plugin here, and keep it live while the device is open ('nvptx_open_device'), together with other per-device data? > + case GOMP_DEVICE_PROPERTY_DRIVER: > + { > + static char ver[11]; > + int v; > + > + CUDA_CALL_ERET (propval, cuDriverGetVersion, &v); > + snprintf (ver, sizeof (ver) - 1, "%u.%u", v / 1000, v % 1000 / 10); > + propval.ptr = ver; > + } > + break; Similar here. Is that 'snprintf' formatting the generic way to display a CUDA driver version number? As, in theory, such Nvidia GPU offloading support could also be implemented via the Nouveau/Mesa GalliumCompute driver, should the string returned here actually include "CUDA Driver"? > + default: > + break; Should this 'GOMP_PLUGIN_error' or even 'GOMP_PLUGIN_fatal'? (Similar then elsewhere.) > --- /dev/null > +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc-get-property.c > @@ -0,0 +1,37 @@ > +/* Test the `acc_get_property' and '`acc_get_property_string' library > + functions. */ > +/* { dg-do run } */ > + > +#include > +#include > +#include > +#include > + > +int main () > +{ > + const char *s; > + size_t v; > + int r; > + > + /* Verify that the vendor is a proper non-empty string. */ > + s = acc_get_property_string (0, acc_device_default, acc_property_vendor); > + r = !s || !strlen (s); > + if (s) > + printf ("OpenACC vendor: %s\n", s); Should we check the actual string returned, as defined by OpenACC/our implementation, as applicable? Use '#if defined ACC_DEVICE_TYPE_[...]'. (See 'libgomp/testsuite/libgomp.oacc-c-c++-common/avoid-offloading-2.c', for example.) Isn't this the "Device vendor" instead of the "OpenACC vendor"? Similar for all other 'printf's? > + > + /* For the rest just check that they do not crash. */ > + v = acc_get_property (0, acc_device_default, acc_property_memory); > + if (v) > + printf ("OpenACC total memory: %zd\n", v); > + v = acc_get_property (0, acc_device_default, acc_property_free_memory); > + if (v) > + printf ("OpenACC free memory: %zd\n", v); > + s = acc_get_property_string (0, acc_device_default, acc_property_name); > + if (s) > + printf ("OpenACC name: %s\n", s); > + s = acc_get_property_string (0, acc_device_default, acc_property_driver); > + if (s) > + printf ("OpenACC driver: %s\n", s); > + > + return r; > +} Likewise, as we define the implementation, we can declare that something reasonable must have been returned, so don't need 'if (s)' checks, but should instead verify 's' to the extent possible. > --- /dev/null > +++ b/libgomp/testsuite/libgomp.oacc-fortran/acc-get-property.f Similar. (See 'libgomp/testsuite/libgomp.oacc-fortran/avoid-offloading-2.f', for example.) These tests only use 'acc_device_default', should they also check other valid as well as invalid values? Grüße Thomas From 1fa609ba73e9990ae7a65b083047f0ee219167b3 Mon Sep 17 00:00:00 2001 From: Thomas Schwinge Date: Tue, 8 Jan 2019 15:21:35 +0100 Subject: [PATCH] Add OpenACC 2.6 `acc_get_property' support: restore Intel MIC offloading The "OpenACC 2.6 `acc_get_property' support" changes regressed the relevant libgomp OpenMP execution test cases to no longer consider Intel MIC offloading because of: libgomp: while loading libgomp-plugin-intelmic.so.1: [...]/libgomp-plugin-intelmic.so.1: undefined symbol: GOMP_OFFLOAD_get_property liboffloadmic/ * plugin/libgomp-plugin-intelmic.cpp (GOMP_OFFLOAD_get_property): New function. --- liboffloadmic/ChangeLog.openacc | 5 +++++ .../plugin/libgomp-plugin-intelmic.cpp | 21 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/liboffloadmic/ChangeLog.openacc b/liboffloadmic/ChangeLog.openacc index 521d4490f0b..75f8ee518e4 100644 --- a/liboffloadmic/ChangeLog.openacc +++ b/liboffloadmic/ChangeLog.openacc @@ -1,3 +1,8 @@ +2019-01-08 Thomas Schwinge + + * plugin/libgomp-plugin-intelmic.cpp (GOMP_OFFLOAD_get_property): + New function. + 2019-02-26 Chung-Lin Tang * plugin/libgomp-plugin-intelmic.cpp (GOMP_OFFLOAD_version): diff --git a/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp b/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp index d1678d0514e..ed78c8d9dd4 100644 --- a/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp +++ b/liboffloadmic/plugin/libgomp-plugin-intelmic.cpp @@ -174,6 +174,27 @@ GOMP_OFFLOAD_get_num_devices (void) return num_devices; } +extern "C" union gomp_device_property_value +GOMP_OFFLOAD_get_property (int n, int prop) +{ + union gomp_device_property_value nullval = { .val = 0 }; + + if (n >= num_devices) + { + GOMP_PLUGIN_error + ("Request for a property of a non-existing Intel MIC device %i", n); + return nullval; + } + + switch (prop) + { + case GOMP_DEVICE_PROPERTY_VENDOR: + return (union gomp_device_property_value) { .ptr = /* TODO: "error: invalid conversion from 'const void*' to 'void*' [-fpermissive]" */ (char *) "Intel" }; + default: + return nullval; + } +} + static bool offload (const char *file, uint64_t line, int device, const char *name, int num_vars, VarDesc *vars, const void **async_data) -- 2.17.1