OpenMP: Accept argument to depobj's destroy clause

  I stumbled over this trivial omission which blocks some testcases.

I am not sure whether I have solved the is-same-expr most elegantly,
but I did loosely follow the duplicated-entry check for 'map'. As that's
a restriction to the user, we don't have to catch all and I hope the code
catches the most important violations, doesn't ICE and does not reject
valid code. At least for all real-world code it should™ work, but I
guess for lvalue expressions involving function calls it probably doesn't.

Thoughts, comments?

Tobias

PS: GCC accepts an lvalue expression in C/C++ and only a identifier
for a scalar variable in Fortran, i.e. neither array elements nor
structure components.

Which variant is right depends whether one reads OpenMP 5.1 (lvalue expr,
scalar variable) or 5.2 (variable without permitting array sections or
structure components) - whereas TR12 has the same but talks about
locator list items in one restriction. For the OpenMP mess, see spec
issue #3739.
-----------------
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
  

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,
> but I did loosely follow the duplicated-entry check for 'map'. As that's
> a restriction to the user, we don't have to catch all and I hope the code
> catches the most important violations, doesn't ICE and does not reject
> valid code. At least for all real-world code it should™ work, but I
> guess for lvalue expressions involving function calls it probably doesn't.
> 
> Thoughts, comments?
> 
> Tobias
> 
> PS: GCC accepts an lvalue expression in C/C++ and only a identifier
> for a scalar variable in Fortran, i.e. neither array elements nor
> structure components.
> 
> Which variant is right depends whether one reads OpenMP 5.1 (lvalue expr,
> scalar variable) or 5.2 (variable without permitting array sections or
> structure components) - whereas TR12 has the same but talks about
> locator list items in one restriction. For the OpenMP mess, see spec
> issue #3739.
> -----------------
> 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: 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                           | 57 ++++++++++++++++++++++++++-
>  gcc/cp/parser.cc                            | 60 ++++++++++++++++++++++++++++-
>  gcc/fortran/openmp.cc                       | 15 +++++++-
>  gcc/testsuite/c-c++-common/gomp/depobj-3.c  | 40 +++++++++++++++++++
>  gcc/testsuite/gfortran.dg/gomp/depobj-3.f90 | 18 +++++++++
>  libgomp/libgomp.texi                        |  2 +-
>  6 files changed, 188 insertions(+), 4 deletions(-)
> 
> diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
> index 371dd29557b..378647c1a67 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,59 @@ 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;
> +	      /* OpenMP requires that the two expressions are identical; catch
> +		 the most common mismatches.  */
> +	      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)
> +		{
> +		  tree t = depobj;
> +		  tree t2 = build_unary_op (EXPR_LOC_OR_LOC (destobj, c_loc),
> +					    ADDR_EXPR, destobj, false);
> +		  if (t2 != error_mark_node)
> +		    t2 = build_indirect_ref (EXPR_LOC_OR_LOC (t2, c_loc),
> +					     t2, RO_UNARY_STAR);

Please watch indentation, seems there is a mix of 8 spaces vs. tabs:

> +		  while (TREE_CODE (t) == COMPONENT_REF
> +			 || TREE_CODE (t) == ARRAY_REF)
> +                    {
> +		      t = TREE_OPERAND (t, 0);
> +		      if (TREE_CODE (t) == MEM_REF || INDIRECT_REF_P (t))
> +			{
> +			  t = TREE_OPERAND (t, 0);
> +			  STRIP_NOPS (t);
> +			  if (TREE_CODE (t) == POINTER_PLUS_EXPR)
> +			    t = TREE_OPERAND (t, 0);
> +                        }
> +		    }
> +		  while (TREE_CODE (t2) == COMPONENT_REF
> +			 || TREE_CODE (t2) == ARRAY_REF)
> +                    {
> +		      t2 = TREE_OPERAND (t2, 0);
> +		      if (TREE_CODE (t2) == MEM_REF || INDIRECT_REF_P (t2))
> +			{
> +			  t2 = TREE_OPERAND (t2, 0);
> +			  STRIP_NOPS (t2);
> +			  if (TREE_CODE (t2) == POINTER_PLUS_EXPR)
> +			    t2 = TREE_OPERAND (t2, 0);
> +                        }
> +		    }
> +		  if (DECL_UID (t) != DECL_UID (t2))

Nothing checks that t and t2 here are decls.  Use operand_equal_p instead?

> +		    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);
> +	    }
> +	}
> +		  if (t != t2)

