On Sat, Jul 20, 2024 at 02:42:27PM -0600, Sandra Loosemore wrote:
> The OpenMP spec says:
>
> "If trait-property any is specified in the kind trait-selector of the
> device selector set or the target_device selector sets, no other
> trait-property may be specified in the same selector set."
That is OpenMP 5.1 addition, the code was written for OpenMP 5.0, so it was
valid at that time.
> GCC was not previously enforcing this restriction and several testcases
> included such valid constructs. This patch fixes it.
>
> gcc/ChangeLog
> * omp-general.cc (omp_check_context_selector): Reject other
> properties in the same selector set with kind(any).
>
> gcc/testsuite/ChangeLog
> * c-c++-common/gomp/declare-variant-10.c: Fix broken tests.
> * c-c++-common/gomp/declare-variant-3.c: Likewise.
> * c-c++-common/gomp/declare-variant-9.c: Likewise.
> * c-c++-common/gomp/declare-variant-any.c: New.
> * gfortran.dg/gomp/declare-variant-10.f90: Fix broken tests.
> * gfortran.dg/gomp/declare-variant-3.f90: Likewise.
> * gfortran.dg/gomp/declare-variant-9.f90: Likewise.
> * gfortran.dg/gomp/declare-variant-any.f90: Likewise.
> ---
> gcc/omp-general.cc | 31 +++++++++++++++++++
> .../c-c++-common/gomp/declare-variant-10.c | 4 +--
> .../c-c++-common/gomp/declare-variant-3.c | 10 ++----
> .../c-c++-common/gomp/declare-variant-9.c | 4 +--
> .../c-c++-common/gomp/declare-variant-any.c | 10 ++++++
> .../gfortran.dg/gomp/declare-variant-10.f90 | 4 +--
> .../gfortran.dg/gomp/declare-variant-3.f90 | 12 ++-----
> .../gfortran.dg/gomp/declare-variant-9.f90 | 2 +-
> .../gfortran.dg/gomp/declare-variant-any.f90 | 28 +++++++++++++++++
> 9 files changed, 82 insertions(+), 23 deletions(-)
> create mode 100644 gcc/testsuite/c-c++-common/gomp/declare-variant-any.c
> create mode 100644 gcc/testsuite/gfortran.dg/gomp/declare-variant-any.f90
>
> diff --git a/gcc/omp-general.cc b/gcc/omp-general.cc
> index 87a245ec8b3..12f178c5a2d 100644
> --- a/gcc/omp-general.cc
> +++ b/gcc/omp-general.cc
> @@ -1288,6 +1288,8 @@ omp_check_context_selector (location_t loc, tree ctx,
> bool metadirective_p)
> for (tree tss = ctx; tss; tss = TREE_CHAIN (tss))
> {
> enum omp_tss_code tss_code = OMP_TSS_CODE (tss);
> + bool saw_any_prop = false;
> + bool saw_other_prop = false;
>
> /* FIXME: not implemented yet. */
> if (!metadirective_p && tss_code == OMP_TRAIT_SET_TARGET_DEVICE)
> @@ -1325,6 +1327,27 @@ omp_check_context_selector (location_t loc, tree ctx,
> bool metadirective_p)
> else
> ts_seen[ts_code] = true;
>
> +
> + /* If trait-property "any" is specified in the "kind"
> + trait-selector of the "device" selector set or the
> + "target_device" selector sets, no other trait-property
> + may be specified in the same selector set. */
> + if (ts_code == OMP_TRAIT_DEVICE_KIND)
> + for (tree p = OMP_TS_PROPERTIES (ts); p; p = TREE_CHAIN (p))
> + {
> + const char *prop = omp_context_name_list_prop (p);
> + if (!prop)
> + continue;
> + else if (strcmp (prop, "any") == 0)
> + saw_any_prop = true;
> + else
> + saw_other_prop = true;
> + }
> + else if (ts_code == OMP_TRAIT_DEVICE_ARCH
The indentation looks wrong here, should be indented by 2 more columns to
the right.
Anyway, while 5.0/5.1/5.2 had
"Each trait-selector-name can only be specified once."
restriction and in 5.0 it made perfect sense, with the addition
of target_device set in 5.1 this seems to be weird and TR13 has
"Each trait-selector-name may only be specified once in a trait selector set."
So, in 5.1/5.2 pedantically
device={kind(host)},target_device={device_num(whatever),kind(nohost)}
was invalid and I think the code still rejects it, for 6.0 it will be
valid, so wonder if we shouldn't have
OMP_TRAIT_TARGET_DEVICE_KIND,
OMP_TRAIT_TARGET_DEVICE_ISA,
OMP_TRAIT_TARGET_DEVICE_ARCH,
and whether
OMP_TRAIT_DEVICE_NUM,
is appropriate and shouldn't be instead
OMP_TRAIT_TARGET_DEVICE_DEVICE_NUM,
Also wonder if
"If trait-property any is specified in the kind trait-selector of the device
selector set or
the target_device selector sets, no other trait-property may be specified in
the same
selector set."
shouldn't have an exception for device_num, one could possibly just want to
declare that some particular device_num has any kind, so
target_device={device_num(whatever),kind(any)}
rather than just the only pedantically allowed
target_device={kind(any)}
where the latter specifies something (well, nothing) about the default device
num while the
former would specify something (well, nothing) about a chosen device num.
Anyway, if OMP_TRAIT_TARGET_DEVICE_{KIND,ISA,ARCH} are separate from
OMP_TRAIT_DEVICE_{KIND,ISA,ARCH}, all this checking could be done simply by
looking at tss_seen array rather than remembering saw_other_prop. Unhandled
extension selectors don't make it through here anyway.
> + || ts_code == OMP_TRAIT_DEVICE_ISA
> + || ts_code == OMP_TRAIT_DEVICE_NUM)
> + saw_other_prop = true;
> +
> if (omp_ts_map[ts_code].valid_properties == NULL)
> continue;
>
> @@ -1377,6 +1400,14 @@ omp_check_context_selector (location_t loc, tree ctx,
> bool metadirective_p)
> break;
> }
> }
> +
> + if (saw_any_prop && saw_other_prop)
> + {
> + error_at (loc,
> + "no other trait-property may be specified "
> + "in the same selector set with %<kind(\"any\")%>");
> + return error_mark_node;
> + }
> }
> return ctx;
> }
If this can apply (perhaps with small fuzz) to vanilla trunk, guess it can
be committed right now, doesn't need to wait for the rest of the
metadirective patch set.
Jakub
