Re: [Patch] OpenMP: Support acquires/release in 'omp require atomic_default_mem_order'

2023-12-08 Thread Jakub Jelinek 
On Tue, Nov 28, 2023 at 12:28:05PM +0100, Tobias Burnus wrote:
> I stumbled over this omission when looking at Sandra's patch. It turned out 
> that this is
> a new OpenMP 5.2 feature - probably added to simplify/unify the syntax. I 
> guess the reason
> that release/acquire wasn't added before is that it cannot be universally be 
> used - read/write
> do only accept one of them.

I thought when this was discussed that it was meant to behave right (choose
some more appropriate memory model if one was not allowed), but reading 5.2
I think that is not what ended up in the spec, because [213:11-13] says that
atomic_default_mem_order is as if the argument appeared on any atomic
directive without explicit mem-order clause and atomic directive has the
[314:9-10] restrictions.

I'd bring this to omp-lang whether it was really meant that
#pragma omp requires atomic_default_mem_order (release)
int foo (int *p) {
  int t;
  #pragma omp atomic read
t = *p;
  return t;
}
and
#pragma omp requires atomic_default_mem_order (acquire)
void bar (int *p) {
  #pragma omp atomic write
*p = 0;
}
are meant to be invalid.

Another comment, atomic_default_mem_order arguments aren't handled
just in the requires parsing, but also in context selectors.
So, the additions would need to be reflected in
omp_check_context_selector and omp_context_selector_matches
as well.

Jakub

[Patch] OpenMP: Support acquires/release in 'omp require atomic_default_mem_order'

2023-11-28 Thread Tobias Burnus 

I stumbled over this omission when looking at Sandra's patch. It turned out 
that this is
a new OpenMP 5.2 feature - probably added to simplify/unify the syntax. I guess 
the reason
that release/acquire wasn't added before is that it cannot be universally be 
used - read/write
do only accept one of them.

However, as a compilation unit might only/mostly use read (and update) or write 
(and update),
that can be fine - especially as overriding the default clause is still 
possible.

It is not quite clear to me why, but the current patch also fixes a bug 
regarding the
diagnostic message for gfortran.dg/gomp/requires-5.f90. (I think I could find 
out, but
as it changed to the better...)

Comments, suggestions?

Tobias
-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas 
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht 
München, HRB 106955
OpenMP: Support acquires/release in 'omp require atomic_default_mem_order'

This is an OpenMP 5.2 feature.

gcc/c/ChangeLog:

	* c-parser.cc (c_parser_omp_requires): Handle acquires/release
	in atomic_default_mem_order clause.
	(c_parser_omp_atomic): Update. 

gcc/cp/ChangeLog:

	* parser.cc (cp_parser_omp_requires): Handle acquires/release
	in atomic_default_mem_order clause.
	(cp_parser_omp_atomic): Update.

gcc/fortran/ChangeLog:

	* gfortran.h (enum gfc_omp_requires_kind): Add
	OMP_REQ_ATOMIC_MEM_ORDER_ACQUIRE and OMP_REQ_ATOMIC_MEM_ORDER_RELEASE.
	(gfc_namespace): Add a 7th bit to omp_requires.
	* module.cc (enum ab_attribute): Add AB_OMP_REQ_MEM_ORDER_ACQUIRE
	and AB_OMP_REQ_MEM_ORDER_RELEASE
	(mio_symbol_attribute): Handle it.
	* openmp.cc (gfc_omp_requires_add_clause): Update for acquire/release.
	(gfc_match_omp_requires): Likewise.
	(gfc_match_omp_atomic): Handle them for atomic_default_mem_order.
	* parse.cc: Likewise.

gcc/testsuite/ChangeLog:

	* c-c++-common/gomp/requires-3.c: Update for now valid code.
	* gfortran.dg/gomp/requires-3.f90: Likewise.
	* gfortran.dg/gomp/requires-2.f90: Update dg-error.
	* gfortran.dg/gomp/requires-5.f90: Likewise.
	* c-c++-common/gomp/requires-5.c: New test.
	* c-c++-common/gomp/requires-6.c: New test.
	* c-c++-common/gomp/requires-7.c: New test.
	* c-c++-common/gomp/requires-8.c: New test.
	* gfortran.dg/gomp/requires-10.f90: New test.
	* gfortran.dg/gomp/requires-11.f90: New test.

 gcc/c/c-parser.cc  | 32 +++-
 gcc/cp/parser.cc   | 32 +++-
 gcc/fortran/gfortran.h | 22 ++-
 gcc/fortran/module.cc  | 19 +
 gcc/fortran/openmp.cc  | 53 +-
 gcc/fortran/parse.cc   |  8 
 gcc/testsuite/c-c++-common/gomp/requires-3.c   |  8 ++--
 gcc/testsuite/c-c++-common/gomp/requires-5.c   | 23 +++
 gcc/testsuite/c-c++-common/gomp/requires-6.c   | 23 +++
 gcc/testsuite/c-c++-common/gomp/requires-7.c   | 11 ++
 gcc/testsuite/c-c++-common/gomp/requires-8.c   | 14 +++
 gcc/testsuite/gfortran.dg/gomp/requires-10.f90 | 36 +
 gcc/testsuite/gfortran.dg/gomp/requires-11.f90 | 31 +++
 gcc/testsuite/gfortran.dg/gomp/requires-2.f90  |  2 +-
 gcc/testsuite/gfortran.dg/gomp/requires-3.f90  |  7 ++--
 gcc/testsuite/gfortran.dg/gomp/requires-5.f90  |  2 +-
 16 files changed, 291 insertions(+), 32 deletions(-)

diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
index df9a07928b5..5700c49 100644
--- a/gcc/c/c-parser.cc
+++ b/gcc/c/c-parser.cc
@@ -20896,6 +20896,28 @@ c_parser_omp_atomic (location_t loc, c_parser *parser, bool openacc)
 	case OMP_MEMORY_ORDER_SEQ_CST:
 	  memory_order = OMP_MEMORY_ORDER_SEQ_CST;
 	  break;
+	case OMP_MEMORY_ORDER_ACQUIRE:
+	  if (code == NOP_EXPR)  /* atomic write */
+	{
+	  error_at (loc, "%<#pragma omp atomic write%> incompatible with "
+			 "% clause implicitly provided by a "
+			 "% directive");
+	  memory_order = OMP_MEMORY_ORDER_SEQ_CST;
+	}
+	  else
+	memory_order = OMP_MEMORY_ORDER_ACQUIRE;
+	  break;
+	case OMP_MEMORY_ORDER_RELEASE:
+	  if (code == OMP_ATOMIC_READ)
+	{
+	  error_at (loc, "%<#pragma omp atomic read%> incompatible with "
+			 "% clause implicitly provided by a "
+			 "% directive");
+	  memory_order = OMP_MEMORY_ORDER_SEQ_CST;
+	}
+	  else
+	memory_order = OMP_MEMORY_ORDER_RELEASE;
+	  break;
 	case OMP_MEMORY_ORDER_ACQ_REL:
 	  switch (code)
 	{
@@ -25724,15 +25746,21 @@ c_parser_omp_requires (c_parser *parser)
 		  else if (!strcmp (p, "relaxed"))
 			this_req
 			  = (enum omp_requires) OMP_MEMORY_ORDER_RELAXED;
+		  else if (!strcmp (p, "release"))
+			this_req
+			  = (enum omp_requires) OMP_MEMORY_ORDER_RELEASE;
 		  else if (!strcmp (p, "acq_rel"))
 			this_req
 			  = (enum omp_requires) OMP_MEMORY_ORDER_ACQ_REL;
+