From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8588 invoked by alias); 28 Jan 2020 15:42:17 -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 8579 invoked by uid 89); 28 Jan 2020 15:42:16 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-6.4 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.1 spammy=H*f:sk:0f59690, H*i:sk:0f59690, H*MI:sk:0f59690 X-HELO: esa1.mentor.iphmx.com Received: from esa1.mentor.iphmx.com (HELO esa1.mentor.iphmx.com) (68.232.129.153) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 28 Jan 2020 15:42:15 +0000 IronPort-SDR: 3BKPq5KjcUeezEUxGryqIIrIsdcJ5tfqh/u83tum1a1XIH/CMPyM0gXApTCi1Ubo9H6xzTLirE 7k2Q5r0kMc6LJM3HXSWHFi1Nac9oF/QWMC1439KeNzuoPBu5pQf2t2qnC9IfSX22ObPP2ctxN/ bEVkyar0EmGYL4TPQtEUBDEtqGTx/YDTZRE9uQGG9cppW0UdUS/ufoAoJrRHH5a41aRMFRZswE r/muDjvjgZlkDDJp/r2L6jiby4p7DsjZJBoCpC9zVZ8uPCdM9JCWNJ5V0HVxt8mT9MgjDQfv3p fA4= Received: from orw-gwy-01-in.mentorg.com ([192.94.38.165]) by esa1.mentor.iphmx.com with ESMTP; 28 Jan 2020 07:42:13 -0800 IronPort-SDR: gZuVhPOX0BfgdJddS8mxPgbhlEH4c6VKjKeMcw3/nLFYfTyvMnXnsW83iipQiMZBYB1YOOQJi8 zryYvDCxhS7g== Subject: Re: [PATCH] Add OpenACC acc_get_property support for AMD GCN To: "Harwath, Frederik" , GCC Patches , Thomas Schwinge CC: Jakub Jelinek References: <0f59690f-5247-9b6e-169a-9a6ef16c41e1@codesourcery.com> From: Andrew Stubbs Message-ID: <472b4749-7752-1527-c59c-1ab348854f32@codesourcery.com> Date: Tue, 28 Jan 2020 16:14:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <0f59690f-5247-9b6e-169a-9a6ef16c41e1@codesourcery.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Return-Path: ams@codesourcery.com X-SW-Source: 2020-01/txt/msg01861.txt.bz2 On 28/01/2020 14:55, Harwath, Frederik wrote: > Hi, > this patch adds full support for the OpenACC 2.6 acc_get_property and > acc_get_property_string functions to the libgomp GCN plugin. This replaces > the existing stub in libgomp/plugin-gcn.c. > > Andrew: The value returned for acc_property_memory ("size of device memory in bytes" > according to the spec) is the HSA_REGION_INFO_SIZE of the agent's data_region. This > has been adapted from a previous incomplete implementation that we had on the OG9 branch. > Does that sound reasonable? I don't think we can do better than that. > I have tested the patch with amdgcn and nvptx offloading. > > Ok to commit this to the main branch? > > > Best regards, > Frederik > > @@ -544,6 +547,8 @@ struct hsa_context_info > int agent_count; > /* Array of agent_info structures describing the individual HSA agents. */ > struct agent_info *agents; > + /* Driver version string. */ > + char driver_version_s[30]; > }; > > /* Format of the on-device heap. > @@ -1513,6 +1518,15 @@ init_hsa_context (void) > GOMP_PLUGIN_error ("Failed to list all HSA runtime agents"); > } > > + uint16_t minor, major; > + status = hsa_fns.hsa_system_get_info_fn (HSA_SYSTEM_INFO_VERSION_MINOR, &minor); > + if (status != HSA_STATUS_SUCCESS) > + GOMP_PLUGIN_error ("Failed to obtain HSA runtime minor version"); > + status = hsa_fns.hsa_system_get_info_fn (HSA_SYSTEM_INFO_VERSION_MAJOR, &major); > + if (status != HSA_STATUS_SUCCESS) > + GOMP_PLUGIN_error ("Failed to obtain HSA runtime major version"); > + sprintf (hsa_context.driver_version_s, "HSA Runtime %d.%d", major, minor); > + > hsa_context.initialized = true; > return true; > } If we're going to use a fixed-size buffer then we should use snprintf and emit GCN_WARNING if the return value is greater than "sizeof(driver_version_s)", even though that is unlikely. Do the same in the testcase, but use a bigger buffer so that truncation causes a mismatch and test failure. > @@ -75,6 +56,39 @@ void expect_device_properties > "but was %s.\n", s); > abort (); > } > +} > + > +void expect_device_memory > +(acc_device_t dev_type, int dev_num, size_t expected_total_memory) > +{ > + > + I realise that an existing function in this testcase uses this layout, but the code style does not normally have the parameter list on the next line, and certainly not in column 1. > + > +int main() > +{ Space before (). Thanks Andrew