[v3,08/12] OpenMP: Reject other properties with kind(any)

  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."

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
  

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
  

On 8/16/24 06:58, Jakub Jelinek wrote:
> 
> 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.

OK.  I've tested the slightly cleaned-up version of the patch which is 
attached; I'll push it in the next day or two if there is no further 
objection.

-Sandra
  

On Sun, Sep 08, 2024 at 09:15:23AM -0600, Sandra Loosemore wrote:
> On 8/16/24 06:58, Jakub Jelinek wrote:
> > 
> > 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.
> 
> OK.  I've tested the slightly cleaned-up version of the patch which is
> attached; I'll push it in the next day or two if there is no further
> objection.
> 
> -Sandra

> From 23a82bea26805f2a430354d56b444d5bb7eaed3f Mon Sep 17 00:00:00 2001
> From: Sandra Loosemore <sloosemore@baylibre.com>
> Date: Fri, 6 Sep 2024 20:58:13 +0000
> Subject: [PATCH] OpenMP: Reject other properties with kind(any)
> 
> TR13 (pre-6.0) of 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."
> 
> This restriction dates back to OpenMP 5.1 (where it had slightly
> different wording).  GCC's implementation was based on the 5.0 spec, and
> several testcases include selectors that are now considered invalid.
> This patch adds a diagnostic and fixes the testcases.
> 
> 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: New.
> ---
>  gcc/omp-general.cc                            | 35 +++++++++++++++++++
>  .../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, 86 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 0b61335dba4..aa53c00bec5 100644
> --- a/gcc/omp-general.cc
> +++ b/gcc/omp-general.cc
> @@ -1288,6 +1288,8 @@ omp_check_context_selector (location_t loc, tree ctx)
>    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;
>  
>        /* We can parse this, but not handle it yet.  */
>        if (tss_code == OMP_TRAIT_SET_TARGET_DEVICE)
> @@ -1324,6 +1326,31 @@ omp_check_context_selector (location_t loc, tree ctx)
>  	  else
>  	    ts_seen[ts_code] = true;
>  
> +

I think just one empty line is enough, no need for two.

> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/gomp/declare-variant-any.c
> @@ -0,0 +1,10 @@
> +extern int f1 (int);
> +extern int f2 (int);
> +extern int f3 (int);
> +extern int f4 (int);
> +
> +#pragma omp declare variant (f1) match (device={kind(any,gpu)})  /* { dg-error "no other trait-property may be specified" } */
> +#pragma omp declare variant (f2) match (device={kind(cpu,"any")})  /* { dg-error "no other trait-property may be specified" } */
> +#pragma omp declare variant (f3) match (device={kind("any"),arch(x86_64)})  /* { dg-error "no other trait-property may be specified" } */
> +#pragma omp declare variant (f4) match (device={arch(x86_64),kind(any)})  /* { dg-error "no other trait-property may be specified" } */

I think also testing the device={kind(any,any)} and device={kind("any",any)}
and device={kind(any,"any"))} would be useful.
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/gomp/declare-variant-any.f90
> @@ -0,0 +1,28 @@
> +integer function f1 (x)
> +  integer, intent(in) :: x
> +  f1 = x + 1
> +end function
> +integer function f2 (x)
> +  integer, intent(in) :: x
> +  f2 = x + 2
> +end function
> +integer function f3 (x)
> +  integer, intent(in) :: x
> +  f3 = x + 3
> +end function
> +integer function f4 (x)
> +  integer, intent(in) :: x
> +  f4 = x + 4
> +end function
> +
> +integer function f (x)
> +  integer, intent(in) :: x
> +
> +  !$omp declare variant (f1) match (device={kind(any,gpu)})  ! { dg-error "no other trait-property may be specified" }
> +  !$omp declare variant (f2) match (device={kind(cpu,"any")})  ! { dg-error "no other trait-property may be specified" }
> +  !$omp declare variant (f3) match (device={kind("any"),arch(x86_64)})  ! { dg-error "no other trait-property may be specified" }
> +  !$omp declare variant (f4) match (device={arch(x86_64),kind(any)})  ! { dg-error "no other trait-property may be specified" }
> +
> +  f = x
> +end function  
> +

