Re: [Patch] OpenMP: Accept argument to depobj's destroy clause

[Tobias Burnus <tobias@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 1164 bytes --]

Hi Jakub,

On 23.11.23 15:32, Jakub Jelinek wrote:
> On Thu, Nov 23, 2023 at 03:21:41PM +0100, Tobias Burnus wrote:
>> I stumbled over this trivial omission which blocks some testcases.
>> I am not sure whether I have solved the is-same-expr most elegantly,
Answer: I didn't - as expected.
> + if (DECL_UID (t) != DECL_UID (t2))
> Nothing checks that t and t2 here are decls.  Use operand_equal_p instead?

Yes – I think I can simply use this instead all the other checks. That is
the function I was looking for but couldn't find before.

(I even have use the function before for PR108545.)

I decided that volatileness is fine and using twice a volatile function is
Okay according to the spec - hence, I permit this in addition. (One can
argue about it - but as specifying both is mandatory in OpenMP 6.0, it
seems to make sense.)

Revised version attached.

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

[-- Attachment #2: depobj-destroy-v2.diff --]
[-- Type: text/x-patch, Size: 8510 bytes --]

OpenMP: Accept argument to depobj's destroy clause

Since OpenMP 5.2, the destroy clause takes an depend argument as argument;
for the depobj directive, it the new argument is optional but, if present,
it must be identical to the directive's argument.

gcc/c/ChangeLog:

	* c-parser.cc (c_parser_omp_depobj): Accept optionally an argument
	to the destroy clause.

gcc/cp/ChangeLog:

	* parser.cc (cp_parser_omp_depobj): Accept optionally an argument
	to the destroy clause.

gcc/fortran/ChangeLog:

	* openmp.cc (gfc_match_omp_depobj): Accept optionally an argument
	to the destroy clause.

libgomp/ChangeLog:

	* libgomp.texi (5.2 Impl. Status): An argument to the destroy clause
	is now supported.

gcc/testsuite/ChangeLog:

	* c-c++-common/gomp/depobj-3.c: New test.
	* gfortran.dg/gomp/depobj-3.f90: New test.

 gcc/c/c-parser.cc                           | 23 +++++++++++++-
 gcc/cp/parser.cc                            | 24 ++++++++++++++-
 gcc/fortran/openmp.cc                       | 15 ++++++++-
 gcc/testsuite/c-c++-common/gomp/depobj-3.c  | 47 +++++++++++++++++++++++++++++
 gcc/testsuite/gfortran.dg/gomp/depobj-3.f90 | 18 +++++++++++
 libgomp/libgomp.texi                        |  2 +-
 6 files changed, 125 insertions(+), 4 deletions(-)

diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
index 371dd29557b..006aee3e93f 100644
--- a/gcc/c/c-parser.cc
+++ b/gcc/c/c-parser.cc
@@ -21605,6 +21605,9 @@ c_parser_omp_critical (location_t loc, c_parser *parser, bool *if_p)
      destroy
      update (dependence-type)
 
+   OpenMP 5.2 additionally:
+     destroy ( depobj )
+
    dependence-type:
      in
      out
@@ -21663,7 +21666,25 @@ c_parser_omp_depobj (c_parser *parser)
 	    clause = error_mark_node;
 	}
       else if (!strcmp ("destroy", p))
