Hi Cesar!

On Mon, 15 Sep 2014 16:35:02 -0700, Cesar Philippidis <ce...@codesourcery.com> 
wrote:
> This patch adds initial support for the OpenACC routine directive.

Thanks!

> It's
> not complete just yet because it doesn't implement any of the optional
> clauses, except for the optional function/subroutine name. As such, it
> doesn't go beyond marking functions with the "omp declare target" attribute.

Reviewing the C front end bits.

I wonder if we can integrate this better (maybe as a follow-up patch),
for example, inside an OpenMP »#pragma omp declare target« region,
current_omp_declare_target_attribute is set to non-zero, and can then
conveniently be used in gcc/c/c-decl.c:c_decl_attributes.  (Of course,
OpenACC's »#pragma acc routine« is a bit different; doesn't have a »end«
tag.)  I have not yet figured out whether we can do something similar,
for example to avoid all the c_parser_declaration_or_fndef call-sites
changes.

> --- a/gcc/c/c-parser.c
> +++ b/gcc/c/c-parser.c
> @@ -1678,6 +1687,9 @@ c_parser_declaration_or_fndef (c_parser *parser, bool 
> fndef_ok,
|         if (omp_declare_simd_clauses.exists ()
>             || !vec_safe_is_empty (parser->cilk_simd_fn_tokens))
>           c_finish_omp_declare_simd (parser, NULL_TREE, NULL_TREE,
>                                      omp_declare_simd_clauses);
> +       else
> +         c_finish_oacc_routine (parser, NULL_TREE,
> +                                   oacc_routine_clauses, oacc_routine_named);
>         c_parser_skip_to_end_of_block_or_statement (parser);
>         return;
>       }

Please be more explicit.  Using such "catch-all else statements", the
next reader of this code will wonder: why is
»!(omp_declare_simd_clauses.exists () || !vec_safe_is_empty
(parser->cilk_simd_fn_tokens))« the condition for calling
c_finish_oacc_routine?  (Likewise in other places.)

> +#define OACC_ROUTINE_CLAUSE_MASK                                     \
> +     (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_PROC_BIND)

PRAGMA_OMP_CLAUSE_NONE (for the moment), not PRAGMA_OMP_CLAUSE_PROC_BIND.

We're missing test cases for the different syntax options that are
described in the OpenACC specification, especially the invalid ones.  For
example, should gcc/testsuite/c-c++-common/goacc/pragma_context.c be
extended?


Grüße,
 Thomas

Attachment: pgpg3Lfp4y1ep.pgp
Description: PGP signature

Reply via email to