Re: [Patch] OpenMP: Reject non-const 'condition' trait in Fortran (was: [Patch] OpenMP: Handle DECL_ASSEMBLER_NAME with 'declare variant')

2024-03-04 Thread Thomas Schwinge
Hi Tobias!

On 2024-02-13T18:31:02+0100, Tobias Burnus  wrote:
> --- a/gcc/fortran/openmp.cc
> +++ b/gcc/fortran/openmp.cc

> +   /* Device number must be conforming, which includes
> +  omp_initial_device (-1) and omp_invalid_device (-4).  */
> +   if (property_kind == OMP_TRAIT_PROPERTY_DEV_NUM_EXPR
> +   && otp->expr->expr_type == EXPR_CONSTANT
> +   && mpz_sgn (otp->expr->value.integer) < 0
> +   && mpz_cmp_si (otp->expr->value.integer, -1) != 0
> +   && mpz_cmp_si (otp->expr->value.integer, -4) != 0)
> + {
> +   gfc_error ("property must be a conforming device number "
> +  "at %C");

Instead of magic numbers, shouldn't this use 'include/gomp-constants.h':

/* We have a compatibility issue.  OpenMP 5.2 introduced
   omp_initial_device with value of -1 which clashes with our
   GOMP_DEVICE_ICV, so we need to remap user supplied device
   ids, -1 (aka omp_initial_device) to GOMP_DEVICE_HOST_FALLBACK,
   and -2 (one of many non-conforming device numbers, but with
   OMP_TARGET_OFFLOAD=mandatory needs to be treated a
   omp_invalid_device) to -3 (so that for dev_num >= -2U we can
   subtract 1).  -4 is then what we use for omp_invalid_device,
   which unlike the other non-conforming device numbers results
   in fatal error regardless of OMP_TARGET_OFFLOAD.  */
#define GOMP_DEVICE_ICV -1
#define GOMP_DEVICE_HOST_FALLBACK   -2
#define GOMP_DEVICE_INVALID -4


Grüße
 Thomas


Re: [Patch] OpenMP: Reject non-const 'condition' trait in Fortran (was: [Patch] OpenMP: Handle DECL_ASSEMBLER_NAME with 'declare variant')

2024-02-13 Thread Jakub Jelinek
On Tue, Feb 13, 2024 at 06:31:02PM +0100, Tobias Burnus wrote:
>   PR middle-end/113904
> 
> gcc/c/ChangeLog:
> 
>   * c-parser.cc (c_parser_omp_context_selector): Handle splitting of
>   OMP_TRAIT_PROPERTY_EXPR into OMP_TRAIT_PROPERTY_{DEV_NUM,BOOL}_EXPR.
> 
> gcc/cp/ChangeLog:
> 
>   * parser.cc (cp_parser_omp_context_selector): Handle splitting of
>   OMP_TRAIT_PROPERTY_EXPR into OMP_TRAIT_PROPERTY_{DEV_NUM,BOOL}_EXPR.
> 
> gcc/fortran/ChangeLog:
> 
>   * openmp.cc (gfc_match_omp_context_selector):
>   * trans-openmp.cc (gfc_trans_omp_declare_variant): Handle splitting of
>   OMP_TRAIT_PROPERTY_EXPR into OMP_TRAIT_PROPERTY_{DEV_NUM,BOOL}_EXPR.
> 
> 
> gcc/ChangeLog:
> 
>   * omp-general.cc (struct omp_ts_info): Update for splitting of
>   OMP_TRAIT_PROPERTY_EXPR into OMP_TRAIT_PROPERTYDEV_NUM,BOOL}_EXPR.

