Hi,
some more comments:
Paul-Antoine Arras wrote:
Here is an updated patch following these comments.
gcc/testsuite/ChangeLog:
* gcc.dg/gomp/adjust-args-1.c: New test.
* gcc.dg/gomp/dispatch-1.c: New test.
The ChangeLog misses to include libgomp/testsuite/libgomp.c/dispatch-1.c
and libgomp/testsuite/libgomp.c/dispatch-2.c, which are part of this patch.
But there is reason to move them to 5/7: I think we also need a run test
for C++ to make sure that it works, i.e. moving them to
libgomp.c-c++-common/ makes sense, which in turn requires the 4/7 C++ FE
patch.
* * *
+/* Parse a function dispatch structured block:
+
+ lvalue-expression = target-call ( [expression-list] );
+ or
+ target-call ( [expression-list] );
+
+ Adapted from c_parser_expr_no_commas.
+*/
Can you expand the description, e.g. like:
Adapted from c_parser_expr_no_commas and
c_parser_postfix_expression (CPP_NAME/C_ID_ID) for the
function name).
And:
+static tree
+c_parser_omp_dispatch_body (c_parser *parser)
+{
...
+ /* Parse function name. */
+ if (!c_parser_next_token_is (parser, CPP_NAME))
+ {
+ c_parser_error (parser, "expected a function name");
+ rhs.set_error ();
+ return rhs.value;
+ }
+ expr_loc = c_parser_peek_token (parser)->location;
+ tree id = c_parser_peek_token (parser)->value;
+ c_parser_consume_token (parser);
+ if (!c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>"))
+ return error_mark_node;
+
+ rhs.value = build_external_ref (expr_loc, id, true, &rhs.original_type);
+ set_c_expr_source_range (&rhs, tok_range);
+ /* Parse argument list. */
Wouldn't it be way easier and future proof (less code duplication)
to just do the following:
Note:
* rhs.set_error() / return rhs.value; → error_mark_node;
* 'c_parser_require (parser, CPP_OPEN_PAREN' → c_parser_next_token_is
* And then checking that the returned code is the expected CALL.
/* Parse function name. */
if (!c_parser_next_token_is (parser, CPP_NAME))
{
c_parser_error (parser, "expected a function name");
return error_mark_node;
}
expr_loc = c_parser_peek_token (parser)->location;
tree id = c_parser_peek_token (parser)->value;
c_parser_consume_token (parser);
if (!c_parser_next_token_is (parser, CPP_OPEN_PAREN))
{
c_parser_error (parser, "expected a function name");
return error_mark_node;
}
rhs.value = build_external_ref (expr_loc, id, true, &rhs.original_type);
set_c_expr_source_range (&rhs, tok_range);
/* Parse argument list. */
rhs = c_parser_postfix_expression_after_primary
(parser, EXPR_LOC_OR_LOC (rhs.value, expr_loc), rhs);
if (TREE_CODE (lhs.value) != CALL_EXPR)
{
error_at (EXPR_LOC_OR_LOC (rhs.value, expr_loc),
"expected target-function call");
return error_mark_node;
}
* * *
Testing it shows then the following error:
dispatch3.c:11:13: Fehler: expected target-function call
11 | x = bar()[0];
| ~~~~~~~~~^~~
which seems to be reasonable.
* * *
#define OMP_DECLARE_SIMD_CLAUSE_MASK \
@@ -25242,77 +25512,223 @@ c_finish_omp_declare_variant (c_parser *parser, tree
fndecl, tree parms)
[...]
The old code did:
tree ctx = c_parser_omp_context_selector_specification (parser, parms);
if (ctx == error_mark_node) goto fail; ctx = omp_check_context_selector
(match_loc, ctx); if (ctx != error_mark_node && variant !=
error_mark_node) { if (TREE_CODE (variant) != FUNCTION_DECL)
i.e. it effectively finished parsing (except for the ')', then checks
the context selector and, finally, the variant declaration. The new code
is similar, except that the variant declaration check is inside the
'match(…)' check – and missing this when processing 'adjust_args'.
Either all parsing needs to happen before this - or the handling needs
to be be but next to the relative item. NOTE: You need to ensure that
the location is still okay if you move the checking after parsing. * * *
As mentioned in the older 2/7 review, this will fail (ICE) if the
variant has issues (e.g. variant function not declared) as the
adjust-args code assumes that it is valid. → testcase in that email (+
some off list communication) * * * As mentioned off list, the following fails as well:
void variant_fn(); // Assume C < C23; in C++/C23 it becomes the same as
'…(void)'.
#pragma omp declare variant(variant_fn) match(construct={dispatch})
adjust_args(need_device_ptr: x,y)
void bar(int *x, int *y);
void sub(int *x, *y)
{
#pragma omp dispatch is_device_ptr(x)
bar(x, y);
}
Issue: the host-to-device pointer conversion is lacking for 'variant_fn'.
* * *
Side note: I somehow had expected that there is a warning when
mixing incompatible calls in the same translation unit, but all
tried compiler accept it silently even with some warnings turned
on. (Ignoring the warnings that C23 will disallow it.)
void f(); // Assume C < C23.
void g()
{
f (5); // Call with one integer argument
}
void sub(int *x, int *y)
{
f (x,y); // Call with two pointer arguments
}
Thus, bluntly applying the pointer conversion for the case above
seems to be sufficient without doing anything else.
* * *
+ do
{
- c_parser_error (parser, "expected %<match%>");
[cf. above]
+ if (strcmp (p, "need_device_ptr") == 0
+ && TREE_CODE (TREE_TYPE (arg)) != POINTER_TYPE)
+ {
+ error_at (loc, "%qD is not a C pointer", decl);
+ goto fail;
+ }
The wording is odd (or sounds like the wording used for Fortran, where a
'type(C_ptr)' is required). How about 'not a pointer' or 'not of pointer
type'?
Current OpenMP 6.0 draft uses for C: "If the need_device_ptr adjust-op
modifier is present, each list item that appears in the clause that
refers to a specific named argument in the declaration of the function
variant must be of pointer type."
(For C++: + reference type and for Fortran: C_PTR type.)
* * *
+ parens.require_close (parser);
+ } while (c_parser_next_token_is_not (parser, CPP_PRAGMA_EOL));
+ c_parser_skip_to_pragma_eol (parser);
+
+ if (has_adjust_args)
+ {
+ if (!has_match)
+ {
+ error_at (
+ adjust_args_loc,
+ "an %<adjust_args%> clause can only be specified if the "
+ "%<dispatch%> selector of the construct selector set appears "
+ "in the %<match%> clause");
I wonder whether it should also be %<construct%> as it is also a keyword.
* * *
The following testcase gives an ICE due to the type conversion:
long long *f();
int variant_fn();
#pragma omp declare variant(variant_fn) match(construct={dispatch})
int bar();
void sub()
{
#pragma omp dispatch
*f() = bar();
}
* * *
Tobias
