Hi Frederik! On 2019-12-20T17:46:57+0100, "Harwath, Frederik" <frede...@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
signature.asc
Description: PGP signature