_{ missing above before DEV_NUM

>   * omp-selectors.h (enum omp_tp_type): Replace
>   OMP_TRAIT_PROPERTY_EXPR by OMP_TRAIT_PROPERTY_{DEV_NUM,BOOL}_EXPR.
> 
> gcc/testsuite/ChangeLog:
> 
>   * gfortran.dg/gomp/declare-variant-1.f90: Change 'condition' trait's
>   argument from integer to a logical expression.
>   * gfortran.dg/gomp/declare-variant-11.f90: Likewise.
>   * gfortran.dg/gomp/declare-variant-12.f90: Likewise.
>   * gfortran.dg/gomp/declare-variant-13.f90: Likewise.
>   * gfortran.dg/gomp/declare-variant-2.f90: Likewise.
>   * gfortran.dg/gomp/declare-variant-2a.f90: Likewise.
>   * gfortran.dg/gomp/declare-variant-3.f90: Likewise.
>   * gfortran.dg/gomp/declare-variant-4.f90: Likewise.
>   * gfortran.dg/gomp/declare-variant-6.f90: Likewise.
>   * gfortran.dg/gomp/declare-variant-8.f90: Likewise.
>   * gfortran.dg/gomp/declare-variant-20.f90: New test.

Otherwise LGTM.

Of course it makes me wonder to what extent we actually do support the
OpenMP 5.1 target_device device_num trait with constant or non-constant
device num, because similarly to the user condition trait if it something
that can't be decided at compile time but needs to be checked at runtime,
we need to be able to compute scores of all the variants that could be
effective depending on the device_num comparison and/or condition
evaluation and turn that into set of runtime comparisons with choices of
the variants depending on the scores.
If one is lucky, it can be device == device_num ? fn1 : fn2, but it can
be more complex than that.

Jakub



[Patch] OpenMP: Reject non-const 'condition' trait in Fortran (was: [Patch] OpenMP: Handle DECL_ASSEMBLER_NAME with 'declare variant')

2024-02-13 Thread Tobias Burnus

Jakub Jelinek wrote:

Isn't all this caused just by the missing check that condition trait has a
constant expression?

IMHO that is the way to handle it in GCC 14.


Concur – how about the following patch?

Tobias

PS: See PR113904 for follow up tasks. / Instead of '.AND.' etc. I could 
have also used some more '==', '<' etc. expressions in the modified 
examples (as should have Sandra in the initial version), but, 
fortunately, there is at least one '=='.
OpenMP: Reject non-const 'condition' trait in Fortran

OpenMP 5.0 only permits constant expressions for the 'condition' trait
in context selectors; this is relaxed in 5.2 but not implemented. In order
to avoid wrong code, it is now rejected.

Additionally, in Fortran, 'condition' should not accept an integer
expression, which is now ensured. Additionally, as 'device_num' should be
a conforming device number, there is now a check on the value.

	PR middle-end/113904

gcc/c/ChangeLog:

	* c-parser.cc (c_parser_omp_context_selector): Handle splitting of
	OMP_TRAIT_PROPERTY_EXPR into OMP_TRAIT_PROPERTY_{DEV_NUM,BOOL}_EXPR.

gcc/cp/ChangeLog:

	* parser.cc (cp_parser_omp_context_selector): Handle splitting of
	OMP_TRAIT_PROPERTY_EXPR into OMP_TRAIT_PROPERTY_{DEV_NUM,BOOL}_EXPR.

gcc/fortran/ChangeLog:

	* openmp.cc (gfc_match_omp_context_selector):
	* trans-openmp.cc (gfc_trans_omp_declare_variant): Handle splitting of
	OMP_TRAIT_PROPERTY_EXPR into OMP_TRAIT_PROPERTY_{DEV_NUM,BOOL}_EXPR.


gcc/ChangeLog:

	* omp-general.cc (struct omp_ts_info): Update for splitting of
	OMP_TRAIT_PROPERTY_EXPR into OMP_TRAIT_PROPERTYDEV_NUM,BOOL}_EXPR.
	* omp-selectors.h (enum omp_tp_type): Replace
	OMP_TRAIT_PROPERTY_EXPR by OMP_TRAIT_PROPERTY_{DEV_NUM,BOOL}_EXPR.

