Add OpenACC acc_get_property support for AMD GCN

Message ID 0f59690f-5247-9b6e-169a-9a6ef16c41e1@codesourcery.com
State New
Headers show
Series
  • Add OpenACC acc_get_property support for AMD GCN
Related show

Commit Message

Frederik Harwath Jan. 28, 2020, 2:55 p.m.
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

Comments

Andrew Stubbs Jan. 28, 2020, 3:42 p.m. | #1
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
Frederik Harwath Jan. 29, 2020, 9:52 a.m. | #2
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
From fb15cb9058feeda8891d6454d32f43fda885b789 Mon Sep 17 00:00:00 2001
From: Frederik Harwath <frederik@codesourcery.com>

Date: Wed, 29 Jan 2020 10:19:50 +0100
Subject: [PATCH 1/2] 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                   |  71 ++++++++--
 .../acc_get_property-aux.c                    |  79 ++++++-----
 .../acc_get_property-gcn.c                    | 132 ++++++++++++++++++
 .../acc_get_property.c                        |   5 +-
 .../libgomp.oacc-fortran/acc_get_property.f90 |   2 -
 5 files changed, 242 insertions(+), 47 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..45c625495b9 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,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;
 }
@@ -3410,15 +3432,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 +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)
 {
-  /* 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..f43f75b364a 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,39 +6,20 @@
 #include <stdio.h>
 #include <string.h>
 
-void expect_device_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 *vendor = acc_get_property_string (dev_num, dev_type,
-						acc_property_vendor);
-  if (strcmp (vendor, expected_vendor))
-    {
-      fprintf (stderr, "Expected acc_property_vendor to equal \"%s\", "
-	       "but was \"%s\".\n", expected_vendor, vendor);
-      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 ();
-    }
+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)
+{
+const char *vendor = acc_get_property_string (dev_num, dev_type,
+					    acc_property_vendor);
+if (strcmp (vendor, expected_vendor))
+{
+    fprintf (stderr, "Expected acc_property_vendor to equal \"%s\", "
+	    "but was \"%s\".\n", expected_vendor, vendor);
+    abort ();
+}
 
   const char *name = acc_get_property_string (dev_num, dev_type,
 					      acc_property_name);
@@ -75,6 +56,40 @@ 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..059b503bbf6
--- /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;
+}
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
From fd9cdadbbada9ec7f0f108991937455952df7697 Mon Sep 17 00:00:00 2001
From: Frederik Harwath <frederik@codesourcery.com>

Date: Wed, 29 Jan 2020 10:21:18 +0100
Subject: [PATCH 2/2] Adjust formatting of acc_get_property tests

libgomp/
 * testsuite/libgomp.oacc-c-c++-common/acc_get_property-host.c:
 Adjust to GNU coding style.
 * testsuite/libgomp.oacc-c-c++-common/acc_get_property-nvptx.c: Likewise.
 * testsuite/libgomp.oacc-c-c++-common/acc_get_property.c: Likewise.
---
 .../acc_get_property-host.c                   | 11 +++++-----
 .../acc_get_property-nvptx.c                  | 21 ++++++++++---------
 .../acc_get_property.c                        | 20 +++++++++---------
 3 files changed, 27 insertions(+), 25 deletions(-)

diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-host.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-host.c
index f1cd7cfef39..bb80f3f52a3 100644
--- a/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-host.c
+++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-host.c
@@ -6,12 +6,13 @@
 #include <openacc.h>
 #include <stdio.h>
 
-void expect_device_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);
+void expect_device_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);
 
-int main()
+int main ()
 {
   printf ("Checking acc_device_host device properties\n");
   expect_device_properties (acc_device_host, 0, 0, "GNU", "GOMP", "1.0");
diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-nvptx.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-nvptx.c
index 0dcaea7c3e8..3e1b51036bc 100644
--- a/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-nvptx.c
+++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property-nvptx.c
@@ -11,10 +11,11 @@
 #include <string.h>
 #include <stdio.h>
 
-void expect_device_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);
+void expect_device_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);
 
 int main ()
 {
@@ -29,26 +30,26 @@ int main ()
 	  abort ();
 	}
 
-      printf("Checking device %d\n", dev_num);
+      printf ("Checking device %d\n", dev_num);
 
       const char *vendor = "Nvidia";
       size_t free_mem;
       size_t total_mem;
-      if (cudaMemGetInfo(&free_mem, &total_mem) != cudaSuccess)
+      if (cudaMemGetInfo (&free_mem, &total_mem) != cudaSuccess)
 	{
 	  fprintf (stderr, "cudaMemGetInfo failed.\n");
 	  abort ();
 	}
 
       struct cudaDeviceProp p;
-      if (cudaGetDeviceProperties(&p, dev_num) != cudaSuccess)
+      if (cudaGetDeviceProperties (&p, dev_num) != cudaSuccess)
 	{
 	  fprintf (stderr, "cudaGetDeviceProperties failed.\n");
 	  abort ();
 	}
 
       int driver_version;
-      if (cudaDriverGetVersion(&driver_version) != cudaSuccess)
+      if (cudaDriverGetVersion (&driver_version) != cudaSuccess)
 	{
 	  fprintf (stderr, "cudaDriverGetVersion failed.\n");
 	  abort ();
@@ -63,7 +64,7 @@ int main ()
 
       /* Note that this check relies on the fact that the device numbering
 	 used by the nvptx plugin agrees with the CUDA device ordering. */
-      expect_device_properties(acc_device_nvidia, dev_num,
-			       total_mem, vendor, p.name, driver);
+      expect_device_properties (acc_device_nvidia, dev_num,
+				total_mem, vendor, p.name, driver);
     }
 }
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 1984ad3a28d..79b67084852 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
@@ -14,7 +14,7 @@
    and do basic device independent validation. */
 
 void