Here too.  And, please avoid empty line at the end of test.

LGTM with those nits fixed.

	Jakub
  

On 9/9/24 05:01, Jakub Jelinek wrote:
> 
> I think also testing the device={kind(any,any)} and device={kind("any",any)}
> and device={kind(any,"any"))} would be useful.

Hmmm, it looks like GCC does not presently check for the restriction

"Each trait-property may only be specified once in a trait selector 
other than those in the construct selector set."

I'll have to re-work the patch again to add that.  :-S

-Sandra
  

On Mon, Sep 09, 2024 at 02:55:25PM -0600, Sandra Loosemore wrote:
> On 9/9/24 05:01, Jakub Jelinek wrote:
> > 
> > I think also testing the device={kind(any,any)} and device={kind("any",any)}
> > and device={kind(any,"any"))} would be useful.
> 
> Hmmm, it looks like GCC does not presently check for the restriction
> 
> "Each trait-property may only be specified once in a trait selector other
> than those in the construct selector set."

Seems that restriction is new in 5.1 (which is why it hasn't been checked in
the original code).

	Jakub
  

On 9/9/24 14:55, Sandra Loosemore wrote:
> On 9/9/24 05:01, Jakub Jelinek wrote:
>>
>> I think also testing the device={kind(any,any)} and 
>> device={kind("any",any)}
>> and device={kind(any,"any"))} would be useful.
> 
> Hmmm, it looks like GCC does not presently check for the restriction
> 
> "Each trait-property may only be specified once in a trait selector 
> other than those in the construct selector set."
> 
> I'll have to re-work the patch again to add that.  :-S

Here's the revised patch, covering both restrictions.  OK to commit?

-Sandra
  

On Wed, Sep 18, 2024 at 02:50:39PM -0600, Sandra Loosemore wrote:
> Here's the revised patch, covering both restrictions.  OK to commit?

Thanks.

> +	  /* Each trait-property can only be specified once in a trait-selector
> +	     other than the construct selector set.  FIXME: only handles
> +	     name-list properties, not clause-list properties, since the
> +	     "requires" selector is not implemented yet (PR 113067).  */
> +	  if (tss_code != OMP_TRAIT_SET_CONSTRUCT)
> +	    for (tree p1 = OMP_TS_PROPERTIES (ts); p1; p1 = TREE_CHAIN (p1))
> +	      {
> +		if (OMP_TP_NAME (p1) != OMP_TP_NAMELIST_NODE)
> +		  break;
> +		const char *n1 = omp_context_name_list_prop (p1);
> +		if (!n1)
> +		  continue;
> +		for (tree p2 = TREE_CHAIN (p1); p2; p2 = TREE_CHAIN (p2))
> +		  {
> +		    const char *n2 = omp_context_name_list_prop (p2);
> +		    if (!n2)
> +		      continue;
> +		    if (!strcmp (n1, n2))
> +		      {
> +			error_at (loc,
> +				  "trait-property %qs specified more "
> +				  "than once in %qs selector",
> +				  n1, OMP_TS_NAME (ts));
> +			return error_mark_node;
> +		      }
> +		  }
> +	      }

This is O(n^2) in number of properties, but let's hope people are sane and
don't try:
#define A(x) n#x,
#define B(x) A(x##0) A(x##1) A(x##2) A(x##3) A(x##4) A(x##5) A(x##6) A(x##7) A(x##8) A(x##9)
#define C(x) B(x##0) B(x##1) B(x##2) B(x##3) B(x##4) B(x##5) B(x##6) B(x##7) B(x##8) B(x##9)
#define D(x) C(x##0) C(x##1) C(x##2) C(x##3) C(x##4) C(x##5) C(x##6) C(x##7) C(x##8) C(x##9)
#define E(x) D(x##0) D(x##1) D(x##2) D(x##3) D(x##4) D(x##5) D(x##6) D(x##7) D(x##8) D(x##9)
isa(E(1), avx512f)
or something similar.
Otherwise, we'd need a hash table if the list is longer than say 20 entries
(of course, that could be detected in the first inner loop and switch for
that to the different implementation).

