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
