Next set of OpenACC changes: C family

[Jakub Jelinek]

On Tue, May 05, 2015 at 10:57:28AM +0200, Thomas Schwinge wrote:
> --- gcc/c-family/c-common.c
> +++ gcc/c-family/c-common.c
> @@ -809,7 +809,7 @@ const struct attribute_spec c_common_attribute_table[] =
>  			      handle_omp_declare_simd_attribute, false },
>    { "cilk simd function",     0, -1, true,  false, false,
>  			      handle_omp_declare_simd_attribute, false },
> -  { "omp declare target",     0, 0, true, false, false,
> +  { "omp declare target",     0, -1, true, false, false,
>  			      handle_omp_declare_target_attribute, false },
>    { "alloc_align",	      1, 1, false, true, true,
>  			      handle_alloc_align_attribute, false },

Can you explain this change?  "omp declare target" doesn't take any
arguments, so "0, 0," looks right to me.

> @@ -823,6 +823,7 @@ const struct attribute_spec c_common_attribute_table[] =
>  			      handle_bnd_legacy, false },
>    { "bnd_instrument",         0, 0, true, false, false,
>  			      handle_bnd_instrument, false },
> +  { "oacc declare",           0, -1, true,  false, false, NULL, false },
>    { NULL,                     0, 0, false, false, false, NULL, false }

If "oacc declare" is different, then supposedly you shouldn't reuse
"omp declare target" attribute for the OpenACC thingie.

> --- gcc/c-family/c-omp.c
> +++ gcc/c-family/c-omp.c
> @@ -1087,3 +1087,108 @@ c_omp_predetermined_sharing (tree decl)
>  
>    return OMP_CLAUSE_DEFAULT_UNSPECIFIED;
>  }
> +
> +/* Return a numerical code representing the device_type.  Currently,
> +   only device_type(nvidia) is supported.  All device_type parameters
> +   are treated as case-insensitive keywords.  */
> +
> +int
> +oacc_extract_device_id (const char *device)
> +{
> +  if (!strcasecmp (device, "nvidia"))
> +    return GOMP_DEVICE_NVIDIA_PTX;
> +  return GOMP_DEVICE_NONE;
> +}

Why do you support just one particular device_type?  That sounds broken.
You should just have some table with names <-> GOMP_DEVICE_* mappings.

> +	  if (code & (1 << GOMP_DEVICE_NVIDIA_PTX))
> +	    {
> +	      if (seen_nvidia)
> +		{
> +		  seen_nvidia = NULL_TREE;
> +		  error_at (OMP_CLAUSE_LOCATION (c),
> +			    "duplicate device_type (nvidia)");
> +		  goto filter_error;
> +		}
> +	      else
> +		seen_nvidia = OMP_CLAUSE_DEVICE_TYPE_CLAUSES (c);

Again, I must say I don't like the hardcoding of one particular
device type here.
Doesn't Intel want to support OpenACC for XeonPhi?  What about HSA
eventually, etc.?

> @@ -4624,7 +4657,7 @@ c_parser_compound_statement_nostart (c_parser *parser)
>  	  last_label = false;
>  	  mark_valid_location_for_stdc_pragma (false);
>  	  c_parser_declaration_or_fndef (parser, true, true, true, true,
> -					 true, NULL, vNULL);
> +					 true, NULL, vNULL, NULL_TREE, false);

Wouldn't default arguments be in order here?  Though, even those will mean
compile time cost of passing all the zeros almost all the time.

> -/* OpenMP 2.5:
> +/* OpenACC:
> +   num_gangs ( expression )
> +   num_workers ( expression )
> +   vector_length ( expression )
> +
> +   OpenMP 2.5:
>     num_threads ( expression ) */
>  
>  static tree
> -c_parser_omp_clause_num_threads (c_parser *parser, tree list)
> +c_parser_omp_positive_int_clause (c_parser *parser, pragma_omp_clause c_kind,
> +				  const char *str, tree list)
>  {
> -  location_t num_threads_loc = c_parser_peek_token (parser)->location;
> -  if (c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>"))
> +  omp_clause_code kind;
> +  switch (c_kind)

This is undesirable, to add new clauses to the same handler you'd need
to add them both in the caller and to this switch.  Perhaps pass
omp_clause_code kind argument instead of pragma_omp_clause c_kind?

>  static tree
> -c_parser_omp_clause_num_workers (c_parser *parser, tree list)
> +c_parser_oacc_shape_clause (c_parser *parser, pragma_omp_clause c_kind,
> +			    const char *str, tree list)
>  {
> -  location_t num_workers_loc = c_parser_peek_token (parser)->location;
> -  if (c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>"))
> +  omp_clause_code kind;
> +  const char *id = "num";
> +
> +  switch (c_kind)

Likewise.

> +/* Split the 'clauses' into a set of 'loop' clauses and a set of
> +   'not-loop' clauses.  */
>  
>  static tree
> -c_parser_oacc_kernels (location_t loc, c_parser *parser, char *p_name)
> +oacc_split_loop_clauses (tree clauses, tree *not_loop_clauses)

Is this really C specific?  I mean, for OpenMP I'm sharing the clause
splitting code between C and C++ FEs in c-omp.c.

	Jakub