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

[Thomas Schwinge <thomas@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 3014 bytes --]

Hi Frederik!

On 2019-12-20T17:46:57+0100, "Harwath, Frederik" <frederik@codesourcery.com> wrote:
>> | Before Frederik starts working on integrating this into GCC trunk, do you
>> | (Jakub) agree with the libgomp plugin interface changes as implemented by
>> | Maciej?  For example, top-level 'GOMP_OFFLOAD_get_property' function in
>> | 'struct gomp_device_descr' instead of stuffing this into its
>> | 'acc_dispatch_t openacc'.  (I never understood why the OpenACC functions
>> | need to be segregated like they are.)
>>
>> Jakub didn't answer, but I now myself decided that we should group this
>> with the other OpenACC libgomp-plugin functions, as this interface is
>> defined in terms of OpenACC-specific stuff such as 'acc_device_t'.
>> Frederik, please work on that, also try to move function definitions etc.
>> into appropriate places in case they aren't; ask if you need help.
>> That needs to be updated.
>
> Is it ok to do this in a separate follow-up patch?

Yes.  This doesn't affect anything but the libgomp-plugin interface.


>> > --- a/include/gomp-constants.h
>> > +++ b/include/gomp-constants.h
>> > @@ -178,6 +178,20 @@ enum gomp_map_kind
>> >=20=20
>> >  #define GOMP_DEVICE_ICV			-1
>> >  #define GOMP_DEVICE_HOST_FALLBACK	-2
>> > +#define GOMP_DEVICE_CURRENT		-3
>> [...]
>>
>> Not should if this should be grouped with 'GOMP_DEVICE_ICV',
>> 'GOMP_DEVICE_HOST_FALLBACK', for it is not related to there.
>>
>> [...]
>>
>> Should this actually get value '-1' instead of '-3'?  Or, is the OpenACC
>> 'acc_device_t' code already paying special attention to negative values
>> '-1', '-2'?  (I don't think so.)
>> | Also, 'acc_device_current' is a libgomp-internal thing (doesn't interface
>> | with the compiler proper), so strictly speaking 'GOMP_DEVICE_CURRENT'
>> | isn't needed in 'include/gomp-constants.h'.  But probably still a good
>> | idea to list it there, in this canonical place, to keep the several lists
>> | of device types coherent.
>> still wonder about that...  ;-)

> Changing the value of GOMP_DEVICE_ICV violates the following static asserts in oacc-parallel.c: [...]

Hmm, I didn't intend to suggest changing the 'GOMP_DEVICE_ICV' value,
which indeed can't/shouldn't be done.

I still think that 'GOMP_DEVICE_CURENT' should get value '-1' (and
probably be rename 'GOACC_DEVICE_CURRENT' to make more obvious that it's
not related to the 'GOMP_DEVICE_*' ones), but we shall have a look at
that later (before GCC 10 release); that's libgomp/OpenACC-internal,
doesn't affect anything else.

>> Maybe this stuff should move from 'include/gomp-constants.h' to
>> 'libgomp/oacc-int.h'.  I'll think about that again, when I'm awake again
>> tomorrow.  ;-)
>
> Have you made up your mind yet? :-)

Still sleepy.  ;-)


> Is it ok to commit the patch to trunk?

OK, thanks.  And then some follow-up/clean-up next year, also including
some of the open questions that I've snipped off here.


Grüße
 Thomas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 658 bytes --]