And here too?

	Jakub
  

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
  

On Thu, Nov 23, 2023 at 04:21:50PM +0100, Tobias Burnus wrote:
> @@ -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))

There is also OEP_LEXICOGRAPHIC which could be used in addition to that.
The question is if we want to consider say
#pragma depobj (a[++i]) destroy (a[++i])
as same or different (similarly a[foo ()] in both cases).
A function could at least in theory return the same value, for other
side-effects there is some wiggle room in unspecified number of times how
many the side-effects of clauses are evaluated (and for destroy we really
don't intend to evaluate them at all for the clause, just for the directive
argument).

	Jakub
  

Hi Jakub,

On 23.11.23 16:32, Jakub Jelinek wrote:
> On Thu, Nov 23, 2023 at 04:21:50PM +0100, Tobias Burnus wrote:
>> @@ -21663,7 +21666,25 @@ c_parser_omp_depobj (c_parser *parser)
>> +          else if (depobj != error_mark_node
>> +                   && !operand_equal_p (destobj, depobj,
>> +                                        OEP_MATCH_SIDE_EFFECTS))
> There is also OEP_LEXICOGRAPHIC which could be used in addition to that.
> The question is if we want to consider say
> #pragma depobj (a[++i]) destroy (a[++i])
> as same or different (similarly a[foo ()] in both cases).

I don't think that we want to permit those; I think there is (a) the
question whether both expressions have to be evaluated or not and (b),
if so, in which order and (c), if the run-time result is different,
whether both have to be 'destory'ed or only one of them (which one?).

Additionally, 'destroy-var must refer to the same depend object as the
depobj argument of the construct.' cannot be fulfilled if one is
evaluated before the other and both use the same 'i' in your case.

Thus, I do not really see an argument for permitting OEP_LEXICOGRAPHIC.

I think permitting 'volatile' does make sense, in a ways, as a
hyper-careful user might actually write such code.

[I wonder whether the OpenMP wording would permit 'omp depobj(obj)
destroy(f())' with 'auto f() { return obj; }' – but I am sure we don't
want to permit it in the compiler.]

Tobias

PS: In any case, I find it confusing to require that the same
variable/lvalue-expression has to be specified twice. The (only) pro is
that for 'omp interop destroy(...)' the argument is required and for
consistency of the 'destroy' clause, an argument now must be (always)
specified. But that leads to the odd 'omp depobj(obj) destroy(obj)',
which is really ugly. (In 5.2 the arg to destroy is optional but
omitting it is deprecated; hence, in OpenMP 6.0 (TR11, TR12) the
argument must be specified twice.)

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

On Thu, Nov 23, 2023 at 04:59:16PM +0100, Tobias Burnus wrote:
> > There is also OEP_LEXICOGRAPHIC which could be used in addition to that.
> > The question is if we want to consider say
> > #pragma depobj (a[++i]) destroy (a[++i])
> > as same or different (similarly a[foo ()] in both cases).
> 
> I don't think that we want to permit those; I think there is (a) the
> question whether both expressions have to be evaluated or not and (b),
> if so, in which order and (c), if the run-time result is different,
> whether both have to be 'destory'ed or only one of them (which one?).

Well, we don't need to destroy two, because it would be UB if the two
aren't the same.  This is just about diagnostics if user messed stuff
up unintentionally.
The function call case can be the same very easily, just
int foo () { return 0; }
omp_depend_t a[2];
...
#pragma omp depobj (a[foo ()]) destroy (a[foo ()])
or
int i = 0;
#pragma omp depobj (a[((++i) * 2) & 1]) destroy (a[((++i) * 2) & 1])
The former may evaluate the function call multiple times, but user arranges
for it to do the same thing in each case, in the second case while there
are side-effects, they don't really matter for the value, just in whether
i after this pragma has value of 0, 1, 2 or something else (but if again
nothing cares about that value afterwards...).

The question is if same (I admit I haven't looked up the exact wording now)
means lexically same, or anything that has the same value, etc.
Because e.g.
omp_depend_t a;
...
omp_depend_t *p = &a;
#pragma omp depobj (a) destroy (p[0])
is the same value but not lexically same.

IMHO the argument to destroy clause shouldn't have ever been allowed, it is
only unnecessary extra pain.

	Jakub