public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* Re: [PATCH] Avoid registering unsupported OMP offload devices
       [not found] <20240126120143.6978F3858C62@sourceware.org>
@ 2024-01-26 12:06 ` Jakub Jelinek
  2024-01-26 12:30   ` Tobias Burnus
  2024-01-26 13:39   ` Andrew Stubbs
  0 siblings, 2 replies; 10+ messages in thread
From: Jakub Jelinek @ 2024-01-26 12:06 UTC (permalink / raw)
  To: Richard Biener; +Cc: gcc-patches, ams

On Fri, Jan 26, 2024 at 01:00:28PM +0100, Richard Biener wrote:
> 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.
> 
> I'll run a bootstrap with both pending changes and will do
> another round of full libgomp testing with this.
> 
> OK if that succeeds?
> 
> Thanks,
> Richard.
> 
> libgomp/
> 	* plugin/plugin-gcn.c (suitable_hsa_agent_p): Filter out
> 	agents with unsupported ISA.
> ---
>  libgomp/plugin/plugin-gcn.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/libgomp/plugin/plugin-gcn.c b/libgomp/plugin/plugin-gcn.c
> index 588358bbbf9..88ed77ff049 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);

Space before ( please.

> +
>  /* Return true if the agent is a GPU and can accept of concurrent submissions
>     from different threads.  */
>  
> @@ -1443,6 +1445,13 @@ suitable_hsa_agent_p (hsa_agent_t agent)
>    switch (device_type)
>      {
>      case HSA_DEVICE_TYPE_GPU:
> +      {
> +	char name[64];
> +	if ((hsa_fns.hsa_agent_get_info_fn (agent, HSA_AGENT_INFO_NAME, name)
> +	     != HSA_STATUS_SUCCESS)
> +	    || isa_code (name) == EF_AMDGPU_MACH_UNSUPPORTED)
> +	  return false;
> +      }
>        break;
>      case HSA_DEVICE_TYPE_CPU:
>        if (!support_cpu_devices)

Otherwise it looks reasoanble to me, but let's see what Andrew thinks.

	Jakub


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Avoid registering unsupported OMP offload devices
  2024-01-26 12:06 ` [PATCH] Avoid registering unsupported OMP offload devices Jakub Jelinek
@ 2024-01-26 12:30   ` Tobias Burnus
  2024-01-26 12:32     ` Richard Biener
  2024-01-26 13:39   ` Andrew Stubbs
  1 sibling, 1 reply; 10+ messages in thread
From: Tobias Burnus @ 2024-01-26 12:30 UTC (permalink / raw)
  To: Jakub Jelinek, Richard Biener; +Cc: gcc-patches, ams

Jakub Jelinek wrote:
> On Fri, Jan 26, 2024 at 01:00:28PM +0100, Richard Biener wrote:
>> libgomp/
>> 	* plugin/plugin-gcn.c (suitable_hsa_agent_p): Filter out
>> 	agents with unsupported ISA.
...
>> @@ -1443,6 +1445,13 @@ suitable_hsa_agent_p (hsa_agent_t agent)
>>     switch (device_type)
>>       {
>>       case HSA_DEVICE_TYPE_GPU:
>> +      {
>> +	char name[64];
>> +	if ((hsa_fns.hsa_agent_get_info_fn (agent, HSA_AGENT_INFO_NAME, name)
>> +	     != HSA_STATUS_SUCCESS)
>> +	    || isa_code (name) == EF_AMDGPU_MACH_UNSUPPORTED)
>> +	  return false;
>> +      }
>>         break;

I wonder whether we want to add a diagnostic for this or not - like:
GCN_DEBUG ("Ignoring unsupported GPU %s", name)? Or is this more 
confusing than helpful?

>>       case HSA_DEVICE_TYPE_CPU:
>>         if (!support_cpu_devices)
> 
> Otherwise it looks reasoanble to me, but let's see what Andrew thinks.

Likewise: looks reasonable but I also would wait for Andrew.

Tobias

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Avoid registering unsupported OMP offload devices
  2024-01-26 12:30   ` Tobias Burnus
