Hi PA,
thanks for the updated patch!
Paul-Antoine Arras wrote:
OpenMP: C++ front-end support for dispatch + adjust_args
This patch adds C++ support for the `dispatch` construct and the `adjust_args`
clause. It relies on the c-family bits comprised in the corresponding C
front
end patch for pragmas and attributes.
...
I have not yet checked the 5/7 patch, which adds C/C++ testcases; hence,
the following might be already provided:
Namely, tests using the C23/C++11 '[[omp::]]' attributes. Either as full
testcase or as part of an existing file.
[Currently, GCC has c-c++-common/gomp/attrs-*.c (C/C++),
g++.dg/gomp/attrs-*.C (C++), gcc.dg/gomp/attrs-*.c (C) — and the specific
testcases g++.dg/gomp/declare-target-indirect-1.C and
gcc.dg/gomp/named-loops-1.c.]
...
gcc/cp/ChangeLog:
* decl.cc (omp_declare_variant_finalize_one): Set adjust_args
need_device_ptr attribute.
* parser.cc (cp_parser_direct_declarator): Update call to
cp_parser_late_return_type_opt.
(cp_parser_late_return_type_opt): Add parameter. Update call to
cp_parser_late_parsing_omp_declare_simd.
I wonder whether this should be made more explicit. Currently, one might
wonder why a new parameter to a generic C++ parser function is required.
But it is just the already existing 'params' argument, which is passed
along.
Maybe use for _opt: "Add 'tree parms' parameter. ..."
or in _one: "Update call ... to pass 'parms' as argument."
or something similar.
Either variant resolves the puzzle and the rest can be deduced (when reading
the ChangeLog) and/or is OpenMP specific.
* * *
+ /* Build the assignment expression. Its default
+ location:
+ LHS = RHS
+ ~~~~^~~~~
+ is the location of the '=' token as the
+ caret, ranging from the start of the lhs to the
+ end of the rhs. */
+ loc = make_location (loc, expr.get_start (), rhs.get_finish ());
+ expr = build_x_modify_expr (loc, expr, NOP_EXPR, rhs, NULL_TREE,
+ complain_flags (false));
+ /* TODO: build_x_modify_expr doesn't honor the location,
+ so we must set it here. */
+ expr.set_location (loc);
It is actually honored most of the time, e.g. viabuild_min_nt_loc or
build2_loc. However, for some template handling it doesn't.
Can you check whether the following works — and, if so, put it into
a separate patch and submit it to gcc-patches, CCing the two C++
maintainers, Jason and Nathan?
[That's outside of OpenMP, hence, it needs C++ maintainer approval.]
If you wait until the commit of this patch, you can then also
remove the TODO above from cp_parser_omp_dispatch_body in the
same patch.
-------------
--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -9934,7 +9934,8 @@ build_x_modify_expr (location_t loc, tree lhs, enum
tree_code modifycode,
if (overload != NULL_TREE)
- return (build_min_non_dep_op_overload
- (MODIFY_EXPR, rval, overload, orig_lhs, orig_rhs));
+ return cp_expr (build_min_non_dep_op_overload
+ (MODIFY_EXPR, rval, overload, orig_lhs, orig_rhs),
+ loc);
- return (build_min_non_dep
- (MODOP_EXPR, rval, orig_lhs, op, orig_rhs));
+ return cp_expr (build_min_non_dep
+ (MODOP_EXPR, rval, orig_lhs, op, orig_rhs), loc);
}
-------------
* * *
static tree
cp_finish_omp_declare_variant (cp_parser *parser, cp_token *pragma_tok,
- tree attrs)
+ tree attrs, tree parms)
{
matching_parens parens;
if (!parens.require_open (parser))
@@ -49179,44 +49392,195 @@ cp_finish_omp_declare_variant (cp_parser *parser,
cp_token *pragma_tok,
location_t finish_loc = get_finish (varid.get_location ());
location_t varid_loc = make_location (caret_loc, start_loc, finish_loc);
- if (cp_lexer_next_token_is (parser->lexer, CPP_COMMA)
- && cp_lexer_nth_token_is (parser->lexer, 2, CPP_NAME))
- cp_lexer_consume_token (parser->lexer);
+ vec<tree> adjust_args_list = vNULL;
+ bool has_match = false, has_adjust_args = false;
+ location_t adjust_args_loc = UNKNOWN_LOCATION;
+ tree need_device_ptr_list = make_node (TREE_LIST);
...
+ if (strcmp (clause, "match") == 0)
+ ccode = match;
+ else if (strcmp (clause, "adjust_args") == 0)
+ {
...
I think you need a check_no_duplicate_clause style check here
- applying both to the C and the C++ parser. Namely:
The following is now accepted with 'gcc' and with 'g++'
(albeit g++ already accepted it before):
----------------
void f();
#pragma omp declare variant(f) match(construct={target})
match(construct={target})
void g();
----------------
But OpenMP (here version 6.0):
The 'match' clause has "Properties: unique, required"
By contrast, adjust_args has "Properties: default",
which is 'optional, repeatable, compatible (i.e. may appear with other clauses),
and free (i.e. can appear in any order).
(This is handled correctly.)
* * *
I also noticed that there is a location issue. Both 'gcc' and g++' point
to the wrong variable (i.e. 'zzz' and not 'xxx' is underlined):
foo.c:2:123: error: ‘xxx’ is specified more than once
2 | #pragma omp declare variant(f) match(construct={dispatch})
adjust_args(need_device_ptr: xxx) adjust_args(need_device_ptr: zzz, xxx)
|
^~~
Testcase:
-----------
void f(int *, int *, int *); #pragma omp declare variant(f)
match(construct={dispatch}) adjust_args(need_device_ptr: xxx)
adjust_args(need_device_ptr: zzz, xxx) void g(int *xxx, int *yyy, int *zzz);
----------
I think you want to use 'adjust_op_tok->loc' instead, i.e.
underlining 'adjust_args' or even better: 'adjust_args(...)',
i.e. until the closing ')' as the variable location itself
does not seem to be available.
On the other hand, storing the location does not really cost
anything. Maybe we should just use the following in C++
(and likewise in C)?
---------------
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -38834,4 +38834,4 @@ cp_parser_omp_var_list_no_open (cp_parser *parser, enum
omp_clause_code kind,
}
- else
- list = tree_cons (decl, NULL_TREE, list);
+ else /* kind == OMP_CLAUSE_ERROR */
+ list = tree_cons (decl, token->location, list);
---------------
and then 'TREE_VALUE (c)' instead of 'arg_loc' in cp_finish_omp_declare_variant.
* * *
I am pondering whether the following message is sensible or not:
foo.c:2:31: error: expected ‘match’ or ‘adjust_args’ before end of line
2 | #pragma omp declare variant(f)
| ^
Because strictly speaking only 'match' is required and neither
'append_args' nor 'adjust_args'.
Thus, maybe: 'expected 'match' clause [!] before ...',
i.e. only 'match' and adding 'clause'?
[Again applies to C and to C++.]
* * *
This error message provoked the following testcase:
-----------
void f(int *, int *, int *); #pragma omp declare variant(f)
adjust_args(need_device_ptr: xxx) void g(int *xxx, int *yyy, int *zzz);
------------
Result with 'g++':
foo.c:2:32: error: an ‘adjust_args’ clause can only be specified if the
‘dispatch’ selector of the construct selector set appears in the ‘match’ clause
2 | #pragma omp declare variant(f) adjust_args(need_device_ptr: xxx)
| ^~~~~~~~~~~
foo.c:2:64: internal compiler error: Segmentation fault
-> Can you fix the ICE?
With 'gcc', the result is okayish – as there is no segfault.
However, shouldn't it just fail (see above) with an error that
required 'match' clause isn't available at all?
* * *
Otherwise, LGTM.
Thanks,
Tobias
