Re: [PATCH 1/4] Add function for pretty-printing OpenACC clause names

[Thomas Schwinge <thomas@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 3776 bytes --]

Hi Julian!

On 2019-10-06T15:32:34-0700, Julian Brown <julian@codesourcery.com> wrote:
> This patch adds a function to pretty-print OpenACC clause names from
> OMP_CLAUSE_MAP_KINDs, for error output.

Indeed talking about (OpenMP) 'map' clauses in an OpenACC context is not
quite ideal -- that's what PR65095 is about, so please mention that one
in your ChangeLog updates.

> The function is used by subsequent
> patches.

Ah, I somehow had assumed you'd also adapt existing code to use it.  ;-)

> Previously approved as part of:
>
>   https://gcc.gnu.org/ml/gcc-patches/2018-12/msg01292.html
>
> FAOD, OK for trunk?

Still fine.  To record the review effort, please include "Reviewed-by:
Thomas Schwinge <thomas@codesourcery.com>" in the commit log, see
<https://gcc.gnu.org/wiki/Reviewed-by>.


A few more comments, for later:

>  gcc/c-family/c-common.h |  1 +
>  gcc/c-family/c-omp.c    | 33 +++++++++++++++++++++++++++++++++

As I'd mentioned before: 'Eventually (that is, later), this should move
into generic code, next to the other "clause printing".  Also to be
shared with Fortran.'

> --- a/gcc/c-family/c-omp.c
> +++ b/gcc/c-family/c-omp.c

> +/* For OpenACC, the OMP_CLAUSE_MAP_KIND of an OMP_CLAUSE_MAP is used internally
> +   to distinguish clauses as seen by the user.  Return the "friendly" clause
> +   name for error messages etc., where possible.  See also
> +   c/c-parser.c:c_parser_oacc_data_clause and
> +   cp/parser.c:cp_parser_oacc_data_clause.  */
> +
> +const char *
> +c_omp_map_clause_name (tree clause, bool oacc)
> +{
> +  if (oacc && OMP_CLAUSE_CODE (clause) == OMP_CLAUSE_MAP)
> +    switch (OMP_CLAUSE_MAP_KIND (clause))
> +    {
> +    case GOMP_MAP_FORCE_ALLOC:
> +    case GOMP_MAP_ALLOC: return "create";
> +    case GOMP_MAP_FORCE_TO:
> +    case GOMP_MAP_TO: return "copyin";
> +    case GOMP_MAP_FORCE_FROM:
> +    case GOMP_MAP_FROM: return "copyout";
> +    case GOMP_MAP_FORCE_TOFROM:
> +    case GOMP_MAP_TOFROM: return "copy";
> +    case GOMP_MAP_RELEASE: return "delete";
> +    case GOMP_MAP_FORCE_PRESENT: return "present";
> +    case GOMP_MAP_ATTACH: return "attach";
> +    case GOMP_MAP_FORCE_DETACH:
> +    case GOMP_MAP_DETACH: return "detach";
> +    case GOMP_MAP_DEVICE_RESIDENT: return "device_resident";
> +    case GOMP_MAP_LINK: return "link";
> +    case GOMP_MAP_FORCE_DEVICEPTR: return "deviceptr";
> +    default: break;
> +    }
> +  return omp_clause_code_name[OMP_CLAUSE_CODE (clause)];
> +}

Indeed nearby (after) the 'omp_clause_code_name' definition in
'gcc/tree.c' would probably be a better place for this, as that's where
the current clause names are coming from.

I did wonder whether we need to explicitly translate from (OpenMP) "'map'
clause" into (OpenACC) "'create' clause" etc., or if a generic (OpenACC)
"data clause" would be sufficient?  (After all, in diagnostics we also
print out the original code, so the user can then see which specific data
clause is being complained about.  But -- somewhat funnily! -- the way
you're doing this might actually be better in terms of translatability in
diagnostics printing: "%qs clause" might require a different translation
when its "%s" can be "'map'" (doesn't get translated) vs. "data" (gets
translated), but remains the same when "%s" is "'map'" vs. "'create'"
etc.

Do we at all still generate 'GOMP_MAP_FORCE_*' anywhere, or should these
in fact be 'gcc_unreachable'?

Generally, I prefer if all possible 'case's are listed explicitly, and
then the 'default' (and here OpenMP-only ones, too) be 'gcc_unreachable',
so that we easily catch the case that new 'GOMP_MAP_*' get added but such
functions not updated, for example.


Grüße
 Thomas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 658 bytes --]