Re: [PATCH] Add OpenACC 2.6 `acc_get_property' support

[Martin Jambor <mjambor@suse.cz>]

Hi,

On Tue, Dec 17 2019, Thomas Schwinge wrote:
> On 2019-11-14T16:35:31+0100, Frederik Harwath <frederik@codesourcery.com> wrote:
>> this patch implements OpenACC 2.6 "acc_get_property" and related functions.
>
> [...]
>
>> --- a/libgomp/plugin/plugin-hsa.c
>> +++ b/libgomp/plugin/plugin-hsa.c
>> @@ -699,6 +699,32 @@ GOMP_OFFLOAD_get_num_devices (void)
>>    return hsa_context.agent_count;
>>  }
>>  
>> +/* Part of the libgomp plugin interface.  Return the value of property
>> +   PROP of agent number N.  */
>> +
>> +union gomp_device_property_value
>> +GOMP_OFFLOAD_get_property (int n, int prop)
>> +{
>> +  union gomp_device_property_value nullval = { .val = 0 };
>> +
>> +  if (!init_hsa_context ())
>> +    return nullval;
>
> I'm not familiar with that code, but similar to other plugins,
> 'init_hsa_context' already is called via 'GOMP_OFFLOAD_get_num_devices'
> (and 'GOMP_OFFLOAD_init_device', hmm...), so probably don't need to call
> it here?

I assume you always want to get the number of devices before querying
their properties but OTOH there is no harm in calling the initialization
function.

>> +
>> +  switch (prop)
>> +    {
>> +    case GOMP_DEVICE_PROPERTY_VENDOR:
>> +      return (union gomp_device_property_value) { .ptr = "AMD" };
>> +    default:
>> +      return nullval;
>> +    }
>> +}
>
> Not sure if "AMD" is actually correct here -- isn't HSA a
> vendor-independent standard?
>

Yes, it is supposed to be.  I think HSA is the correct "vendor" too,
essentially an abbreviation for HSA Foundation.

Thanks,

Martin