-	kind = OMP_CLAUSE_DEPEND_LAST;
+	{
+	  matching_parens c_parens;
+	  kind = OMP_CLAUSE_DEPEND_LAST;
+	  if (c_parser_next_token_is (parser, CPP_OPEN_PAREN)
+	      && c_parens.require_open (parser))
+	    {
+	      tree destobj = c_parser_expr_no_commas (parser, NULL).value;
+	      if (!lvalue_p (destobj))
+		error_at (EXPR_LOC_OR_LOC (destobj, c_loc),
+			  "%<destrory%> expression is not lvalue expression");
+	      else if (depobj != error_mark_node
+		       && !operand_equal_p (destobj, depobj,
+					    OEP_MATCH_SIDE_EFFECTS))
+		error_at (EXPR_LOC_OR_LOC (destobj, c_loc),
+			  "the %<destroy%> expression %qE must be the same as "
+			  "the %<depobj%> argument %qE", destobj, depobj);
+	      c_parens.skip_until_found_close (parser);
+	    }
+	}
       else if (!strcmp ("update", p))
 	{
 	  matching_parens c_parens;
diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index f6d088bc73f..1fca6bff795 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -43173,6 +43173,9 @@ cp_parser_omp_critical (cp_parser *parser, cp_token *pragma_tok, bool *if_p)
      destroy
      update (dependence-type)
 
+   OpenMP 5.2 additionally:
+     destroy ( depobj )
+
    dependence-type:
      in
      out
@@ -43219,7 +43222,26 @@ cp_parser_omp_depobj (cp_parser *parser, cp_token *pragma_tok)
 	    clause = error_mark_node;
 	}
       else if (!strcmp ("destroy", p))
-	kind = OMP_CLAUSE_DEPEND_LAST;
+	{
+	  kind = OMP_CLAUSE_DEPEND_LAST;
+	  matching_parens c_parens;
+	  if (cp_lexer_next_token_is (parser->lexer, CPP_OPEN_PAREN)
+	      && c_parens.require_open (parser))
+	    {
+	      tree destobj = cp_parser_assignment_expression (parser);
+	      if (depobj != error_mark_node
+		  && destobj != error_mark_node
+		  && !operand_equal_p (destobj, depobj, OEP_MATCH_SIDE_EFFECTS))
+		error_at (EXPR_LOC_OR_LOC (destobj, c_loc),
+			  "the %<destroy%> expression %qE must be the same as "
+			  "the %<depobj%> argument %qE", destobj, depobj);
+	      if (!c_parens.require_close (parser))
+		cp_parser_skip_to_closing_parenthesis (parser,
+						       /*recovering=*/true,
+						       /*or_comma=*/false,
+						       /*consume_paren=*/true);
+	    }
+	}
       else if (!strcmp ("update", p))
 	{
 	  matching_parens c_parens;
diff --git a/gcc/fortran/openmp.cc b/gcc/fortran/openmp.cc
index 2e2e23d567b..5f37f3a2586 100644
--- a/gcc/fortran/openmp.cc
+++ b/gcc/fortran/openmp.cc
@@ -4731,10 +4731,23 @@ gfc_match_omp_depobj (void)
 	  goto error;
 	}
     }
-  else if (gfc_match ("destroy") == MATCH_YES)
+  else if (gfc_match ("destroy ") == MATCH_YES)
     {
+      gfc_expr *destroyobj = NULL;
       c = gfc_get_omp_clauses ();
       c->destroy = true;
+
+      if (gfc_match (" ( %v ) ", &destroyobj) == MATCH_YES)
+	{
+	  if (destroyobj->symtree != depobj->symtree)
+	    {
+	      gfc_error ("The same depend object must be used as DEPOBJ argument at %L"
+			 " and as DESTROY argument at %L", &depobj->where,
+			 &destroyobj->where);
+	      return MATCH_ERROR;
+	    }
+	  gfc_free_expr (destroyobj);
+	}
     }
   else if (gfc_match_omp_clauses (&c, omp_mask (OMP_CLAUSE_DEPEND), true, false)
 	   != MATCH_YES)