@ 2024-01-26 12:32     ` Richard Biener
  0 siblings, 0 replies; 10+ messages in thread
From: Richard Biener @ 2024-01-26 12:32 UTC (permalink / raw)
  To: Tobias Burnus; +Cc: Jakub Jelinek, gcc-patches, ams

On Fri, 26 Jan 2024, Tobias Burnus wrote:

> Jakub Jelinek wrote:
> > On Fri, Jan 26, 2024 at 01:00:28PM +0100, Richard Biener wrote:
> >> libgomp/
> >>  * plugin/plugin-gcn.c (suitable_hsa_agent_p): Filter out
> >>  agents with unsupported ISA.
> ...
> >> @@ -1443,6 +1445,13 @@ suitable_hsa_agent_p (hsa_agent_t agent)
> >>     switch (device_type)
> >>       {
> >>       case HSA_DEVICE_TYPE_GPU:
> >> +      {
> >> +	char name[64];
> >> +	if ((hsa_fns.hsa_agent_get_info_fn (agent, HSA_AGENT_INFO_NAME, name)
> >> +	     != HSA_STATUS_SUCCESS)
> >> +	    || isa_code (name) == EF_AMDGPU_MACH_UNSUPPORTED)
> >> +	  return false;
> >> +      }
> >>         break;
> 
> I wonder whether we want to add a diagnostic for this or not - like:
> GCN_DEBUG ("Ignoring unsupported GPU %s", name)? Or is this more confusing
> than helpful?

I can do that, but there's later reasons it might be ignored as well,
so doing this in the caller might be better (you'd get duplicate
diagnostic as well, once for counting once for registering).  I figured
this might better be done as followup?

Richard.

> >>       case HSA_DEVICE_TYPE_CPU:
> >>         if (!support_cpu_devices)
> > 
> > Otherwise it looks reasoanble to me, but let's see what Andrew thinks.
> 
> Likewise: looks reasonable but I also would wait for Andrew.
> 
> Tobias
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Avoid registering unsupported OMP offload devices
  2024-01-26 12:06 ` [PATCH] Avoid registering unsupported OMP offload devices Jakub Jelinek
  2024-01-26 12:30   ` Tobias Burnus
@ 2024-01-26 13:39   ` Andrew Stubbs
  2024-01-26 14:04     ` Richard Biener
  1 sibling, 1 reply; 10+ messages in thread
From: Andrew Stubbs @ 2024-01-26 13:39 UTC (permalink / raw)
  To: Jakub Jelinek, Richard Biener; +Cc: gcc-patches

On 26/01/2024 12:06, Jakub Jelinek wrote:
> On Fri, Jan 26, 2024 at 01:00:28PM +0100, Richard Biener wrote:
>> 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.
>>
>> I'll run a bootstrap with both pending changes and will do
>> another round of full libgomp testing with this.
>>
>> OK if that succeeds?
>>
>> Thanks,
>> Richard.
>>
>> libgomp/
>> 	* plugin/plugin-gcn.c (suitable_hsa_agent_p): Filter out
>> 	agents with unsupported ISA.
>> ---
>>   libgomp/plugin/plugin-gcn.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/libgomp/plugin/plugin-gcn.c b/libgomp/plugin/plugin-gcn.c
>> index 588358bbbf9..88ed77ff049 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);
> 
> Space before ( please.
> 
>> +
>>   /* Return true if the agent is a GPU and can accept of concurrent submissions
>>      from different threads.  */
>>   
>> @@ -1443,6 +1445,13 @@ suitable_hsa_agent_p (hsa_agent_t agent)
>>     switch (device_type)
>>       {
>>       case HSA_DEVICE_TYPE_GPU:
>> +      {
>> +	char name[64];
>> +	if ((hsa_fns.hsa_agent_get_info_fn (agent, HSA_AGENT_INFO_NAME, name)
>> +	     != HSA_STATUS_SUCCESS)
>> +	    || isa_code (name) == EF_AMDGPU_MACH_UNSUPPORTED)
>> +	  return false;
>> +      }
>>         break;
>>       case HSA_DEVICE_TYPE_CPU:
>>         if (!support_cpu_devices)
> 
> Otherwise it looks reasoanble to me, but let's see what Andrew thinks.

'n' before 'a', please. ;-)

I think we need at least a GCN_DEBUG message when we ignore a GPU 
device. Possibly gomp_debug also.

Andrew

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Avoid registering unsupported OMP offload devices
  2024-01-26 13:39   ` Andrew Stubbs