-print_device_properties(acc_device_t type)
+print_device_properties (acc_device_t type)
 {
   const char *s;
   size_t v;
@@ -23,7 +23,7 @@ print_device_properties(acc_device_t type)
 
   for (int i = 0; i < dev_count; ++i)
     {
-      printf("  Device %d:\n", i+1);
+      printf ("  Device %d:\n", i+1);
 
       s = acc_get_property_string (i, type, acc_property_vendor);
       printf ("    Vendor: %s\n", s);
@@ -59,17 +59,17 @@ print_device_properties(acc_device_t type)
 
 int main ()
 {
-  printf("acc_device_none:\n");
+  printf ("acc_device_none:\n");
   /* For completness; not expected to print anything since there
      should be no devices of this type. */
-  print_device_properties(acc_device_none);
+  print_device_properties (acc_device_none);
 
-  printf("acc_device_default:\n");
-  print_device_properties(acc_device_default);
+  printf ("acc_device_default:\n");
+  print_device_properties (acc_device_default);
 
-  printf("acc_device_host:\n");
-  print_device_properties(acc_device_host);
+  printf ("acc_device_host:\n");
+  print_device_properties (acc_device_host);
 
-  printf("acc_device_not_host:\n");
-  print_device_properties(acc_device_not_host);
+  printf ("acc_device_not_host:\n");
+  print_device_properties (acc_device_not_host);
 }
-- 
2.17.1
Andrew Stubbs Jan. 29, 2020, 10:38 a.m. | #3
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
Frederik Harwath Jan. 29, 2020, 11:04 a.m. | #4
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
Thomas Schwinge Jan. 29, 2020, 5:44 p.m. | #5
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
Andrew Stubbs Jan. 29, 2020, 5:58 p.m. | #6
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
Frederik Harwath Jan. 30, 2020, 7:09 a.m. | #7
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
Thomas Schwinge Jan. 30, 2020, 4:08 p.m. | #8
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
Andrew Stubbs Jan. 30, 2020, 4:45 p.m. | #9
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
Thomas Schwinge Jan. 31, 2020, 9:30 a.m. | #10
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
Frederik Harwath Jan. 31, 2020, 12:17 p.m. | #11
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
Thomas Schwinge Jan. 31, 2020, 2:25 p.m. | #12
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

Patch

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