diff --git a/gcc/testsuite/c-c++-common/gomp/depobj-3.c b/gcc/testsuite/c-c++-common/gomp/depobj-3.c
new file mode 100644
index 00000000000..2df66f4ebf1
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/gomp/depobj-3.c
@@ -0,0 +1,47 @@
+typedef struct __attribute__((__aligned__ (sizeof (void *)))) omp_depend_t {
+  char __omp_depend_t__[2 * sizeof (void *)];
+} omp_depend_t;
+
+void
+f ()
+{
+  omp_depend_t obj2;
+  struct { omp_depend_t c; } s;
+  float a;
+  #pragma omp depobj(s.c) depend(inout: a)
+
+  #pragma omp depobj(s.c) destroy(s.c) /* OK */
+
+  #pragma omp depobj(s.c) destroy(obj2)
+/* { dg-error "the 'destroy' expression 'obj2' must be the same as the 'depobj' argument 's.c'" "" { target c } .-1 } */
+/* { dg-error "the 'destroy' expression 'obj2' must be the same as the 'depobj' argument 's.f\\(\\)::<unnamed struct>::c'" "" { target c++ } .-2 } */
+}
+
+void
+g ()
+{
+  volatile omp_depend_t obj3;
+  #pragma omp depobj(obj3) destroy(obj3)
+}
+
+int
+main ()
+{
+   float a;
+   omp_depend_t obj;
+
+   #pragma omp depobj(obj) depend(inout: a)
+
+   #pragma omp depobj(obj) destroy(obj) /* OK */
+
+   #pragma omp depobj(obj) destroy(a + 5) 
+/* { dg-error "'destrory' expression is not lvalue expression" "" { target c } .-1 } */
+/* { dg-error "the 'destroy' expression '\\(a \\+ \\(float\\)5\\)' must be the same as the 'depobj' argument 'obj'" "" { target c++ } .-2 } */
+
+   #pragma omp depobj(obj+5) destroy(a) 
+/* { dg-error "invalid operands to binary \\+ \\(have 'omp_depend_t' and 'int'\\)" "" { target c } .-1 } */
+/* { dg-error "no match for 'operator\\+' in 'obj \\+ 5' \\(operand types are 'omp_depend_t' and 'int'\\)" "" { target c++ } .-2 } */
+
+   #pragma omp depobj(obj) destroy(a)  /* { dg-error "the 'destroy' expression 'a' must be the same as the 'depobj' argument 'obj'" } */
+   return 0;
+}
diff --git a/gcc/testsuite/gfortran.dg/gomp/depobj-3.f90 b/gcc/testsuite/gfortran.dg/gomp/depobj-3.f90
new file mode 100644
index 00000000000..a0020014f9e
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/gomp/depobj-3.f90
@@ -0,0 +1,18 @@
+! { dg-do compile { target { fortran_integer_16 || ilp32 } } }
+! omp_depend_kind = 2*intptr_t --> 16 (128 bit) on 64bit-pointer systems
+!                              --> 8  (128 bit) on 32bit-pointer systems
+subroutine f1
+  !use omp_lib   ! N/A in gcc/testsuite
+  use iso_c_binding, only: c_intptr_t
+  implicit none
+  integer, parameter :: omp_depend_kind = 2*c_intptr_t
+  integer :: a, b
+  integer(kind=omp_depend_kind) :: depobj, depobj1(5)
+
+  !$omp depobj(depobj) destroy
+
+  !$omp depobj(depobj) destroy( depobj)
+
+  !$omp depobj(depobj) destroy( depobj2)  ! { dg-error "The same depend object must be used as DEPOBJ argument at .1. and as DESTROY argument at .2." }
+  !$omp depobj(depobj) destroy( a)  ! { dg-error "The same depend object must be used as DEPOBJ argument at .1. and as DESTROY argument at .2." }
+end
diff --git a/libgomp/libgomp.texi b/libgomp/libgomp.texi
index 2f6227c94b2..e5fe7af76af 100644
--- a/libgomp/libgomp.texi
+++ b/libgomp/libgomp.texi
@@ -387,7 +387,7 @@ to address of matching mapped list item per 5.1, Sect. 2.21.7.2 @tab N @tab
       @code{-Wall}).  Unknown clauses are always rejected with an error.}
 @item Clauses on @code{end} directive can be on directive @tab Y @tab
 @item @code{destroy} clause with destroy-var argument on @code{depobj}
-      @tab N @tab
+      @tab Y @tab
 @item Deprecation of no-argument @code{destroy} clause on @code{depobj}
       @tab N @tab
 @item @code{linear} clause syntax changes and @code{step} modifier @tab Y @tab