Re: [RFC PATCH] targetm.omp.device_kind_arch_isa and OpenMP declare variant kind/arch/isa handling
Hi, On Tue, Oct 29 2019, Jakub Jelinek wrote: > Does this approach look reasonable and is it ok with the backend maintainers > listed in To:? Martin listed for HSA, I'm afraid right now not really sure > at which point it would be possible to distinguish hsa guarded targeted code > from host targeted one. Right, since the general approach is to heavily rely on falling back on the host, I don't think you can do much better than what the patch does. Thanks, Martin > CCed some backend maintainers for thoughts on what > would be reasonable values for the target hook on their backends. > > 2019-10-29 Jakub Jelinek > > * configure.ac: Compute and substitute omp_device_properties and > omp_device_property_deps. > * Makefile.in (generated_files): Add omp-device-properties.h. > (omp-general.o): Depend on omp-device-properties.h. > (omp_device_properties): New make variable. > (omp-device-properties.h, s-omp-device-properties-h, > install-omp-device-properties): New goals. > (install): Depend on install-omp-device-properties for accelerators. > * target.def (TARGET_OMP_DEVICE_KIND_ARCH_ISA): New target hook. > * target.h (enum omp_device_kind_arch_isa): New enum. > * doc/tm.texi.in: Add placeholder for TARGET_OMP_DEVICE_KIND_ARCH_ISA > documentation. > * omp-general.c: Include omp-device-properties.h. > (omp_max_simt_vf): Expect OFFLOAD_TARGET_NAMES to be separated by > colon instead of comma. > (omp_offload_device_kind_arch_isa, omp_maybe_offloaded): New > functions. > (omp_context_selector_matches): Implement device set arch/isa > selectors, improve device set kind selector handling. > * config/i386/i386-options.h (ix86_omp_device_kind_arch_isa): Declare. > * config/i386/i386.c (TARGET_SIMD_CLONE_ADJUST, > TARGET_SIMD_CLONE_USABLE): Formatting fix. > (TARGET_OMP_DEVICE_KIND_ARCH_ISA): Redefine to > ix86_omp_device_kind_arch_isa. > * config/i386/i386-options.c (struct ix86_target_opts): Move type > definition from ix86_target_string to file scope. > (isa2_opts, isa_opts): Moved arrays from ix86_target_string function > to file scope. > (ix86_omp_device_kind_arch_isa): New function. > (ix86_target_string): Moved struct ix86_target_opts, isa2_opts and > isa_opts definitions to file scope. > * config/i386/t-intelmic (omp-device-properties): New goal. > * config/nvptx/t-nvptx (omp-device-properties): Likewise. > * config/nvptx/nvptx.c (nvptx_omp_device_kind_arch_isa): New function. > (TARGET_OMP_DEVICE_KIND_ARCH_ISA): Redefine to > nvptx_omp_device_kind_arch_isa. > * configure: Regenerate. > * doc/tm.texi: Regenerate. > testsuite/ > * c-c++-common/gomp/declare-variant-9.c: New test. > * c-c++-common/gomp/declare-variant-10.c: New test. >
Re: [RFC PATCH] targetm.omp.device_kind_arch_isa and OpenMP declare variant kind/arch/isa handling
On 31/10/2019 09:35, Jakub Jelinek wrote: > On Thu, Oct 31, 2019 at 09:16:00AM +, Richard Sandiford wrote: >>> Yes, it is indeed not clear, subject to ongoing discussions. >>> My reading of the spec was that all the *-name-list are comma >>> separated lists of identifiers, some others in the committee >>> want now (yesterday's discussions) string literals instead >>> when I and others pointed out that isa(core-avx512) can't be valid, >>> but strangely only for isa/arch/extension but not e.g. for >>> kind or vendor which would still take identifier lists etc. >> >> Might be completely wrong, but wouldn't the identifiers be subject to >> macro expansion? That would make it harder to use the pragmas safely >> in system headers. > > Well, all the identifiers in the OpenMP/OpenACC pragmas after the omp > are subject to macro expansion, including clause names etc., so even > #define device foobar > #pragma omp declare variant (foo) match (device={kind(host)}) > is a problem, or macro redefinition of declare, variant, match, > kind or host. That said, for the arch and isa it is perhaps a bigger > problem, as can be seen in the c-c++-common/gomp/declare-variant-9.c > testcase where I had to #undef i386 because in non-strict modes > that is defined... that is surprising, preproc directives are normally not subject to macro expansion unless stated explicitly so gcc normally don't macro expand pragmas.. i consider this a bug in the omp spec and one more reason why _Pragma should be used instead of #pragma.
Re: [RFC PATCH] targetm.omp.device_kind_arch_isa and OpenMP declare variant kind/arch/isa handling
On Thu, Oct 31, 2019 at 09:16:00AM +, Richard Sandiford wrote: > > Yes, it is indeed not clear, subject to ongoing discussions. > > My reading of the spec was that all the *-name-list are comma > > separated lists of identifiers, some others in the committee > > want now (yesterday's discussions) string literals instead > > when I and others pointed out that isa(core-avx512) can't be valid, > > but strangely only for isa/arch/extension but not e.g. for > > kind or vendor which would still take identifier lists etc. > > Might be completely wrong, but wouldn't the identifiers be subject to > macro expansion? That would make it harder to use the pragmas safely > in system headers. Well, all the identifiers in the OpenMP/OpenACC pragmas after the omp are subject to macro expansion, including clause names etc., so even #define device foobar #pragma omp declare variant (foo) match (device={kind(host)}) is a problem, or macro redefinition of declare, variant, match, kind or host. That said, for the arch and isa it is perhaps a bigger problem, as can be seen in the c-c++-common/gomp/declare-variant-9.c testcase where I had to #undef i386 because in non-strict modes that is defined... Jakub
Re: [RFC PATCH] targetm.omp.device_kind_arch_isa and OpenMP declare variant kind/arch/isa handling
Thanks for implementing this. Jakub Jelinek writes: > On Wed, Oct 30, 2019 at 02:12:30PM +, Szabolcs Nagy wrote: >> On 29/10/2019 17:15, Jakub Jelinek wrote: >> > +void f03 (void); >> > +#pragma omp declare variant (f03) match >> > (device={kind(any),arch(x86_64),isa(avx512f,avx512bw)}) >> > +void f04 (void); >> >> 1) it's not clear from the omp spec what is the intended >> syntax for isa-name, arch-name and extension-name, but >> i expected strings in "". > > Yes, it is indeed not clear, subject to ongoing discussions. > My reading of the spec was that all the *-name-list are comma > separated lists of identifiers, some others in the committee > want now (yesterday's discussions) string literals instead > when I and others pointed out that isa(core-avx512) can't be valid, > but strangely only for isa/arch/extension but not e.g. for > kind or vendor which would still take identifier lists etc. Might be completely wrong, but wouldn't the identifiers be subject to macro expansion? That would make it harder to use the pragmas safely in system headers. Richard
Re: [RFC PATCH] targetm.omp.device_kind_arch_isa and OpenMP declare variant kind/arch/isa handling
On Tue, Oct 29, 2019 at 11:57:40PM +0100, Jakub Jelinek wrote: > On Tue, Oct 29, 2019 at 05:40:11PM -0500, Segher Boessenkool wrote: > > On Tue, Oct 29, 2019 at 06:15:31PM +0100, Jakub Jelinek wrote: > > There already are a lot of different ways to get information about the > > execution environment you're running on; why is this any better? > > There is no question of better or worse, it is simply a part of a standard > that GCC is trying to implement, like we try to implement all of C++20, we > also try to implement all of OpenMP or OpenACC etc. We now get three levels: "arch", "isa", "extensions". Power has only at most half of such levels: arch is Power, isa is Power, you can say things like AltiVec (aka VMX) and VSX are extensions (that's what the X stands for anyway :-) ). In older versions of the architecture we had (optional) "categories", but that is gone as well. So yes I wonder how we should map reality to these three levels. And I also do wonder how this is better than one level as anything else has :-) > The exact identifiers are implementation defined though, and that is why we > need to think of what is reasonable for each target (at least each target > where OpenMP is used often, powerpc is certainly one of those). Do you know any OpenMP on Power users we can ask for their opinions? Segher
Re: [RFC PATCH] targetm.omp.device_kind_arch_isa and OpenMP declare variant kind/arch/isa handling
On 30/10/2019 14:48, Jakub Jelinek wrote: > ARM is an OpenMP member, so if you want, you can participate too. > https://github.com/OpenMP/spec/issues/2028 > is where I'm trying to track all the declare variant issues that need > clarification (plus in two examples tickets). it's unfortunate that neither the mailing list nor the github repo for the spec are public, i'll try to get access to them to see the discussions. thanks for the explanations. >> for simd variants it means i need to declare the function >> with the right simd types and attributes. > > For simd it is actually not finished yet, what needs to be done is that > given the declare simd clauses used as properties of the simd selector > the FEs use some target hook that will guide it how to transform > the parameters like targetm.simd_clone.compute_vecsize_and_simdlen > does and for C tries to just match the types against it and determine > through that the ABI and perhaps missing simd clauses like > notinbranch/inbranch, simdlen etc., for C++ actually for each possibility > will try to construct a call with such arguments and then compare the types. this omp declare variant mechanism seems fairly complicated. i need is a way to specify a simd variant unambiguously, which requires is_inbranch, simd_len and vector_call_abi setting as far as i can tell (potentially a symbol_name too if the vector abi mangled name is not good enough). i think i can extend the simd attribute to do this e.g. __attribute__((simd("notinbranch", 4, "sse2"))) float expf(float); would declare the _ZGVbN4v_expf simd variant of expf. (multiple attributes can be used to declare multiple variants.) or __attribute__((simd("notinbranch", 4, "sse2", "my_vexpf"))) float expf(float); or maybe typedef float vfloat __attribute__((vector_size(16))); vfloat my_vexpf(vfloat); __attribute__((simd("notinbranch", 4, "sse2", my_vexpf))) float expf(float); if we allow custom symbol name for the simd variant. here "sse2" is not specifying a gcc target nor instruction set, but the vector call convention, e.g. on x86_64 there could be "sse2", "avx", "avx2" and "avx512", on aarch64 "advsimd" and "sve". i thought this would match the omp isa, but based on your description omp isa will be something else. there is a further complication that vector length agnostic (scalable) sve calls need a special simd len value: i'd reserve 0 for it, but it seems internally that means 'simd len is unset' so that has to change. i will try to prepare an initial patch for such attribute to make the proposal more concrete, but if you have any concerns please let me know.
Re: [RFC PATCH] targetm.omp.device_kind_arch_isa and OpenMP declare variant kind/arch/isa handling
On Wed, Oct 30, 2019 at 02:12:30PM +, Szabolcs Nagy wrote: > On 29/10/2019 17:15, Jakub Jelinek wrote: > > +void f03 (void); > > +#pragma omp declare variant (f03) match > > (device={kind(any),arch(x86_64),isa(avx512f,avx512bw)}) > > +void f04 (void); > > 1) it's not clear from the omp spec what is the intended > syntax for isa-name, arch-name and extension-name, but > i expected strings in "". Yes, it is indeed not clear, subject to ongoing discussions. My reading of the spec was that all the *-name-list are comma separated lists of identifiers, some others in the committee want now (yesterday's discussions) string literals instead when I and others pointed out that isa(core-avx512) can't be valid, but strangely only for isa/arch/extension but not e.g. for kind or vendor which would still take identifier lists etc. My preference at this point would be probably to allow in all vendor/kind/arch/isa/extension lists of either identifiers or string literals, so for names which don't contain characters invalid in identifiers users could choose what to write, so both isa(avx512f,avx512bw) and isa("avx512f","avx512bw") would be valid (and so would be isa(avx512f,"avx512bw")), obviously for something that is not a valid identifier users wouldn't have a choice, armv8.2-a+sve is not an identifier. > what if an arch or isa name contains ',' ')' etc? > > we were planing to use things like > > isa("sve") > arch("armv8.2-a+sve") > extension("scalable") That is subject to yet another ongoing discussion in the committee, what shall be the meaning of isa and what shall be the meaning of arch, I think we need something that will hold the GCC target or something similar and something that holds the instruction sets for that target. extension, given it is in the implementation trait set, is IMNSHO not meant to hold device extensions, but rather software extensions or something similar. ARM is an OpenMP member, so if you want, you can participate too. https://github.com/OpenMP/spec/issues/2028 is where I'm trying to track all the declare variant issues that need clarification (plus in two examples tickets). > 2) does f03 need to be declared before the declare variant > pragma appears? Yes. For C++ the spec says that the actual function declaration is determined by what would be called at the point of the pragma with the given id-expression and arguments with types from the function declaration on which the pragma is used, so it can involve ADL, needs to deal with function overloading etc. > for simd variants it means i need to declare the function > with the right simd types and attributes. For simd it is actually not finished yet, what needs to be done is that given the declare simd clauses used as properties of the simd selector the FEs use some target hook that will guide it how to transform the parameters like targetm.simd_clone.compute_vecsize_and_simdlen does and for C tries to just match the types against it and determine through that the ABI and perhaps missing simd clauses like notinbranch/inbranch, simdlen etc., for C++ actually for each possibility will try to construct a call with such arguments and then compare the types. I'd like to make non-simd working first though, for some cases it already works and replacement is done in the gimplifier, but scoring needs to be added, then some way to keep such info in the cgraph (will need to talk to Honza) and after IPA perform another attempt to redirect. Jakub
Re: [RFC PATCH] targetm.omp.device_kind_arch_isa and OpenMP declare variant kind/arch/isa handling
On 29/10/2019 17:15, Jakub Jelinek wrote: > +void f03 (void); > +#pragma omp declare variant (f03) match > (device={kind(any),arch(x86_64),isa(avx512f,avx512bw)}) > +void f04 (void); 1) it's not clear from the omp spec what is the intended syntax for isa-name, arch-name and extension-name, but i expected strings in "". what if an arch or isa name contains ',' ')' etc? we were planing to use things like isa("sve") arch("armv8.2-a+sve") extension("scalable") i think we can drop the ", but it looks a bit weird: normal pp-token parsing of directives would break the arch name up into multiple tokens (unless it's special cased like include <...>, at least inside _Pragma("omp ...") there is no expectation of normal pp-token parsing), either way is fine with me, but it may be worth asking clarification from omp? 2) does f03 need to be declared before the declare variant pragma appears? for simd variants it means i need to declare the function with the right simd types and attributes.
Re: [RFC PATCH] targetm.omp.device_kind_arch_isa and OpenMP declare variant kind/arch/isa handling
On Tue, Oct 29, 2019 at 05:40:11PM -0500, Segher Boessenkool wrote: > On Tue, Oct 29, 2019 at 06:15:31PM +0100, Jakub Jelinek wrote: > > The standard makes it implementation defined what an arch is and what is > > isa, but I think because there is no selector like target that arch should > > mostly contain identifiers that match the ABI incompatible stuff (target, > > perhaps whether it is 32-bit or 64-bit, plus endianity where needed etc.) > > and keep isa to be identifiers for the ISAs, or perhaps where there are no > > clear ISA names say architecture variants or revisions or similar. > > > > I've only implemented i386 and nvptx so far, will leave the rest to > > port maintainers; would be nice to coordinate what is added a little bit > > with other implementations like LLVM, if they'd be willing to coordinate. > > What would this be used for? Can you give some more context? https://www.openmp.org/spec-html/5.0/openmpse11.html#x41-480002.3 OpenMP 5.0 has two directives, declare variant which I'm implementing right now and metadirective, which I'll be working on next stage1. declare variant is a direct call redirection based on context, where one can provide a specialized function implementation for a particular OpenMP construct, compiler implementation, CPU architecture, etc. The metadirective allows specialization of OpenMP pragmas, use this OpenMP pragma only in some OpenMP context and not in another one, or only on certain architecture, isa, whatever. These actually don't query anything at runtime, it is purely compile time specialization, based on what either the whole translation unit or a particular function are compiled for. > There already are a lot of different ways to get information about the > execution environment you're running on; why is this any better? There is no question of better or worse, it is simply a part of a standard that GCC is trying to implement, like we try to implement all of C++20, we also try to implement all of OpenMP or OpenACC etc. The exact identifiers are implementation defined though, and that is why we need to think of what is reasonable for each target (at least each target where OpenMP is used often, powerpc is certainly one of those). Jakub
Re: [RFC PATCH] targetm.omp.device_kind_arch_isa and OpenMP declare variant kind/arch/isa handling
Hi! On Tue, Oct 29, 2019 at 06:15:31PM +0100, Jakub Jelinek wrote: > The standard makes it implementation defined what an arch is and what is > isa, but I think because there is no selector like target that arch should > mostly contain identifiers that match the ABI incompatible stuff (target, > perhaps whether it is 32-bit or 64-bit, plus endianity where needed etc.) > and keep isa to be identifiers for the ISAs, or perhaps where there are no > clear ISA names say architecture variants or revisions or similar. > > I've only implemented i386 and nvptx so far, will leave the rest to > port maintainers; would be nice to coordinate what is added a little bit > with other implementations like LLVM, if they'd be willing to coordinate. What would this be used for? Can you give some more context? There already are a lot of different ways to get information about the execution environment you're running on; why is this any better? Segher
[RFC PATCH] targetm.omp.device_kind_arch_isa and OpenMP declare variant kind/arch/isa handling
Hi! The following patch attempts to implement the OpenMP declare variant (and later on metadirective) device set arch/isa selectors. The standard makes it implementation defined what an arch is and what is isa, but I think because there is no selector like target that arch should mostly contain identifiers that match the ABI incompatible stuff (target, perhaps whether it is 32-bit or 64-bit, plus endianity where needed etc.) and keep isa to be identifiers for the ISAs, or perhaps where there are no clear ISA names say architecture variants or revisions or similar. I've only implemented i386 and nvptx so far, will leave the rest to port maintainers; would be nice to coordinate what is added a little bit with other implementations like LLVM, if they'd be willing to coordinate. The target hook returns a tri-state value, 0 for doesn't match and will not match anywhere in the translation unit, 1 for matches and -1 for doesn't match in the current context, but could match in some other function. On targets that don't support target attribute or something similar, returning just 0 or 1 might be enough, -1 is meant for cases where e.g. during parsing of the pragma when we do not know in which context it will be called we can signal "don't know whether it will match or not". The patch doesn't just add a hook, but also infrastructure through which the --enable-as-accelerator-for= configured compilers can tell the host compiler what identifiers they do support for kind, arch and isa, so that for calls used in contexts that are or might be offloaded, we can defer decisions until after IPA where we know for sure if it is the offloading version say for nvptx, or offloading version for host fallback, or something yet different. Initially I wanted the target hook to handle name == NULL by printing the list of kind/arch/isa and some undocumented option that the host compiler would call the accelerator compiler with, but then realized it wouldn't really work with canadian crosses, so the current version just uses sed when there are too many values that it needs to be maintained in one place. Tested on x86_64-linux with offloading to nvptx-none,x86_64-intelmicemul-linux-gnu. Will bootstrap/regtest also without any offloading configured. Does this approach look reasonable and is it ok with the backend maintainers listed in To:? Martin listed for HSA, I'm afraid right now not really sure at which point it would be possible to distinguish hsa guarded targeted code from host targeted one. CCed some backend maintainers for thoughts on what would be reasonable values for the target hook on their backends. 2019-10-29 Jakub Jelinek * configure.ac: Compute and substitute omp_device_properties and omp_device_property_deps. * Makefile.in (generated_files): Add omp-device-properties.h. (omp-general.o): Depend on omp-device-properties.h. (omp_device_properties): New make variable. (omp-device-properties.h, s-omp-device-properties-h, install-omp-device-properties): New goals. (install): Depend on install-omp-device-properties for accelerators. * target.def (TARGET_OMP_DEVICE_KIND_ARCH_ISA): New target hook. * target.h (enum omp_device_kind_arch_isa): New enum. * doc/tm.texi.in: Add placeholder for TARGET_OMP_DEVICE_KIND_ARCH_ISA documentation. * omp-general.c: Include omp-device-properties.h. (omp_max_simt_vf): Expect OFFLOAD_TARGET_NAMES to be separated by colon instead of comma. (omp_offload_device_kind_arch_isa, omp_maybe_offloaded): New functions. (omp_context_selector_matches): Implement device set arch/isa selectors, improve device set kind selector handling. * config/i386/i386-options.h (ix86_omp_device_kind_arch_isa): Declare. * config/i386/i386.c (TARGET_SIMD_CLONE_ADJUST, TARGET_SIMD_CLONE_USABLE): Formatting fix. (TARGET_OMP_DEVICE_KIND_ARCH_ISA): Redefine to ix86_omp_device_kind_arch_isa. * config/i386/i386-options.c (struct ix86_target_opts): Move type definition from ix86_target_string to file scope. (isa2_opts, isa_opts): Moved arrays from ix86_target_string function to file scope. (ix86_omp_device_kind_arch_isa): New function. (ix86_target_string): Moved struct ix86_target_opts, isa2_opts and isa_opts definitions to file scope. * config/i386/t-intelmic (omp-device-properties): New goal. * config/nvptx/t-nvptx (omp-device-properties): Likewise. * config/nvptx/nvptx.c (nvptx_omp_device_kind_arch_isa): New function. (TARGET_OMP_DEVICE_KIND_ARCH_ISA): Redefine to nvptx_omp_device_kind_arch_isa. * configure: Regenerate. * doc/tm.texi: Regenerate. testsuite/ * c-c++-common/gomp/declare-variant-9.c: New test. * c-c++-common/gomp/declare-variant-10.c: New test. --- gcc/configure.ac.jj 2019-10-29