@ 2024-01-26 14:04     ` Richard Biener
  2024-01-26 14:11       ` Jakub Jelinek
  2024-01-26 14:22       ` Andrew Stubbs
  0 siblings, 2 replies; 10+ messages in thread
From: Richard Biener @ 2024-01-26 14:04 UTC (permalink / raw)
  To: Andrew Stubbs; +Cc: Jakub Jelinek, gcc-patches

On Fri, 26 Jan 2024, Andrew Stubbs wrote:

> On 26/01/2024 12:06, Jakub Jelinek wrote:
> > On Fri, Jan 26, 2024 at 01:00:28PM +0100, Richard Biener wrote:
> >> 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.
> >>
> >> I'll run a bootstrap with both pending changes and will do
> >> another round of full libgomp testing with this.
> >>
> >> OK if that succeeds?
> >>
> >> Thanks,
> >> Richard.
> >>
> >> libgomp/
> >>  * plugin/plugin-gcn.c (suitable_hsa_agent_p): Filter out
> >>  agents with unsupported ISA.
> >> ---
> >>   libgomp/plugin/plugin-gcn.c | 9 +++++++++
> >>   1 file changed, 9 insertions(+)
> >>
> >> diff --git a/libgomp/plugin/plugin-gcn.c b/libgomp/plugin/plugin-gcn.c
> >> index 588358bbbf9..88ed77ff049 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);
> > 
> > Space before ( please.
> > 
> >> +
> >>   /* Return true if the agent is a GPU and can accept of concurrent
> >>   submissions
> >>      from different threads.  */
> >>   @@ -1443,6 +1445,13 @@ suitable_hsa_agent_p (hsa_agent_t agent)
> >>     switch (device_type)
> >>       {
> >>       case HSA_DEVICE_TYPE_GPU:
> >> +      {
> >> +	char name[64];
> >> +	if ((hsa_fns.hsa_agent_get_info_fn (agent, HSA_AGENT_INFO_NAME, name)
> >> +	     != HSA_STATUS_SUCCESS)
> >> +	    || isa_code (name) == EF_AMDGPU_MACH_UNSUPPORTED)
> >> +	  return false;
> >> +      }
> >>         break;
> >>       case HSA_DEVICE_TYPE_CPU:
> >>         if (!support_cpu_devices)
> > 
> > Otherwise it looks reasoanble to me, but let's see what Andrew thinks.
> 
> 'n' before 'a', please. ;-)

?!

> I think we need at least a GCN_DEBUG message when we ignore a GPU device.
> Possibly gomp_debug also.

Like the following?  This will do

GCN debug: HSA run-time initialized for GCN
GCN debug: HSA_SYSTEM_INFO_ENDIANNESS: LITTLE
GCN debug: HSA_SYSTEM_INFO_EXTENSIONS: IMAGES
GCN debug: Ignoring unsupported agent 'gfx1036'
GCN debug: There are 1 GCN GPU devices.
GCN debug: Ignoring unsupported agent 'gfx1036'
GCN debug: HSA_AGENT_INFO_NAME: AMD Ryzen 9 7900X 12-Core Processor
...

for debug it's probably not too imporant to say this twice.

That said, no idea how to do gomp_debug.

OK?

Thanks,
Richard.


From 7462a8ac70aeebc231c65456b9060d8cbf7d4c50 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.

libgomp/
	* plugin/plugin-gcn.c (suitable_hsa_agent_p): Filter out
	agents with unsupported ISA.
---
 libgomp/plugin/plugin-gcn.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/libgomp/plugin/plugin-gcn.c b/libgomp/plugin/plugin-gcn.c
index 588358bbbf9..2a17dc8accc 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,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;
+	  }
+      }
       break;
     case HSA_DEVICE_TYPE_CPU:
       if (!support_cpu_devices)
-- 
2.35.3


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Avoid registering unsupported OMP offload devices
  2024-01-26 14:04     ` Richard Biener
