| Message ID | 0f59690f-5247-9b6e-169a-9a6ef16c41e1@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add OpenACC acc_get_property support for AMD GCN | expand |
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
Hi Andrew, On 28.01.20 16:42, Andrew Stubbs wrote: > On 28/01/2020 14:55, Harwath, Frederik wrote: > > 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. Ok. > 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. Ok. I have also adjusted the formatting in the other acc_get_property tests to the code style. I have turned this into a separate trivial patch. Ok to commit the revised patch? Best regards, Frederik
On 29/01/2020 09:52, Harwath, Frederik wrote: > @@ -1513,6 +1518,23 @@ 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"); > + > + size_t len = sizeof hsa_context.driver_version_s; > + int printed = snprintf (hsa_context.driver_version_s, len, > + "HSA Runtime %hu.%hu", (unsigned short int)major, > + (unsigned short int)minor); > + if (printed >= len) > + GCN_WARNING ("HSA runtime version string was truncated." > + "Version %hu.%hu is too long.", (unsigned short int)major, > + (unsigned short int)minor); > + > hsa_context.initialized = true; > return true; > } Please wrap the long lines (I should have spotted this before, sorry). The buffer checking is good, thank you. > +void expect_device_string_properties (acc_device_t dev_type, int dev_num, > + const char* expected_vendor, > + const char* expected_name, > + const char* expected_driver) > + GNU style would be ... void expect_device_string_properties (acc_device_t dev_type, int dev_num, const char* expected_vendor, const char* expected_name, const char* expected_driver) That is, with the return type on the previous line, and the function name starting in the first column. The prototype should have the type on the same line, however. (I think the point is that you should be able to find a function definition with "grep '^name' *".) Otherwise you have the parameter list correct now. Patch 1 is OK with the formatting fixed. Patch 2 is OK. Thanks very much, Andrew
Hi Andrew, On 29.01.20 11:38, Andrew Stubbs wrote: > On 29/01/2020 09:52, Harwath, Frederik wrote: > > Patch 1 is OK with the formatting fixed. > Patch 2 is OK. > > Thanks very much, > Committed as 2e5ea57959183bd5bd0356739bb5167417401a31 and 87c3fcfa6bbb5c372d4e275276d21f601d0b62b0. Thank you for the review, Frederik
Hi! On 2020-01-29T10:52:57+0100, "Harwath, Frederik" <frederik@codesourcery.com> wrote: > On 28.01.20 16:42, Andrew Stubbs wrote: >> On 28/01/2020 14:55, Harwath, Frederik wrote: >> >> 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. > --- a/libgomp/plugin/plugin-gcn.c > +++ b/libgomp/plugin/plugin-gcn.c > @@ -425,7 +425,10 @@ struct agent_info > > /* The instruction set architecture of the device. */ > gcn_isa device_isa; > - > + /* Name of the agent. */ > + char name[64]; > + /* Name of the vendor of the agent. */ > + char vendor_name[64]; > /* Command queues of the agent. */ > hsa_queue_t *sync_queue; > struct goacc_asyncqueue *async_queues, *omp_async_queue; > @@ -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]; > }; > @@ -1513,6 +1518,23 @@ init_hsa_context (void) > + size_t len = sizeof hsa_context.driver_version_s; > + int printed = snprintf (hsa_context.driver_version_s, len, > + "HSA Runtime %hu.%hu", (unsigned short int)major, > + (unsigned short int)minor); > + if (printed >= len) > + GCN_WARNING ("HSA runtime version string was truncated." > + "Version %hu.%hu is too long.", (unsigned short int)major, > + (unsigned short int)minor); (Can it actually happen that 'snprintf' returns 'printed > len' -- meaning that it's written into random memory? I thought 'snprintf' has a hard stop at 'len'? Or does this indicate the amount of memory it would've written? I should re-read the manpage at some point...) ;-) For 'printed = len' does or doesn't 'snprintf' store the terminating 'NUL' character, or do we manually have to set: hsa_context.driver_version_s[len - 1] = '\0'; ... in that case? > @@ -3410,15 +3432,19 @@ GOMP_OFFLOAD_init_device (int n) > - char buf[64]; > status = hsa_fns.hsa_agent_get_info_fn (agent->id, HSA_AGENT_INFO_NAME, > - &buf); > + &agent->name); > if (status != HSA_STATUS_SUCCESS) > return hsa_error ("Error querying the name of the agent", status); (That's of course pre-existing, but) this looks like a dangerous API, given that 'hsa_agent_get_info_fn' doesn't know 'sizeof agent->name' (or 'sizeof buf' before)... > + status = hsa_fns.hsa_agent_get_info_fn (agent->id, HSA_AGENT_INFO_VENDOR_NAME, > + &agent->vendor_name); > + if (status != HSA_STATUS_SUCCESS) > + return hsa_error ("Error querying the vendor name of the agent", status); Similar here. > --- a/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property.c > +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property.c > @@ -3,8 +3,7 @@ > of all device types mentioned in the OpenACC standard. > > See also acc_get_property.f90. */ > -/* { dg-do run { target { { ! { openacc_host_selected } } && { ! { openacc_amdgcn_accel_selected } } } } } */ > -/* FIXME: This test does not work with the GCN implementation stub yet. */ (I had wondered why 'host' was disabled here; now I don't need to wonder any longer.) ;-) > +/* { dg-do run } */ This one is not actually needed, is the default. (But no reason to remove it.) > --- a/libgomp/testsuite/libgomp.oacc-fortran/acc_get_property.f90 > +++ b/libgomp/testsuite/libgomp.oacc-fortran/acc_get_property.f90 > @@ -3,8 +3,6 @@ > ! of all device types mentioned in the OpenACC standard. > ! > ! See also acc_get_property.c > -! { dg-do run { target { { ! { openacc_host_selected } } && { ! { openacc_amdgcn_accel_selected } } } } } > -! FIXME: This test does not work with the GCN implementation stub yet. ..., but here, with 'dg-do run' removed, this is no longer doing "torture testing" (cycle through optimization levels/flags). (See the rationale in 'libgomp/testsuite/libgomp.oacc-fortran/fortran.exp'.) ... but that should actually be OK given that we're really only testing the 'acc_get_property'/'acc_get_property_string' API calls, no other Fortran extravaganza. Grüße Thomas
On 29/01/2020 17:44, Thomas Schwinge wrote: >> @@ -1513,6 +1518,23 @@ init_hsa_context (void) > >> + size_t len = sizeof hsa_context.driver_version_s; >> + int printed = snprintf (hsa_context.driver_version_s, len, >> + "HSA Runtime %hu.%hu", (unsigned short int)major, >> + (unsigned short int)minor); >> + if (printed >= len) >> + GCN_WARNING ("HSA runtime version string was truncated." >> + "Version %hu.%hu is too long.", (unsigned short int)major, >> + (unsigned short int)minor); > > (Can it actually happen that 'snprintf' returns 'printed > len' -- > meaning that it's written into random memory? I thought 'snprintf' has a > hard stop at 'len'? Or does this indicate the amount of memory it > would've written? I should re-read the manpage at some point...) ;-) snprintf returns the length it would have been, if not truncated. > For 'printed = len' does or doesn't 'snprintf' store the terminating > 'NUL' character, or do we manually have to set: > > hsa_context.driver_version_s[len - 1] = '\0'; > > ... in that case? It does the right thing already, hence why printed == len is an overflow: the last character will be missing. > >> @@ -3410,15 +3432,19 @@ GOMP_OFFLOAD_init_device (int n) > >> - char buf[64]; >> status = hsa_fns.hsa_agent_get_info_fn (agent->id, HSA_AGENT_INFO_NAME, >> - &buf); >> + &agent->name); >> if (status != HSA_STATUS_SUCCESS) >> return hsa_error ("Error querying the name of the agent", status); > > (That's of course pre-existing, but) this looks like a dangerous API, > given that 'hsa_agent_get_info_fn' doesn't know 'sizeof agent->name' (or > 'sizeof buf' before)... Those ones are written into the HSA documentation. They are fixed-sized fields, so there must be 64 bytes regardless of the actual name of the agent. >> + status = hsa_fns.hsa_agent_get_info_fn (agent->id, HSA_AGENT_INFO_VENDOR_NAME, >> + &agent->vendor_name); >> + if (status != HSA_STATUS_SUCCESS) >> + return hsa_error ("Error querying the vendor name of the agent", status); > > Similar here. Same reason. >> --- a/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property.c >> +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property.c >> @@ -3,8 +3,7 @@ >> of all device types mentioned in the OpenACC standard. >> >> See also acc_get_property.f90. */ >> -/* { dg-do run { target { { ! { openacc_host_selected } } && { ! { openacc_amdgcn_accel_selected } } } } } */ >> -/* FIXME: This test does not work with the GCN implementation stub yet. */ > > (I had wondered why 'host' was disabled here; now I don't need to wonder > any longer.) ;-) > >> +/* { dg-do run } */ > > This one is not actually needed, is the default. (But no reason to > remove it.) > >> --- a/libgomp/testsuite/libgomp.oacc-fortran/acc_get_property.f90 >> +++ b/libgomp/testsuite/libgomp.oacc-fortran/acc_get_property.f90 >> @@ -3,8 +3,6 @@ >> ! of all device types mentioned in the OpenACC standard. >> ! >> ! See also acc_get_property.c >> -! { dg-do run { target { { ! { openacc_host_selected } } && { ! { openacc_amdgcn_accel_selected } } } } } >> -! FIXME: This test does not work with the GCN implementation stub yet. > > ..., but here, with 'dg-do run' removed, this is no longer doing "torture > testing" (cycle through optimization levels/flags). (See the rationale > in 'libgomp/testsuite/libgomp.oacc-fortran/fortran.exp'.) > > ... but that should actually be OK given that we're really only testing > the 'acc_get_property'/'acc_get_property_string' API calls, no other > Fortran extravaganza. Thanks for the additional review, Thomas. Andrew
Hi Thomas, On 29.01.20 18:44, Thomas Schwinge wrote: >> + size_t len = sizeof hsa_context.driver_version_s; >> + int printed = snprintf (hsa_context.driver_version_s, len, >> + "HSA Runtime %hu.%hu", (unsigned short int)major, >> + (unsigned short int)minor); >> + if (printed >= len) >> + GCN_WARNING ("HSA runtime version string was truncated." >> + "Version %hu.%hu is too long.", (unsigned short int)major, >> + (unsigned short int)minor); > > (Can it actually happen that 'snprintf' returns 'printed > len' -- > meaning that it's written into random memory? I thought 'snprintf' has a > hard stop at 'len'? Or does this indicate the amount of memory it > would've written? I should re-read the manpage at some point...) ;-) > Yes, "printed > len" can happen. Seems that I have chosen a bad variable name. "actual_len" (of the formatted string that should have been written - excluding the terminating '\0') would have been more appropriate. > For 'printed = len' does or doesn't 'snprintf' store the terminating > 'NUL' character, or do we manually have to set: > > hsa_context.driver_version_s[len - 1] = '\0'; > > ... in that case? No, in this case, the printed string is missing the last character, but the terminating '\0' has been written. Consider: #include <stdio.h> int main () { char s[] = "foo"; char buf[3]; // buf is too short to hold terminating '\0' int actual_len = snprintf (buf, 3, "%s", s); printf ("buf: %s\n", buf); printf ("actual_len: %d\n", actual_len); } Output: buf: fo actual_len: 3 > >> @@ -3410,15 +3432,19 @@ GOMP_OFFLOAD_init_device (int n) > >> - char buf[64]; >> status = hsa_fns.hsa_agent_get_info_fn (agent->id, HSA_AGENT_INFO_NAME, >> - &buf); >> + &agent->name); >> if (status != HSA_STATUS_SUCCESS) >> return hsa_error ("Error querying the name of the agent", status); > > (That's of course pre-existing, but) this looks like a dangerous API, > given that 'hsa_agent_get_info_fn' doesn't know 'sizeof agent->name' (or > 'sizeof buf' before)... The API documentation (cf. https://rocm-documentation.readthedocs.io/en/latest/ROCm_API_References/ROCr-API.html) states that "the type of this attribute is a NUL-terminated char[64]". But, right, should this ever change, we might not notice it. Best regards, Frederik
Hi! Andrew and Frederik, thanks for your emails reminding/educating me about 'snprintf' as well as this HSA fixed-size buffer API. There doesn't happen to be something available in the HSA API available so that we could use 'sizeof [something]' instead of hard-coding '64' etc.? I understand correctly that the only reason for: On 2020-01-29T10:52:57+0100, "Harwath, Frederik" <frederik@codesourcery.com> wrote: > * testsuite/libgomp.oacc-c-c++-common/acc_get_property-aux.c > (expect_device_properties): Split function into ... > (expect_device_string_properties): ... this new function ... > (expect_device_memory): ... and this new function. ... this split is that we can't test 'expect_device_memory' here: > * testsuite/libgomp.oacc-c-c++-common/acc_get_property-gcn.c: > Add test. ..., because that one doesn't (re-)implement the 'acc_property_memory' interface? > --- a/libgomp/plugin/plugin-gcn.c > +++ b/libgomp/plugin/plugin-gcn.c > @@ -4115,12 +4141,37 @@ GOMP_OFFLOAD_openacc_async_dev2host (int device, void *dst, const void *src, > union goacc_property_value > GOMP_OFFLOAD_openacc_get_property (int device, enum goacc_property prop) > { > [...] > + switch (prop) > + { > + case GOACC_PROPERTY_FREE_MEMORY: > + /* Not supported. */ > + break; (OK, can be added later when somebody feels like doing that.) > + case GOACC_PROPERTY_MEMORY: > + { > + size_t size; > + hsa_region_t region = agent->data_region; > + hsa_status_t status = > + hsa_fns.hsa_region_get_info_fn (region, HSA_REGION_INFO_SIZE, &size); > + if (status == HSA_STATUS_SUCCESS) > + propval.val = size; > + break; > + } > [...] > } Here we got 'acc_property_memory' implemented, but not here: > --- /dev/null > +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-gcn.c > @@ -0,0 +1,132 @@ > +/* Test the `acc_get_property' and `acc_get_property_string' library > + functions on amdgcn devices by comparing property values with > + those obtained through the HSA API. */ > +/* { dg-additional-sources acc_get_property-aux.c } */ > +/* { dg-additional-options "-ldl" } */ > +/* { dg-do run { target openacc_amdgcn_accel_selected } } */ > + > +#include <dlfcn.h> > +#include <stdint.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include <string.h> > +#include <openacc.h> > + > +#ifndef __cplusplus > +typedef int bool; > +#endif > +#include <hsa.h> > + > + > +void expect_device_string_properties (acc_device_t dev_type, int dev_num, > + const char* expected_vendor, > + const char* expected_name, > + const char* expected_driver); > + > +hsa_status_t (*hsa_agent_get_info_fn) (hsa_agent_t agent, > + hsa_agent_info_t attribute, > + void *value); > +hsa_status_t (*hsa_system_get_info_fn) (hsa_system_info_t attribute, > + void *value); > +hsa_status_t (*hsa_iterate_agents_fn) > +(hsa_status_t (*callback)(hsa_agent_t agent, void *data), void *data); > +hsa_status_t (*hsa_init_fn) (void); > + > +char* support_cpu_devices; > + > +void test_setup () > +{ > + char* env_runtime; > + char* hsa_runtime_lib; > + void *handle; > + > +#define DLSYM_FN(function) \ > + function##_fn = (typeof(function##_fn))dlsym (handle, #function); \ > + if (function##_fn == NULL) \ > + { \ > + fprintf (stderr, "Could not get symbol " #function ".\n"); \ > + abort (); \ > + } > + > + env_runtime = getenv ("HSA_RUNTIME_LIB"); > + hsa_runtime_lib = env_runtime ? env_runtime : (char*)"libhsa-runtime64.so"; > + > + handle = dlopen (hsa_runtime_lib, RTLD_LAZY); > + if (!handle) > + { > + fprintf (stderr, "Could not load %s.\n", hsa_runtime_lib); > + abort (); > + } > + > + DLSYM_FN (hsa_agent_get_info) > + DLSYM_FN (hsa_system_get_info) > + DLSYM_FN (hsa_iterate_agents) > + DLSYM_FN (hsa_init) > + > + hsa_init_fn (); > + > + support_cpu_devices = getenv ("GCN_SUPPORT_CPU_DEVICES"); > +} > + > +static hsa_status_t check_agent_properties (hsa_agent_t agent, void *dev_num_arg) > +{ > + > + char name[64]; > + char vendor_name[64]; > + uint16_t minor; > + uint16_t major; > + char driver[60]; > + > + hsa_status_t status; > + hsa_device_type_t device_type; > + int* dev_num = (int*)dev_num_arg; > + > +#define AGENT_GET_INFO(info_type, val) \ > + status = hsa_agent_get_info_fn (agent, info_type, &val); \ > + if (status != HSA_STATUS_SUCCESS) \ > + { \ > + fprintf (stderr, "Failed to obtain " #info_type ".\n"); \ > + abort (); \ > + } > +#define SYSTEM_GET_INFO(info_type, val) \ > + status = hsa_system_get_info_fn (info_type, &val); \ > + if (status != HSA_STATUS_SUCCESS) \ > + { \ > + fprintf (stderr, "Failed to obtain " #info_type ".\n"); \ > + abort (); \ > + } > + > + AGENT_GET_INFO (HSA_AGENT_INFO_DEVICE, device_type) > + > + /* Skip unsupported device types. Mimic the GCN plugin's behavior. */ > + if (!(device_type == HSA_DEVICE_TYPE_GPU > + || (support_cpu_devices && device_type == HSA_DEVICE_TYPE_CPU))) > + return HSA_STATUS_SUCCESS; > + > + AGENT_GET_INFO (HSA_AGENT_INFO_NAME, name) > + AGENT_GET_INFO (HSA_AGENT_INFO_VENDOR_NAME, vendor_name) > + > + SYSTEM_GET_INFO (HSA_SYSTEM_INFO_VERSION_MINOR, minor) > + SYSTEM_GET_INFO (HSA_SYSTEM_INFO_VERSION_MAJOR, major) > + > + snprintf (driver, sizeof driver, "HSA Runtime %hu.%hu", > + (unsigned short int)major, (unsigned short int)minor); > + > + expect_device_string_properties(acc_device_radeon, *dev_num, > + vendor_name, name, driver); > + > + (*dev_num)++; > + > + return status; > +} > + > +int main () > +{ > + int dev_num = 0; > + test_setup (); > + > + hsa_status_t status = > + hsa_iterate_agents_fn (&check_agent_properties, &dev_num); > + > + return status; > +} Grüße Thomas
On 30/01/2020 16:08, Thomas Schwinge wrote: > Hi! > > Andrew and Frederik, thanks for your emails reminding/educating me about > 'snprintf' as well as this HSA fixed-size buffer API. There doesn't > happen to be something available in the HSA API available so that we > could use 'sizeof [something]' instead of hard-coding '64' etc.? No, not at present; hsa_agent_get_info_fn is a somewhat generic interface that takes an enum and returns a void*. The return type is written in the documentation: https://rocm-documentation.readthedocs.io/en/latest/ROCm_API_References/ROCr-API.html#rocr-api However, we don't use the official ROCm header files, because dependencies and licenses, so we could invent our own typedefs in hsa.h, if we chose. I don't see that doing so would be worth the effort now, or maintenance burden later. I'll let Frederik explain his implementation decisions. Andrew
Hi! On 2020-01-30T16:45:39+0000, Andrew Stubbs <ams@codesourcery.com> wrote: > On 30/01/2020 16:08, Thomas Schwinge wrote: >> Andrew and Frederik, thanks for your emails reminding/educating me about >> 'snprintf' as well as this HSA fixed-size buffer API. There doesn't >> happen to be something available in the HSA API available so that we >> could use 'sizeof [something]' instead of hard-coding '64' etc.? > > No, not at present; hsa_agent_get_info_fn is a somewhat generic > interface that takes an enum and returns a void*. The return type is > written in the documentation: > https://rocm-documentation.readthedocs.io/en/latest/ROCm_API_References/ROCr-API.html#rocr-api > > However, we don't use the official ROCm header files, because > dependencies and licenses, so we could invent our own typedefs in hsa.h, > if we chose. I don't see that doing so would be worth the effort now, or > maintenance burden later. ACK. I was just curious if there's an easy way to properly resolve this "magic numbers" issue now, once and for all. Apparently, there isn't -- which shall be fine, then. Grüße Thomas
Hi Thomas, On 30.01.20 17:08, Thomas Schwinge wrote: > I understand correctly that the only reason for: > > On 2020-01-29T10:52:57+0100, "Harwath, Frederik" <frederik@codesourcery.com> wrote: >> * testsuite/libgomp.oacc-c-c++-common/acc_get_property-aux.c >> (expect_device_properties): Split function into ... >> (expect_device_string_properties): ... this new function ... >> (expect_device_memory): ... and this new function. > > ... this split is that we can't test 'expect_device_memory' here: > [...] > ..., because that one doesn't (re-)implement the 'acc_property_memory' > interface? Correct. But why "re-"? It has not been implemented before. >> --- a/libgomp/plugin/plugin-gcn.c >> +++ b/libgomp/plugin/plugin-gcn.c > >> @@ -4115,12 +4141,37 @@ GOMP_OFFLOAD_openacc_async_dev2host (int device, void *dst, const void *src, >> union goacc_property_value >> GOMP_OFFLOAD_openacc_get_property (int device, enum goacc_property prop) >> { >> [...] >> + switch (prop) >> + { >> + case GOACC_PROPERTY_FREE_MEMORY: >> + /* Not supported. */ >> + break; > > (OK, can be added later when somebody feels like doing that.) Well, "not supported" means that there seems to be no (reasonable) way to obtain the necessary information from the runtime - in contrast to the nvptx plugin where it can be obtained easily through the CUDA API. > >> + case GOACC_PROPERTY_MEMORY: >> + { >> + size_t size; >> + hsa_region_t region = agent->data_region; >> + hsa_status_t status = >> + hsa_fns.hsa_region_get_info_fn (region, HSA_REGION_INFO_SIZE, &size); >> + if (status == HSA_STATUS_SUCCESS) >> + propval.val = size; >> + break; >> + } >> [...] >> } > > Here we got 'acc_property_memory' implemented, but not here: > >> --- /dev/null >> +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-gcn.c Yes, there seems to be no straightforward way to determine the expected value through the runtime API. We might of course try to replicate the logic that is used in plugin-gcn.c. Best regards, Frederik
Hi Frederik! On 2020-01-31T13:17:52+0100, "Harwath, Frederik" <frederik@codesourcery.com> wrote: > On 30.01.20 17:08, Thomas Schwinge wrote: > >> I understand correctly that the only reason for: >> >> On 2020-01-29T10:52:57+0100, "Harwath, Frederik" <frederik@codesourcery.com> wrote: >>> * testsuite/libgomp.oacc-c-c++-common/acc_get_property-aux.c >>> (expect_device_properties): Split function into ... >>> (expect_device_string_properties): ... this new function ... >>> (expect_device_memory): ... and this new function. >> >> ... this split is that we can't test 'expect_device_memory' here: > >> [...] > >> ..., because that one doesn't (re-)implement the 'acc_property_memory' >> interface? > > Correct. OK, thanks for confirming. > But why "re-"? It has not been implemented before. Well, yes, it has already been implemented: in 'libgomp/plugin/plugin-gcn.c' ;-) -- that's why I meant it's re-implemented in the test case(s): in a similar yet slightly different way compared to the respective plugin(s). And for avoidance of doubt: I agree that's a good approach to verify that we're getting some consistent and meaningful results.) >>> --- a/libgomp/plugin/plugin-gcn.c >>> +++ b/libgomp/plugin/plugin-gcn.c >>> + case GOACC_PROPERTY_MEMORY: >>> + { >>> + size_t size; >>> + hsa_region_t region = agent->data_region; >>> + hsa_status_t status = >>> + hsa_fns.hsa_region_get_info_fn (region, HSA_REGION_INFO_SIZE, &size); >>> + if (status == HSA_STATUS_SUCCESS) >>> + propval.val = size; >>> + break; >>> + } >>> [...] >>> } >> >> Here we got 'acc_property_memory' implemented, but not here: >> >>> --- /dev/null >>> +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-gcn.c > > Yes, there seems to be no straightforward way to determine the expected value through > the runtime API. We might of course try to replicate the logic that is > used in plugin-gcn.c. No need to do that now; I was just curious whether that's the reason. >>> --- a/libgomp/plugin/plugin-gcn.c >>> +++ b/libgomp/plugin/plugin-gcn.c >> >>> @@ -4115,12 +4141,37 @@ GOMP_OFFLOAD_openacc_async_dev2host (int device, void *dst, const void *src, >>> union goacc_property_value >>> GOMP_OFFLOAD_openacc_get_property (int device, enum goacc_property prop) >>> { >>> [...] >>> + switch (prop) >>> + { >>> + case GOACC_PROPERTY_FREE_MEMORY: >>> + /* Not supported. */ >>> + break; >> >> (OK, can be added later when somebody feels like doing that.) > > Well, "not supported" means that there seems to be no (reasonable) way to obtain > the necessary information from the runtime - in contrast to the nvptx plugin > where it can be obtained easily through the CUDA API. OK, I see, and again that's fine. (The API explicitly permits such "zero" returns.) And, I don't know what a user is actually supposed to tell from 'acc_property_free_memory' given that this can change at any point in time, arbitrarily, not under control of the executing processe, given that multiple processes may be using the GPU concurrently. Grüße Thomas
From 6f1855281c38993a088f9b4af020a786f8e05fe9 Mon Sep 17 00:00:00 2001 From: Frederik Harwath <frederik@codesourcery.com> Date: Tue, 28 Jan 2020 08:01:00 +0100 Subject: [PATCH] Add OpenACC acc_get_property support for AMD GCN Add full support for the OpenACC 2.6 acc_get_property and acc_get_property_string functions to the libgomp GCN plugin. libgomp/ * plugin-gcn.c (struct agent_info): Add fields "name" and "vendor_name" ... (GOMP_OFFLOAD_init_device): ... and init from here. (struct hsa_context_info): Add field "driver_version_s" ... (init_hsa_contest): ... and init from here. (GOMP_OFFLOAD_openacc_get_property): Replace stub with a proper implementation. * testsuite/libgomp.oacc-c-c++-common/acc_get_property.c: Enable test execution for amdgcn and host offloading targets. * testsuite/libgomp.oacc-fortran/acc_get_property.f90: Likewise. * testsuite/libgomp.oacc-c-c++-common/acc_get_property-aux.c (expect_device_properties): Split function into ... (expect_device_string_properties): ... this new function ... (expect_device_memory): ... and this new function. * testsuite/libgomp.oacc-c-c++-common/acc_get_property-gcn.c: Add test. --- libgomp/plugin/plugin-gcn.c | 63 +++++++-- .../acc_get_property-aux.c | 60 +++++--- .../acc_get_property-gcn.c | 132 ++++++++++++++++++ .../acc_get_property.c | 5 +- .../libgomp.oacc-fortran/acc_get_property.f90 | 2 - 5 files changed, 224 insertions(+), 38 deletions(-) create mode 100644 libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-gcn.c diff --git a/libgomp/plugin/plugin-gcn.c b/libgomp/plugin/plugin-gcn.c index 7854c142f05..0a09daaa0a4 100644 --- a/libgomp/plugin/plugin-gcn.c +++ b/libgomp/plugin/plugin-gcn.c @@ -425,7 +425,10 @@ struct agent_info /* The instruction set architecture of the device. */ gcn_isa device_isa; - + /* Name of the agent. */ + char name[64]; + /* Name of the vendor of the agent. */ + char vendor_name[64]; /* Command queues of the agent. */ hsa_queue_t *sync_queue; struct goacc_asyncqueue *async_queues, *omp_async_queue; @@ -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; } @@ -3410,15 +3424,19 @@ GOMP_OFFLOAD_init_device (int n) return hsa_error ("Error requesting maximum queue size of the GCN agent", status); - char buf[64]; status = hsa_fns.hsa_agent_get_info_fn (agent->id, HSA_AGENT_INFO_NAME, - &buf); + &agent->name); if (status != HSA_STATUS_SUCCESS) return hsa_error ("Error querying the name of the agent", status); - agent->device_isa = isa_code (buf); + agent->device_isa = isa_code (agent->name); if (agent->device_isa < 0) - return hsa_error ("Unknown GCN agent architecture.", HSA_STATUS_ERROR); + return hsa_error ("Unknown GCN agent architecture", HSA_STATUS_ERROR); + + status = hsa_fns.hsa_agent_get_info_fn (agent->id, HSA_AGENT_INFO_VENDOR_NAME, + &agent->vendor_name); + if (status != HSA_STATUS_SUCCESS) + return hsa_error ("Error querying the vendor name of the agent", status); status = hsa_fns.hsa_queue_create_fn (agent->id, queue_size, HSA_QUEUE_TYPE_MULTI, @@ -4115,12 +4133,37 @@ GOMP_OFFLOAD_openacc_async_dev2host (int device, void *dst, const void *src, union goacc_property_value GOMP_OFFLOAD_openacc_get_property (int device, enum goacc_property prop) { - /* Stub. Check device and return default value for unsupported properties. */ - /* TODO: Implement this function. */ - get_agent_info (device); + struct agent_info *agent = get_agent_info (device); + + union goacc_property_value propval = { .val = 0 }; + + switch (prop) + { + case GOACC_PROPERTY_FREE_MEMORY: + /* Not supported. */ + break; + case GOACC_PROPERTY_MEMORY: + { + size_t size; + hsa_region_t region = agent->data_region; + hsa_status_t status = + hsa_fns.hsa_region_get_info_fn (region, HSA_REGION_INFO_SIZE, &size); + if (status == HSA_STATUS_SUCCESS) + propval.val = size; + break; + } + case GOACC_PROPERTY_NAME: + propval.ptr = agent->name; + break; + case GOACC_PROPERTY_VENDOR: + propval.ptr = agent->vendor_name; + break; + case GOACC_PROPERTY_DRIVER: + propval.ptr = hsa_context.driver_version_s; + break; + } - union goacc_property_value nullval = { .val = 0 }; - return nullval; + return propval; } /* Set up plugin-specific thread-local-data (host-side). */ diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-aux.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-aux.c index 6bb01250148..6a4a499eb21 100644 --- a/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-aux.c +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-aux.c @@ -6,10 +6,11 @@ #include <stdio.h> #include <string.h> -void expect_device_properties + +void expect_device_string_properties (acc_device_t dev_type, int dev_num, - size_t expected_memory, const char* expected_vendor, - const char* expected_name, const char* expected_driver) + const char* expected_vendor, const char* expected_name, + const char* expected_driver) { const char *vendor = acc_get_property_string (dev_num, dev_type, acc_property_vendor); @@ -20,26 +21,6 @@ void expect_device_properties abort (); } - size_t total_mem = acc_get_property (dev_num, dev_type, - acc_property_memory); - if (total_mem != expected_memory) - { - fprintf (stderr, "Expected acc_property_memory to equal %zu, " - "but was %zu.\n", expected_memory, total_mem); - abort (); - - } - - size_t free_mem = acc_get_property (dev_num, dev_type, - acc_property_free_memory); - if (free_mem > total_mem) - { - fprintf (stderr, "Expected acc_property_free_memory <= acc_property_memory" - ", but free memory was %zu and total memory was %zu.\n", - free_mem, total_mem); - abort (); - } - const char *name = acc_get_property_string (dev_num, dev_type, acc_property_name); if (strcmp (name, expected_name)) @@ -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) +{ + + size_t total_mem = acc_get_property (dev_num, dev_type, + acc_property_memory); + + if (total_mem != expected_total_memory) + { + fprintf (stderr, "Expected acc_property_memory to equal %zu, " + "but was %zu.\n", expected_total_memory, total_mem); + abort (); + } + size_t free_mem = acc_get_property (dev_num, dev_type, + acc_property_free_memory); + if (free_mem > total_mem) + { + fprintf (stderr, "Expected acc_property_free_memory <= acc_property_memory" + ", but free memory was %zu and total memory was %zu.\n", + free_mem, total_mem); + abort (); + } +} +void expect_device_properties +(acc_device_t dev_type, int dev_num, + size_t expected_total_memory, const char* expected_vendor, + const char* expected_name, const char* expected_driver) +{ + expect_device_string_properties (dev_type, dev_num, expected_vendor, + expected_name, expected_driver); + expect_device_memory (dev_type, dev_num, expected_total_memory); } diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-gcn.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-gcn.c new file mode 100644 index 00000000000..305bb1af365 --- /dev/null +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-gcn.c @@ -0,0 +1,132 @@ +/* Test the `acc_get_property' and `acc_get_property_string' library + functions on amdgcn devices by comparing property values with + those obtained through the HSA API. */ +/* { dg-additional-sources acc_get_property-aux.c } */ +/* { dg-additional-options "-ldl" } */ +/* { dg-do run { target openacc_amdgcn_accel_selected } } */ + +#include <dlfcn.h> +#include <stdint.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <openacc.h> + +#ifndef __cplusplus +typedef int bool; +#endif +#include <hsa.h> + + +void expect_device_string_properties +(acc_device_t dev_type, int dev_num, + const char* expected_vendor, const char* expected_name, + const char* expected_driver); + +hsa_status_t (*hsa_agent_get_info_fn) (hsa_agent_t agent, + hsa_agent_info_t attribute, + void *value); +hsa_status_t (*hsa_system_get_info_fn) (hsa_system_info_t attribute, + void *value); +hsa_status_t (*hsa_iterate_agents_fn) +(hsa_status_t (*callback)(hsa_agent_t agent, void *data), void *data); +hsa_status_t (*hsa_init_fn) (void); + +char* support_cpu_devices; + +void test_setup () +{ + char* env_runtime; + char* hsa_runtime_lib; + void *handle; + +#define DLSYM_FN(function) \ + function##_fn = (typeof(function##_fn))dlsym (handle, #function); \ + if (function##_fn == NULL) \ + { \ + fprintf (stderr, "Could not get symbol " #function ".\n"); \ + abort (); \ + } + + env_runtime = getenv ("HSA_RUNTIME_LIB"); + hsa_runtime_lib = env_runtime ? env_runtime : (char*)"libhsa-runtime64.so"; + + handle = dlopen (hsa_runtime_lib, RTLD_LAZY); + if (!handle) + { + fprintf (stderr, "Could not load %s.\n", hsa_runtime_lib); + abort (); + } + + DLSYM_FN (hsa_agent_get_info) + DLSYM_FN (hsa_system_get_info) + DLSYM_FN (hsa_iterate_agents) + DLSYM_FN (hsa_init) + + hsa_init_fn (); + + support_cpu_devices = getenv ("GCN_SUPPORT_CPU_DEVICES"); +} + +static hsa_status_t check_agent_properties +(hsa_agent_t agent, void *dev_num_arg) +{ + + char name[64]; + char vendor_name[64]; + uint16_t minor; + uint16_t major; + char driver[30]; + + hsa_status_t status; + hsa_device_type_t device_type; + int* dev_num = (int*)dev_num_arg; + +#define AGENT_GET_INFO(info_type, val) \ + status = hsa_agent_get_info_fn (agent, info_type, &val); \ + if (status != HSA_STATUS_SUCCESS) \ + { \ + fprintf (stderr, "Failed to obtain " #info_type ".\n"); \ + abort (); \ + } +#define SYSTEM_GET_INFO(info_type, val) \ + status = hsa_system_get_info_fn (info_type, &val); \ + if (status != HSA_STATUS_SUCCESS) \ + { \ + fprintf (stderr, "Failed to obtain " #info_type ".\n"); \ + abort (); \ + } + + AGENT_GET_INFO (HSA_AGENT_INFO_DEVICE, device_type) + + /* Skip unsupported device types. Mimic the GCN plugin's behavior. */ + if (!(device_type == HSA_DEVICE_TYPE_GPU + || (support_cpu_devices && device_type == HSA_DEVICE_TYPE_CPU))) + return HSA_STATUS_SUCCESS; + + AGENT_GET_INFO (HSA_AGENT_INFO_NAME, name) + AGENT_GET_INFO (HSA_AGENT_INFO_VENDOR_NAME, vendor_name) + + SYSTEM_GET_INFO (HSA_SYSTEM_INFO_VERSION_MINOR, minor) + SYSTEM_GET_INFO (HSA_SYSTEM_INFO_VERSION_MAJOR, major) + + sprintf (driver, "HSA Runtime %d.%d", major, minor); + + expect_device_string_properties(acc_device_radeon, *dev_num, + vendor_name, name, driver); + + (*dev_num)++; + + return status; +} + +int main() +{ + int dev_num = 0; + test_setup (); + + hsa_status_t status = + hsa_iterate_agents_fn (&check_agent_properties, &dev_num); + + return status; +} diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property.c index 388c66c1319..1984ad3a28d 100644 --- a/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property.c +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property.c @@ -3,8 +3,7 @@ of all device types mentioned in the OpenACC standard. See also acc_get_property.f90. */ -/* { dg-do run { target { { ! { openacc_host_selected } } && { ! { openacc_amdgcn_accel_selected } } } } } */ -/* FIXME: This test does not work with the GCN implementation stub yet. */ +/* { dg-do run } */ #include <openacc.h> #include <stdlib.h> @@ -20,7 +19,7 @@ print_device_properties(acc_device_t type) const char *s; size_t v; - int dev_count = acc_get_num_devices(type); + int dev_count = acc_get_num_devices (type); for (int i = 0; i < dev_count; ++i) { diff --git a/libgomp/testsuite/libgomp.oacc-fortran/acc_get_property.f90 b/libgomp/testsuite/libgomp.oacc-fortran/acc_get_property.f90 index ce695475ae4..80ae292f41f 100644 --- a/libgomp/testsuite/libgomp.oacc-fortran/acc_get_property.f90 +++ b/libgomp/testsuite/libgomp.oacc-fortran/acc_get_property.f90 @@ -3,8 +3,6 @@ ! of all device types mentioned in the OpenACC standard. ! ! See also acc_get_property.c -! { dg-do run { target { { ! { openacc_host_selected } } && { ! { openacc_amdgcn_accel_selected } } } } } -! FIXME: This test does not work with the GCN implementation stub yet. program test use openacc -- 2.17.1
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 have tested the patch with amdgcn and nvptx offloading. Ok to commit this to the main branch? Best regards, Frederik