Re: [PATCH] Add OpenACC acc_get_property support for AMD GCN

[Andrew Stubbs <ams@codesourcery.com>]

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