@ 2024-01-26 14:11       ` Jakub Jelinek
  2024-01-26 14:21         ` Richard Biener
  2024-01-26 14:22       ` Andrew Stubbs
  1 sibling, 1 reply; 10+ messages in thread
From: Jakub Jelinek @ 2024-01-26 14:11 UTC (permalink / raw)
  To: Richard Biener; +Cc: Andrew Stubbs, gcc-patches

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.

	Jakub


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Avoid registering unsupported OMP offload devices
  2024-01-26 14:11       ` Jakub Jelinek
@ 2024-01-26 14:21         ` Richard Biener
  2024-01-26 14:25           ` Andrew Stubbs
  0 siblings, 1 reply; 10+ messages in thread
From: Richard Biener @ 2024-01-26 14:21 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Andrew Stubbs, gcc-patches

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.

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)
-- 
2.35.3


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Avoid registering unsupported OMP offload devices
  2024-01-26 14:04     ` Richard Biener
  2024-01-26 14:11       ` Jakub Jelinek
@ 2024-01-26 14:22       ` Andrew Stubbs
  1 sibling, 0 replies; 10+ messages in thread
From: Andrew Stubbs @ 2024-01-26 14:22 UTC (permalink / raw)
  To: Richard Biener; +Cc: Jakub Jelinek, gcc-patches

On 26/01/2024 14:04, Richard Biener wrote:
> On Fri, 26 Jan 2024, Andrew Stubbs wrote:
> 
>> On 26/01/2024 12:06, Jakub Jelinek wrote:
>>> On Fri, Jan 26, 2024 at 01:00:28PM +0100, Richard Biener wrote:
>>>> 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.
>>>>
>>>> I'll run a bootstrap with both pending changes and will do
>>>> another round of full libgomp testing with this.
>>>>
>>>> OK if that succeeds?
>>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>> libgomp/
>>>>   * plugin/plugin-gcn.c (suitable_hsa_agent_p): Filter out
>>>>   agents with unsupported ISA.
>>>> ---
>>>>    libgomp/plugin/plugin-gcn.c | 9 +++++++++
>>>>    1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/libgomp/plugin/plugin-gcn.c b/libgomp/plugin/plugin-gcn.c
>>>> index 588358bbbf9..88ed77ff049 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);
>>>
>>> Space before ( please.
>>>
>>>> +
>>>>    /* Return true if the agent is a GPU and can accept of concurrent
>>>>    submissions
>>>>       from different threads.  */
>>>>    @@ -1443,6 +1445,13 @@ suitable_hsa_agent_p (hsa_agent_t agent)
>>>>      switch (device_type)
>>>>        {
>>>>        case HSA_DEVICE_TYPE_GPU:
>>>> +      {
>>>> +	char name[64];
>>>> +	if ((hsa_fns.hsa_agent_get_info_fn (agent, HSA_AGENT_INFO_NAME, name)
>>>> +	     != HSA_STATUS_SUCCESS)
>>>> +	    || isa_code (name) == EF_AMDGPU_MACH_UNSUPPORTED)
>>>> +	  return false;
>>>> +      }
>>>>          break;
>>>>        case HSA_DEVICE_TYPE_CPU:
>>>>          if (!support_cpu_devices)
>>>
>>> Otherwise it looks reasoanble to me, but let's see what Andrew thinks.
>>
>> 'n' before 'a', please. ;-)
> 
> ?!
> 
>> I think we need at least a GCN_DEBUG message when we ignore a GPU device.
>> Possibly gomp_debug also.
> 
> Like the following?  This will do
> 
> GCN debug: HSA run-time initialized for GCN
> GCN debug: HSA_SYSTEM_INFO_ENDIANNESS: LITTLE
> GCN debug: HSA_SYSTEM_INFO_EXTENSIONS: IMAGES
> GCN debug: Ignoring unsupported agent 'gfx1036'
> GCN debug: There are 1 GCN GPU devices.
> GCN debug: Ignoring unsupported agent 'gfx1036'
> GCN debug: HSA_AGENT_INFO_NAME: AMD Ryzen 9 7900X 12-Core Processor
> ...
> 
> for debug it's probably not too imporant to say this twice.
> 
> That said, no idea how to do gomp_debug.
> 
> OK?

I'm fairly comfortable with the repeat in debug output.

I mentioned gomp_debug because the target-independent GOMP_DEBUG=1 is a 
lot less noisy and actually documented where end-users might find it. 
 From the plugin you would call GOMP_PLUGIN_debug (there are examples in 
plugin-nvptx.c). Probably the repeat is less welcome in that case 
though, so perhaps good for a follow-up.

> 
> Thanks,
> Richard.
> 
> 
>  From 7462a8ac70aeebc231c65456b9060d8cbf7d4c50 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.
> 
> libgomp/
> 	* plugin/plugin-gcn.c (suitable_hsa_agent_p): Filter out
> 	agents with unsupported ISA.
> ---
>   libgomp/plugin/plugin-gcn.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/libgomp/plugin/plugin-gcn.c b/libgomp/plugin/plugin-gcn.c
> index 588358bbbf9..2a17dc8accc 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,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;
> +	  }
> +      }
>         break;
>       case HSA_DEVICE_TYPE_CPU:
>         if (!support_cpu_devices)

Like Jakub says, I think it needs to be like this, to be safe:

   status = hsa_fns.hsa_agent_get_info_fn (...)
   if (status unsuccessful || name unsupported)
     if (status successful) output debug
     return false

Andrew

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Avoid registering unsupported OMP offload devices
  2024-01-26 14:21         ` Richard Biener
