On Wed, Jul 07, 2021 at 07:59:58PM +0200, Marcel Vollweiler wrote:
> OpenMP: Add support for device-modifiers for 'omp target device'
>
> gcc/c/ChangeLog:
>
> * c-parser.c (c_parser_omp_clause_device): Add support for
> device-modifiers for 'omp target device'.
>
> gcc/cp/ChangeLog:
>
> * parser.c (cp_parser_omp_clause_device): Add support for
> device-modifiers for 'omp target device'.
>
> gcc/fortran/ChangeLog:
>
> * openmp.c (gfc_match_omp_clauses): Add support for
> device-modifiers for 'omp target device'.
>
> gcc/testsuite/ChangeLog:
>
> * c-c++-common/gomp/target-device-1.c: New test.
> * c-c++-common/gomp/target-device-2.c: New test.
> * gfortran.dg/gomp/target-device-1.f90: New test.
> * gfortran.dg/gomp/target-device-2.f90: New test.
> static tree
> c_parser_omp_clause_device (c_parser *parser, tree list)
> {
> location_t clause_loc = c_parser_peek_token (parser)->location;
> + location_t expr_loc;
> + c_expr expr;
> + tree c, t;
> +
> matching_parens parens;
> - if (parens.require_open (parser))
> + if (!parens.require_open (parser))
> + return list;
> +
> + int pos = 1;
> + int pos_colon = 0;
> + while (c_parser_peek_nth_token_raw (parser, pos)->type == CPP_NAME
> + || c_parser_peek_nth_token_raw (parser, pos)->type == CPP_COLON
> + || c_parser_peek_nth_token_raw (parser, pos)->type == CPP_COMMA)
Why CPP_COMMA? The OpenMP 5.0/5.1/5.2 grammar only supports a single device
modifier.
So please simplify it to just an
if (c_parser_next_token_is (parser, CPP_NAME)
&& c_parser_peek_2nd_token (parser, 2)->type == CPP_COLON)
{
and check there just for the two modifiers.
const char *p
= IDENTIFIER_POINTER (c_parser_peek_token (parser)->value);
if (strcmp ("ancestor", p) == 0)
...
else if (strcmp ("device-num", p) == 0)
;
else
error_at (..., "expected %<ancestor%> or %<device-num%>");
}
Similarly for C++.
Also, even if we sorry on device(ancestor: ...), it would be nice if you
in tree.h define OMP_CLAUSE_DEVICE_ANCESTOR macro (with
(OMP_CLAUSE_SUBCODE_CHECK (NODE, OMP_CLAUSE_DEVICE)->base.public_flag)
definition), set it, sorry later on it (e.g. omp-expand.c) only if it
survived till then (wasn't removed because of other errors) and diagnose
the various restrictions/requirements on device(ancestor:).
In particular:
1) that OMP_CLAUSE_DEVICE clauses with OMP_CLAUSE_DEVICE_ANCESTOR
only appear on OMP_TARGET and not on other constructs
(this can be easily tested e.g. during gimplification, when
gimplify_scan_omp_clauses sees OMP_CLAUSE_DEVICE with
OMP_CLAUSE_DEVICE_ANCESTOR and code != OMP_TARGET, diagnose)
2) that if after the usual fully folding the argument is INTEGER_CST,
it is equal to 1 (the spec says must evaluate to 1, but doesn't say
it has to be a constant, so it can evaluate to 1 at runtime but if it is
a constant other than 1, we know it will not evaluate to 1); this can be
done in *finish_omp_clauses
3) that omp_requires_mask has OMP_REQUIRES_REVERSE_OFFLOAD set; this should
be checked during the parsing
4) only the device, firstprivate, private, defaultmap, and map clauses may
appear on the construct; can be also done during gimplification, there is
at most one device clause, so walking all clauses when we see
OMP_CLAUSE_DEVICE_ANCESTOR is still linear complexity
5) no OpenMP constructs or calls to OpenMP API runtime routines are allowed
inside
the corresponding target region (this is something that should be checked
in omp-low.c region nesting code, we already have similar restrictions
for e.g. the loop construct)
Everything should be covered by testcases.
Jakub