gcc/testsuite/ChangeLog:

	* gfortran.dg/gomp/declare-variant-1.f90: Change 'condition' trait's
	argument from integer to a logical expression.
	* gfortran.dg/gomp/declare-variant-11.f90: Likewise.
	* gfortran.dg/gomp/declare-variant-12.f90: Likewise.
	* gfortran.dg/gomp/declare-variant-13.f90: Likewise.
	* gfortran.dg/gomp/declare-variant-2.f90: Likewise.
	* gfortran.dg/gomp/declare-variant-2a.f90: Likewise.
	* gfortran.dg/gomp/declare-variant-3.f90: Likewise.
	* gfortran.dg/gomp/declare-variant-4.f90: Likewise.
	* gfortran.dg/gomp/declare-variant-6.f90: Likewise.
	* gfortran.dg/gomp/declare-variant-8.f90: Likewise.
	* gfortran.dg/gomp/declare-variant-20.f90: New test.

 gcc/c/c-parser.cc  |  3 +-
 gcc/cp/parser.cc   |  3 +-
 gcc/fortran/openmp.cc  | 30 ++---
 gcc/fortran/trans-openmp.cc|  3 +-
 gcc/omp-general.cc |  4 +-
 gcc/omp-selectors.h|  3 +-
 .../gfortran.dg/gomp/declare-variant-1.f90 |  4 +-
 .../gfortran.dg/gomp/declare-variant-11.f90|  4 +-
 .../gfortran.dg/gomp/declare-variant-12.f90| 12 ++---
 .../gfortran.dg/gomp/declare-variant-13.f90|  2 +-
 .../gfortran.dg/gomp/declare-variant-2.f90 |  8 ++--
 .../gfortran.dg/gomp/declare-variant-20.f90| 51 ++
 .../gfortran.dg/gomp/declare-variant-2a.f90|  4 +-
 .../gfortran.dg/gomp/declare-variant-3.f90 |  8 ++--
 .../gfortran.dg/gomp/declare-variant-4.f90 |  8 ++--
 .../gfortran.dg/gomp/declare-variant-6.f90 | 14 +++---
 .../gfortran.dg/gomp/declare-variant-8.f90 |  2 +-
 17 files changed, 119 insertions(+), 44 deletions(-)

diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
index c31349dae2f..3be91d666a5 100644
--- a/gcc/c/c-parser.cc
+++ b/gcc/c/c-parser.cc
@@ -24656,7 +24656,8 @@ c_parser_omp_context_selector (c_parser *parser, enum omp_tss_code set,
 		}
 	  while (1);
 	  break;
-	case OMP_TRAIT_PROPERTY_EXPR:
+	case OMP_TRAIT_PROPERTY_DEV_NUM_EXPR:
+	case OMP_TRAIT_PROPERTY_BOOL_EXPR:
 	  t = c_parser_expr_no_commas (parser, NULL).value;
 	  if (t != error_mark_node)
 		{
diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index f0c8f9c4005..68ab74d70b9 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -47984,7 +47984,8 @@ cp_parser_omp_context_selector (cp_parser *parser, enum omp_tss_code set,
 		}
 	  while (1);
 	  break;
-	case OMP_TRAIT_PROPERTY_EXPR:
+	case OMP_TRAIT_PROPERTY_DEV_NUM_EXPR:
+	case OMP_TRAIT_PROPERTY_BOOL_EXPR:
 	  /* FIXME: this is bogus, the expression need
 		 not be constant.  */
 	  t = cp_parser_constant_expression (parser);
diff --git a/gcc/fortran/openmp.cc b/gcc/fortran/openmp.cc
index 0af80d54fad..d8cce6922b0 100644
--- a/gcc/fortran/openmp.cc
+++ b/gcc/fortran/openmp.cc
@@ -5790,19 +5790,39 @@ gfc_match_omp_context_selector (gfc_omp_set_selector *oss)
 		}
 	  while (1);
 	  break;
-	case OMP_TRAIT_PROPERTY_EXPR:
+	case OMP_TRAIT_PROPERTY_DEV_NUM_EXPR:
+	case OMP_TRAIT_PROPERTY_BOOL_EXPR:
 	  if (gfc_match_expr (&otp->expr) != MATCH_YES)
 		{
 		  gfc