Re: [PATCH] Avoid registering unsupported OMP offload devices

[Andrew Stubbs <ams@baylibre.com>]

On 26/01/2024 14:21, Richard Biener wrote:
> On Fri, 26 Jan 2024, Jakub Jelinek wrote:
> 
>> On Fri, Jan 26, 2024 at 03:04:11PM +0100, Richard Biener wrote:
>>>>> Otherwise it looks reasoanble to me, but let's see what Andrew thinks.
>>>>
>>>> 'n' before 'a', please. ;-)
>>>
>>> ?!
>>
>> I've misspelled a word.
>>
>>> @@ -1443,6 +1445,16 @@ suitable_hsa_agent_p (hsa_agent_t agent)
>>>     switch (device_type)
>>>       {
>>>       case HSA_DEVICE_TYPE_GPU:
>>> +      {
>>> +	char name[64] = "nil";
>>> +	if ((hsa_fns.hsa_agent_get_info_fn (agent, HSA_AGENT_INFO_NAME, name)
>>> +	     != HSA_STATUS_SUCCESS)
>>> +	    || isa_code (name) == EF_AMDGPU_MACH_UNSUPPORTED)
>>> +	  {
>>> +	    GCN_DEBUG ("Ignoring unsupported agent '%s'\n", name);
>>> +	    return false;
>>> +	  }
>>
>> I must say I know nothing about HSA libraries, but generally if a function
>> that is supposed to fill some buffer fails the content of the buffer is
>> undefined/unpredictable.
>> So might be better not to not initialize name before calling the function
>> (unless it has to be initialized) and strcpy it to nil or something similar
>> if it fails.
> 
> Yeah, sorry.  Here's a proper engineered variant.  I don't expect that
> function to ever fail of course.

I keep crossing emails with you today. :(

This version looks good to me. It has to work for all versions of ROCm 
from maybe 3.8 (the last version known to work with the Fiji devices) 
onwards to forever.

OK.

Andrew

>  From 445891ba57e858d980441bd63249e3bc94632db3 Mon Sep 17 00:00:00 2001
> From: Richard Biener <rguenther@suse.de>
> Date: Fri, 26 Jan 2024 12:57:10 +0100
> Subject: [PATCH] Avoid registering unsupported OMP offload devices
> To: gcc-patches@gcc.gnu.org
> 
> The following avoids registering unsupported GCN offload devices
> when iterating over available ones.  With a Zen4 desktop CPU
> you will have an IGPU (unspported) which will otherwise be made
> available.  This causes testcases like
> libgomp.c-c++-common/non-rect-loop-1.c which iterate over all
> decives to FAIL.
> 
> OK?
> 
> libgomp/
> 	* plugin/plugin-gcn.c (suitable_hsa_agent_p): Filter out
> 	agents with unsupported ISA.
> ---
>   libgomp/plugin/plugin-gcn.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/libgomp/plugin/plugin-gcn.c b/libgomp/plugin/plugin-gcn.c
> index 588358bbbf9..2771123252a 100644
> --- a/libgomp/plugin/plugin-gcn.c
> +++ b/libgomp/plugin/plugin-gcn.c
> @@ -1427,6 +1427,8 @@ init_hsa_runtime_functions (void)
>   #undef DLSYM_FN
>   }
>   
> +static gcn_isa isa_code (const char *isa);
> +
>   /* Return true if the agent is a GPU and can accept of concurrent submissions
>      from different threads.  */
>   
> @@ -1443,6 +1445,18 @@ suitable_hsa_agent_p (hsa_agent_t agent)
>     switch (device_type)
>       {
>       case HSA_DEVICE_TYPE_GPU:
> +      {
> +	char name[64];
> +	hsa_status_t status
> +	  = hsa_fns.hsa_agent_get_info_fn (agent, HSA_AGENT_INFO_NAME, name);
> +	if (status != HSA_STATUS_SUCCESS
> +	    || isa_code (name) == EF_AMDGPU_MACH_UNSUPPORTED)
> +	  {
> +	    GCN_DEBUG ("Ignoring unsupported agent '%s'\n",
> +		       status == HSA_STATUS_SUCCESS ? name : "invalid");
> +	    return false;
> +	  }
> +      }
>         break;
>       case HSA_DEVICE_TYPE_CPU:
>         if (!support_cpu_devices)