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

[Andrew Stubbs <ams@codesourcery.com>]

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