@ 2024-01-26 14:25           ` Andrew Stubbs
  0 siblings, 0 replies; 10+ messages in thread
From: Andrew Stubbs @ 2024-01-26 14:25 UTC (permalink / raw)
  To: Richard Biener, Jakub Jelinek; +Cc: gcc-patches

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)


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] Avoid registering unsupported OMP offload devices
@ 2024-01-26 12:00 Richard Biener
  0 siblings, 0 replies; 10+ messages in thread
From: Richard Biener @ 2024-01-26 12:00 UTC (permalink / raw)
  To: gcc-patches; +Cc: ams

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.

I'll run a bootstrap with both pending changes and will do
another round of full libgomp testing with this.

OK if that succeeds?

Thanks,
Richard.

libgomp/
	* plugin/plugin-gcn.c (suitable_hsa_agent_p): Filter out
	agents with unsupported ISA.
---
 libgomp/plugin/plugin-gcn.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/libgomp/plugin/plugin-gcn.c b/libgomp/plugin/plugin-gcn.c
index 588358bbbf9..88ed77ff049 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,13 @@ suitable_hsa_agent_p (hsa_agent_t agent)
   switch (device_type)
     {
     case HSA_DEVICE_TYPE_GPU:
+      {
+	char name[64];
+	if ((hsa_fns.hsa_agent_get_info_fn (agent, HSA_AGENT_INFO_NAME, name)
+	     != HSA_STATUS_SUCCESS)
+	    || isa_code (name) == EF_AMDGPU_MACH_UNSUPPORTED)
+	  return false;
+      }
       break;
     case HSA_DEVICE_TYPE_CPU:
       if (!support_cpu_devices)
-- 
2.35.3

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-01-26 14:25 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20240126120143.6978F3858C62@sourceware.org>
2024-01-26 12:06 ` [PATCH] Avoid registering unsupported OMP offload devices Jakub Jelinek
2024-01-26 12:30   ` Tobias Burnus
2024-01-26 12:32     ` Richard Biener
2024-01-26 13:39   ` Andrew Stubbs
2024-01-26 14:04     ` Richard Biener
2024-01-26 14:11       ` Jakub Jelinek
2024-01-26 14:21         ` Richard Biener
2024-01-26 14:25           ` Andrew Stubbs
2024-01-26 14:22       ` Andrew Stubbs
2024-01-26 12:00 Richard Biener

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).