> +
> +	  /* Check for unknown properties.  */
>  	  if (omp_ts_map[ts_code].valid_properties == NULL)
>  	    continue;
> -

Why?

Otherwise LGTM.

	Jakub
  

On 9/20/24 01:41, Jakub Jelinek wrote:
>> +
>> +	  /* Check for unknown properties.  */
>>   	  if (omp_ts_map[ts_code].valid_properties == NULL)
>>   	    continue;
>> -
> 
> Why?

Why what?  I made this change because when I added another check in this 
loop I was temporarily confused about the control flow and I thought 
this would help.  If you're asking why it's doing the null check here, 
it's because it doesn't make sense to check properties on all selectors, 
like properties that can have implementation-defined values for other 
compilers.

-Sandra
  

On Sat, Sep 21, 2024 at 08:08:29PM -0600, Sandra Loosemore wrote:
> On 9/20/24 01:41, Jakub Jelinek wrote:
> > > +
> > > +	  /* Check for unknown properties.  */
> > >   	  if (omp_ts_map[ts_code].valid_properties == NULL)
> > >   	    continue;
> > > -
> > 
> > Why?
> 
> Why what?  I made this change because when I added another check in this
> loop I was temporarily confused about the control flow and I thought this
> would help.  If you're asking why it's doing the null check here, it's
> because it doesn't make sense to check properties on all selectors, like
> properties that can have implementation-defined values for other compilers.

I meant why the empty line was removed?

	Jakub
  

On 9/21/24 22:52, Jakub Jelinek wrote:
> On Sat, Sep 21, 2024 at 08:08:29PM -0600, Sandra Loosemore wrote:
>> On 9/20/24 01:41, Jakub Jelinek wrote:
>>>> +
>>>> +	  /* Check for unknown properties.  */
>>>>    	  if (omp_ts_map[ts_code].valid_properties == NULL)
>>>>    	    continue;
>>>> -
>>>
>>> Why?
>>
>> Why what?  I made this change because when I added another check in this
>> loop I was temporarily confused about the control flow and I thought this
>> would help.  If you're asking why it's doing the null check here, it's
>> because it doesn't make sense to check properties on all selectors, like
>> properties that can have implementation-defined values for other compilers.
> 
> I meant why the empty line was removed?

To make it more clear that the null check is part of the code chunk to 
diagnose unknown properties -- each of which is now formatted the same 
way, with a blank line, comment to explain what it's doing, and no blank 
lines within the chunk.

-Sandra
  

On Sun, Sep 22, 2024 at 08:45:40AM -0600, Sandra Loosemore wrote:
> On 9/21/24 22:52, Jakub Jelinek wrote:
> > On Sat, Sep 21, 2024 at 08:08:29PM -0600, Sandra Loosemore wrote:
> > > On 9/20/24 01:41, Jakub Jelinek wrote:
> > > > > +
> > > > > +	  /* Check for unknown properties.  */
> > > > >    	  if (omp_ts_map[ts_code].valid_properties == NULL)
> > > > >    	    continue;
> > > > > -
> > > > 
> > > > Why?
> > > 
> > > Why what?  I made this change because when I added another check in this
> > > loop I was temporarily confused about the control flow and I thought this
> > > would help.  If you're asking why it's doing the null check here, it's
> > > because it doesn't make sense to check properties on all selectors, like
> > > properties that can have implementation-defined values for other compilers.
> > 
> > I meant why the empty line was removed?
> 
> To make it more clear that the null check is part of the code chunk to
> diagnose unknown properties -- each of which is now formatted the same way,
> with a blank line, comment to explain what it's doing, and no blank lines
> within the chunk.

Ok.

	Jakub