Message ID | 43e9e456-5114-468b-95a1-b22d8abea73c@codesourcery.com |
---|---|
State | New |
Headers | show |
Series | OpenMP: Accept argument to depobj's destroy clause | expand |
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
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); + 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)) + 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..0fff66555bf 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,62 @@ 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); + /* OpenMP requires that the two expressions are identical; catch + the most common mismatches. */ + if (depobj != error_mark_node && destobj != error_mark_node) + { + tree t = depobj; + tree t2 = destobj; + while (TREE_CODE (t) == COMPONENT_REF + || TREE_CODE (t) == ARRAY_REF + || TREE_CODE (t) == VIEW_CONVERT_EXPR) + { + t = TREE_OPERAND (t, 0); + if (REFERENCE_REF_P (t)) + 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 + || TREE_CODE (t2) == VIEW_CONVERT_EXPR) + { + t2 = TREE_OPERAND (t2, 0); + if (REFERENCE_REF_P (t2)) + 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 (t != t2) + 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..27c66ed5019 --- /dev/null +++ b/gcc/testsuite/c-c++-common/gomp/depobj-3.c @@ -0,0 +1,40 @@ +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 } */ +} + +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