Re: [PATCH] OpenACC routines -- c++ front end

[Cesar Philippidis]

On 11/11/2016 03:43 PM, Cesar Philippidis wrote:
> Like it's c FE counterpart, this contains the following changes:
> 
>  * Updates c_parser_oacc_shape_clause to accept a location_t
>argument in order to make the diagnostics more precise.
> 
>  * Adds support for the bind and nohost clauses.
> 
>  * Adds more diagnostics for invalid acc routines.
> 
> Is this patch OK for trunk?

Here is the updated version of the c++ OpenACC routine patch. It's
mostly the same as before, but now cp_parser_oacc_shape_clause no has a
dummy cp_parser argument like its c FE counterpart.

Is this patch ok for trunk?

Cesar

2016-11-22  Cesar Philippidis  
	Thomas Schwinge  

	gcc/cp/
	* cp-tree.h (bind_decls_match): Declare.
	* decl.c (bind_decls_match): New function.
	* parser.c (cp_parser_omp_clause_name): 
	(cp_parser_oacc_simple_clause): Remove unused cp_parser argument.
	(cp_parser_oacc_shape_clause): New location_t loc argument.  Use it
	to report more accurate diagnostics.  Remove parser argument.
	(cp_parser_oacc_clause_bind): New function.
	(cp_parser_oacc_all_clauses): Handle OpenACC bind and nohost clauses.
	Update calls to c_parser_oacc_{simple,shape}_clause.
	(OACC_ROUTINE_CLAUSE_MASK): Add PRAGMA_OACC_CLAUSE_{BIND,NOHOST}.
	(cp_parser_oacc_routine): Update diagnostics.
	(cp_parser_late_parsing_oacc_routine): Likewise.
	(cp_finalize_oacc_routine): Likewise.
	* semantics.c (finish_omp_clauses): Handle OMP_CLAUSE_{BIND,NOHOST}.


diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 5674886..c9dbc4f 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -5785,6 +5785,7 @@ extern void finish_scope			(void);
 extern void push_switch(tree);
 extern void pop_switch(void);
 extern tree make_lambda_name			(void);
+extern int bind_decls_match			(tree, tree);
 extern int decls_match(tree, tree);
 extern tree duplicate_decls			(tree, tree, bool);
 extern tree declare_local_label			(tree);
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 6893eae..09f9ffc 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -1198,6 +1198,138 @@ decls_match (tree newdecl, tree olddecl)
   return types_match;
 }
 
+/* Similiar to decls_match, but only applies to FUNCTION_DECLS.  Functions
+   in separate namespaces may match.
+*/
+
+int
+bind_decls_match (tree newdecl, tree olddecl)
+{
+  int types_match;
+
+  if (newdecl == olddecl)
+return 1;
+
+  if (TREE_CODE (newdecl) != TREE_CODE (olddecl))
+/* If the two DECLs are not even the same kind of thing, we're not
+   interested in their types.  */
+return 0;
+
+  gcc_assert (DECL_P (newdecl));
+  gcc_assert (TREE_CODE (newdecl) == FUNCTION_DECL);
+
+  tree f1 = TREE_TYPE (newdecl);
+  tree f2 = TREE_TYPE (olddecl);
+  tree p1 = TYPE_ARG_TYPES (f1);
+  tree p2 = TYPE_ARG_TYPES (f2);
+  tree r2;
+
+  /* Specializations of different templates are different functions
+ even if they have the same type.  */
+  tree t1 = (DECL_USE_TEMPLATE (newdecl)
+	 ? DECL_TI_TEMPLATE (newdecl)
+	 : NULL_TREE);
+  tree t2 = (DECL_USE_TEMPLATE (olddecl)
+	 ? DECL_TI_TEMPLATE (olddecl)
+	 : NULL_TREE);
+  if (t1 != t2)
+return 0;
+
+  if (CP_DECL_CONTEXT (newdecl) != CP_DECL_CONTEXT (olddecl)
+  && TREE_CODE (CP_DECL_CONTEXT (newdecl)) != NAMESPACE_DECL
+  && TREE_CODE (CP_DECL_CONTEXT (olddecl)) != NAMESPACE_DECL
+  && ! (DECL_EXTERN_C_P (newdecl)
+	&& DECL_EXTERN_C_P (olddecl)))
+return 0;
+
+  /* A new declaration doesn't match a built-in one unless it
+ is also extern "C".  */
+  if (DECL_IS_BUILTIN (olddecl)
+  && DECL_EXTERN_C_P (olddecl) && !DECL_EXTERN_C_P (newdecl))
+return 0;
+
+  if (TREE_CODE (f1) != TREE_CODE (f2))
+return 0;
+
+  /* A declaration with deduced return type should use its pre-deduction
+ type for declaration matching.  */
+  r2 = fndecl_declared_return_type (olddecl);
+
+  if (same_type_p (TREE_TYPE (f1), r2))
+{
+  if (!prototype_p (f2) && DECL_EXTERN_C_P (olddecl)
+	  && (DECL_BUILT_IN (olddecl)
+#ifndef NO_IMPLICIT_EXTERN_C
+	  || (DECL_IN_SYSTEM_HEADER (newdecl) && !DECL_CLASS_SCOPE_P (newdecl))
+	  || (DECL_IN_SYSTEM_HEADER (olddecl) && !DECL_CLASS_SCOPE_P (olddecl))
+#endif
+	  ))
+	{
+	  types_match = self_promoting_args_p (p1);
+	  if (p1 == void_list_node)
+	TREE_TYPE (newdecl) = TREE_TYPE (olddecl);
+	}
+#ifndef NO_IMPLICIT_EXTERN_C
+  else if (!prototype_p (f1)
+	   && (DECL_EXTERN_C_P (olddecl)
+		   && DECL_IN_SYSTEM_HEADER (olddecl)
+		   && !DECL_CLASS_SCOPE_P (olddecl))
+	   && (DECL_EXTERN_C_P (newdecl)
+		   && DECL_IN_SYSTEM_HEADER (newdecl)
+		   && !DECL_CLASS_SCOPE_P (newdecl)))
+	{
+	  types_match = self_promoting_args_p (p2);
+	  TREE_TYPE (newdecl) = TREE_TYPE (olddecl);
+	}
+#endif
+  else
+	types_match =
+	  compparms (p1, p2)
+	  && type_memfn_rqual (f1) == type_memfn_rqual (f2)
+	  && (TYPE_ATTRIBUTES (TREE_TYPE (newdecl)) == NULL_TREE
+	  || comp_type_attributes 

Re: [PATCH] OpenACC routines -- c front end

[Cesar Philippidis]

On 11/18/2016 04:21 AM, Jakub Jelinek wrote:
> On Fri, Nov 11, 2016 at 03:43:23PM -0800, Cesar Philippidis wrote:
>> @@ -11801,12 +11807,11 @@ c_parser_oacc_shape_clause (c_parser *parser, 
>> omp_clause_code kind,
>>  }
>>  
>>location_t expr_loc = c_parser_peek_token (parser)->location;
>> -  c_expr cexpr = c_parser_expr_no_commas (parser, NULL);
>> -  cexpr = convert_lvalue_to_rvalue (expr_loc, cexpr, false, true);
>> -  tree expr = cexpr.value;
>> +  tree expr = c_parser_expr_no_commas (parser, NULL).value;
>>if (expr == error_mark_node)
>>  goto cleanup_error;
>>  
>> +  mark_exp_read (expr);
>>expr = c_fully_fold (expr, false, NULL);
>>  
>>/* Attempt to statically determine when the number isn't a
> 
> Why?  Are the arguments of the clauses lvalues?

The spec is unclear if those args must be constants or not. The only
time it explicitly mentions constant int-expr is for the tile clause,
which was added late. Gang, worker and vector were added early in the
1.0 spec, where things were defined somewhat loosely.

>> @@ -11867,12 +11872,12 @@ c_parser_oacc_shape_clause (c_parser *parser, 
>> omp_clause_code kind,
>> seq */
>>  
>>  static tree
>> -c_parser_oacc_simple_clause (c_parser *parser, enum omp_clause_code code,
>> - tree list)
>> +c_parser_oacc_simple_clause (c_parser * /* parser */, location_t loc,
> 
> Just leave it as c_parser *, or better yet remove the argument if you don't
> need it.

I removed that argument.

>> +  else
>> +{
>> +  //TODO? TREE_USED (decl) = 1;
> 
> This would be /* FIXME: TREE_USED (decl) = 1;  */
> but wouldn't it be better to figure out if you want to do that or not?

Thomas has more state on that, but it seems unneeded. The c++ FE doesn't
do that either, so I removed that comment.

Is this patch ok for trunk?

Cesar

2016-11-22  Cesar Philippidis  
	Thomas Schwinge  

	gcc/c/
	* c-parser.c (c_parser_omp_clause_name): Handle OpenACC bind and
	nohost.
	(c_parser_oacc_shape_clause): New location_t loc argument.  Use it
	to report more accurate diagnostics.
	(c_parser_oacc_simple_clause): Likewise.
	(c_parser_oacc_clause_bind): New function.
	(c_parser_oacc_all_clauses): Handle OpenACC bind and nohost clauses.
	Update calls to c_parser_oacc_{simple,shape}_clause.
	(OACC_ROUTINE_CLAUSE_MASK): Add PRAGMA_OACC_CLAUSE_{BIND,NOHOST}.
	(c_parser_oacc_routine): Update diagnostics.
	(c_finish_oacc_routine): Likewise.
	* c-typeck.c (c_finish_omp_clauses): Handle OMP_CLAUSE_{BIND,NOHOST}.


diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 00fe731..fd87b54 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -10408,6 +10408,10 @@ c_parser_omp_clause_name (c_parser *parser)
 	  else if (!strcmp ("async", p))
 	result = PRAGMA_OACC_CLAUSE_ASYNC;
 	  break;
+	case 'b':
+	  if (!strcmp ("bind", p))
+	result = PRAGMA_OACC_CLAUSE_BIND;
+	  break;
 	case 'c':
 	  if (!strcmp ("collapse", p))
 	result = PRAGMA_OMP_CLAUSE_COLLAPSE;
@@ -10489,6 +10493,8 @@ c_parser_omp_clause_name (c_parser *parser)
 	result = PRAGMA_OMP_CLAUSE_NOTINBRANCH;
 	  else if (!strcmp ("nowait", p))
 	result = PRAGMA_OMP_CLAUSE_NOWAIT;
+	  else if (!strcmp ("nohost", p))
+	result = PRAGMA_OACC_CLAUSE_NOHOST;
 	  else if (!strcmp ("num_gangs", p))
 	result = PRAGMA_OACC_CLAUSE_NUM_GANGS;
 	  else if (!strcmp ("num_tasks", p))
@@ -11676,12 +11682,12 @@ c_parser_omp_clause_num_workers (c_parser *parser, tree list)
 */
 
 static tree
-c_parser_oacc_shape_clause (c_parser *parser, omp_clause_code kind,
+c_parser_oacc_shape_clause (c_parser *parser, location_t loc,
+			omp_clause_code kind,
 			const char *str, tree list)
 {
   const char *id = "num";
   tree ops[2] = { NULL_TREE, NULL_TREE }, c;
-  location_t loc = c_parser_peek_token (parser)->location;
 
   if (kind == OMP_CLAUSE_VECTOR)
 id = "length";
@@ -11746,12 +11752,11 @@ c_parser_oacc_shape_clause (c_parser *parser, omp_clause_code kind,
 	}
 
 	  location_t expr_loc = c_parser_peek_token (parser)->location;
-	  c_expr cexpr = c_parser_expr_no_commas (parser, NULL);
-	  cexpr = convert_lvalue_to_rvalue (expr_loc, cexpr, false, true);
-	  tree expr = cexpr.value;
+	  tree expr = c_parser_expr_no_commas (parser, NULL).value;
 	  if (expr == error_mark_node)
 	goto cleanup_error;
 
+	  mark_exp_read (expr);
 	  expr = c_fully_fold (expr, false, NULL);
 
 	  /* Attempt to statically determine when the number isn't a
@@ -11812,12 +11817,12 @@ c_parser_oacc_shape_clause (c_parser *parser, omp_clause_code kind,
seq */
 
 static tree
-c_parser_oacc_simple_clause (c_parser *parser, enum omp_clause_code code,
+c_parser_oacc_simple_clause (location_t loc, enum omp_clause_code code,
 			 tree list)
 {
   check_no_duplicate_clause (list, code, omp_clause_code_name[code]);
 
-  tree c = build_omp_clause (c_parser_peek_token (parser)->location, code);
+ 

Re: [PATCH] OpenACC routines -- c front end

[Jakub Jelinek]

On Fri, Nov 11, 2016 at 03:43:23PM -0800, Cesar Philippidis wrote:
> @@ -11801,12 +11807,11 @@ c_parser_oacc_shape_clause (c_parser *parser, 
> omp_clause_code kind,
>   }
>  
> location_t expr_loc = c_parser_peek_token (parser)->location;
> -   c_expr cexpr = c_parser_expr_no_commas (parser, NULL);
> -   cexpr = convert_lvalue_to_rvalue (expr_loc, cexpr, false, true);
> -   tree expr = cexpr.value;
> +   tree expr = c_parser_expr_no_commas (parser, NULL).value;
> if (expr == error_mark_node)
>   goto cleanup_error;
>  
> +   mark_exp_read (expr);
> expr = c_fully_fold (expr, false, NULL);
>  
> /* Attempt to statically determine when the number isn't a

Why?  Are the arguments of the clauses lvalues?

> @@ -11867,12 +11872,12 @@ c_parser_oacc_shape_clause (c_parser *parser, 
> omp_clause_code kind,
> seq */
>  
>  static tree
> -c_parser_oacc_simple_clause (c_parser *parser, enum omp_clause_code code,
> -  tree list)
> +c_parser_oacc_simple_clause (c_parser * /* parser */, location_t loc,

Just leave it as c_parser *, or better yet remove the argument if you don't
need it.

> +  else
> + {
> +   //TODO? TREE_USED (decl) = 1;

This would be /* FIXME: TREE_USED (decl) = 1;  */
but wouldn't it be better to figure out if you want to do that or not?

Jakub

Re: [PATCH] OpenACC routines -- c++ front end

[Chung-Lin Tang]

On 2016/11/12 07:43 AM, Cesar Philippidis wrote:
> Like it's c FE counterpart, this contains the following changes:
> 
>  * Updates c_parser_oacc_shape_clause to accept a location_t
>argument in order to make the diagnostics more precise.
> 
>  * Adds support for the bind and nohost clauses.
> 
>  * Adds more diagnostics for invalid acc routines.
> 
> Is this patch OK for trunk?
> 
> Cesar
> 

ENOPATCH

Re: [PATCH] OpenACC for C front end

[Jakub Jelinek]

On Thu, Nov 20, 2014 at 05:50:33PM -0600, James Norris wrote:
 +   case 'h':
 + if (!strcmp (host, p))
 +   result = PRAGMA_OMP_CLAUSE_SELF;
 + break;
 Shouldn't this be PRAGMA_OMP_CLAUSE_HOST (PRAGMA_OACC_CLAUSE_HOST)
 instead?  It is _HOST in the C++ patch, are there no C tests with
 that clause covering it?
 
 The host clause is a synonym for the self clause. The initial
 C++ patch did not treat host as a synonym and has amended
 accordingly.

Can you add a comment mentioning that (for casual readers)?

 There was a mistake in naming the function:
 c_parser_omp_clause_vector_length.
 Once it was renamed to: c_parser_oacc_clause_vector_length, diff was able to
 keep track.

Great.

 OK to commit after middle end is accepted?

Ok, thanks.

Jakub

Re: [PATCH] OpenACC for C++ front end

[Jakub Jelinek]

On Thu, Nov 20, 2014 at 05:33:57PM -0600, James Norris wrote:
 + t = OMP_CLAUSE_ASYNC_EXPR (c);
 + if (t == error_mark_node)
 +   remove = true;
 + else if (!type_dependent_expression_p (t)
 +   !INTEGRAL_TYPE_P (TREE_TYPE (t)))
 +   {
 + error (%async% expression must be integral);
 You have OMP_CLAUSE_LOCATION (c) which you could use for error_at.
 
 I followed the convention that was used elsewhere in the function
 at using error ().

Perhaps it would be better to change even those other spots in the function.
But that can be certainly done as a follow-up patch.

 Thank you for taking the time to review!
 
 OK to commit after middle end has been accepted?

Yes, thanks.

Jakub

Re: [PATCH] OpenACC for C++ front end

[James Norris]

Hi!

On 11/13/2014 07:02 AM, Jakub Jelinek wrote:

On Wed, Nov 05, 2014 at 03:37:08PM -0600, James Norris wrote:

2014-11-05  James Norris  jnor...@codesourcery.com
Cesar Philippidis  ce...@codesourcery.com
Thomas Schwinge  tho...@codesourcery.com
Ilmir Usmanov  i.usma...@samsung.com

...

Please check formatting.  I see various spots with 8 spaces instead of tabs,
e.g. on
+  check_no_duplicate_clause (list, OMP_CLAUSE_VECTOR_LENGTH,
+vector_length, location);
even the alignment is wrong, I see calls without space before (:
+  if (args == NULL || args-length() == 0)
...
+ error(%wait% expression must be integral);
other spots where the alignment isn't right:


Fixed.


+static tree
+cp_parser_oacc_cache (cp_parser *parser,
+   cp_token *pragma_tok __attribute__((unused)))
(cp_token should be below cp_parser).  While at this,
__attribute__((unused)) should be replaced by ATTRIBUTE_UNUSED, or removing
the parameter name, or removing the parameter altogether even better.


Fixed by removing unused parameter.


For the formatting issues, you can easily look for them in the patch
(in lines starting with +), change the patch and apply interdiff to your
tree.


Thank you for the pointer to interdiff!




--- a/gcc/c-family/c-omp.c
+++ b/gcc/c-family/c-omp.c
@@ -29,8 +29,47 @@ along with GCC; see the file COPYING3.  If not see
  #include c-pragma.h
  #include gimple-expr.h
  #include langhooks.h
+#include omp-low.h

As Thomas? has said, you should include gomp-constants.h and use them in:


+t = build_int_cst (integer_type_node, -2);  /* TODO: XXX FIX -2.  */

spots like this.


Fixed.




--- a/gcc/c-family/c-pragma.c
+++ b/gcc/c-family/c-pragma.c
@@ -1180,6 +1180,16 @@ typedef struct
  static vecpragma_ns_name registered_pp_pragmas;
  
  struct omp_pragma_def { const char *name; unsigned int id; };

+static const struct omp_pragma_def oacc_pragmas[] = {
+  { data, PRAGMA_OACC_DATA },
+  { enter, PRAGMA_OACC_ENTER_DATA },
+  { exit, PRAGMA_OACC_EXIT_DATA },
+  { kernels, PRAGMA_OACC_KERNELS },
+  { loop, PRAGMA_OACC_LOOP },
+  { parallel, PRAGMA_OACC_PARALLEL },
+  { update, PRAGMA_OACC_UPDATE },
+  { wait, PRAGMA_OACC_WAIT },

I'd avoid the , after the last element.


Fixed.




@@ -1383,6 +1402,17 @@ c_invoke_pragma_handler (unsigned int id)
  void
  init_pragma (void)
  {
+  if (flag_openacc)
+{
+  const int n_oacc_pragmas
+   = sizeof (oacc_pragmas) / sizeof (*oacc_pragmas);
+  int i;
+
+  for (i = 0; i  n_oacc_pragmas; ++i)
+   cpp_register_deferred_pragma (parse_in, acc, oacc_pragmas[i].name,
+ oacc_pragmas[i].id, true, true);
+}
+
if (flag_openmp)
  {
const int n_omp_pragmas = sizeof (omp_pragmas) / sizeof (*omp_pragmas);

Is -fopenmp -fopenacc tested not to run out of number of supported pragmas
by libcpp?


Tests will be added in a follow-up patch once the middle end has been 
accepted.





@@ -65,23 +74,30 @@ typedef enum pragma_kind {
  } pragma_kind;
  
  
-/* All clauses defined by OpenMP 2.5, 3.0, 3.1 and 4.0.

+/* All clauses defined by OpenACC 2.0, and OpenMP 2.5, 3.0, 3.1, and 4.0.
 Used internally by both C and C++ parsers.  */
  typedef enum pragma_omp_clause {
PRAGMA_OMP_CLAUSE_NONE = 0,
  
PRAGMA_OMP_CLAUSE_ALIGNED,

+  PRAGMA_OMP_CLAUSE_ASYNC,
PRAGMA_OMP_CLAUSE_COLLAPSE,
+  PRAGMA_OMP_CLAUSE_COPY,
PRAGMA_OMP_CLAUSE_COPYIN,
+  PRAGMA_OMP_CLAUSE_COPYOUT,
PRAGMA_OMP_CLAUSE_COPYPRIVATE,
+  PRAGMA_OMP_CLAUSE_CREATE,
PRAGMA_OMP_CLAUSE_DEFAULT,
+  PRAGMA_OMP_CLAUSE_DELETE,
PRAGMA_OMP_CLAUSE_DEPEND,
PRAGMA_OMP_CLAUSE_DEVICE,
+  PRAGMA_OMP_CLAUSE_DEVICEPTR,
PRAGMA_OMP_CLAUSE_DIST_SCHEDULE,
PRAGMA_OMP_CLAUSE_FINAL,
PRAGMA_OMP_CLAUSE_FIRSTPRIVATE,
PRAGMA_OMP_CLAUSE_FOR,
PRAGMA_OMP_CLAUSE_FROM,
+  PRAGMA_OMP_CLAUSE_HOST,
PRAGMA_OMP_CLAUSE_IF,
PRAGMA_OMP_CLAUSE_INBRANCH,
PRAGMA_OMP_CLAUSE_LASTPRIVATE,
@@ -90,16 +106,24 @@ typedef enum pragma_omp_clause {
PRAGMA_OMP_CLAUSE_MERGEABLE,
PRAGMA_OMP_CLAUSE_NOTINBRANCH,
PRAGMA_OMP_CLAUSE_NOWAIT,
+  PRAGMA_OMP_CLAUSE_NUM_GANGS,
PRAGMA_OMP_CLAUSE_NUM_TEAMS,
PRAGMA_OMP_CLAUSE_NUM_THREADS,
+  PRAGMA_OMP_CLAUSE_NUM_WORKERS,
PRAGMA_OMP_CLAUSE_ORDERED,
PRAGMA_OMP_CLAUSE_PARALLEL,
+  PRAGMA_OMP_CLAUSE_PRESENT,
+  PRAGMA_OMP_CLAUSE_PRESENT_OR_COPY,
+  PRAGMA_OMP_CLAUSE_PRESENT_OR_COPYIN,
+  PRAGMA_OMP_CLAUSE_PRESENT_OR_COPYOUT,
+  PRAGMA_OMP_CLAUSE_PRESENT_OR_CREATE,
PRAGMA_OMP_CLAUSE_PRIVATE,
PRAGMA_OMP_CLAUSE_PROC_BIND,
PRAGMA_OMP_CLAUSE_REDUCTION,
PRAGMA_OMP_CLAUSE_SAFELEN,
PRAGMA_OMP_CLAUSE_SCHEDULE,
PRAGMA_OMP_CLAUSE_SECTIONS,
+  PRAGMA_OMP_CLAUSE_SELF,
PRAGMA_OMP_CLAUSE_SHARED,
PRAGMA_OMP_CLAUSE_SIMDLEN,
PRAGMA_OMP_CLAUSE_TASKGROUP,
@@ -107,6 +131,8 @@ typedef enum pragma_omp_clause {
PRAGMA_OMP_CLAUSE_TO,

Re: [PATCH] OpenACC for C front end

[James Norris]

Hi!

On 11/13/2014 09:04 AM, Jakub Jelinek wrote:

On Wed, Nov 05, 2014 at 03:39:44PM -0600, James Norris wrote:

* c-typeck.c (c_finish_oacc_parallel, c_finish_oacc_kernels,
c_finish_oacc_data): New functions.
(handle_omp_array_sections, c_finish_omp_clauses):

Handle should be on the above line, no need to wrap too early.


Fixed.




@@ -9763,6 +9830,10 @@ c_parser_omp_clause_name (c_parser *parser)
  else if (!strcmp (from, p))
result = PRAGMA_OMP_CLAUSE_FROM;
  break;
+   case 'h':
+ if (!strcmp (host, p))
+   result = PRAGMA_OMP_CLAUSE_SELF;
+ break;

Shouldn't this be PRAGMA_OMP_CLAUSE_HOST (PRAGMA_OACC_CLAUSE_HOST)
instead?  It is _HOST in the C++ patch, are there no C tests with
that clause covering it?


The host clause is a synonym for the self clause. The initial
C++ patch did not treat host as a synonym and has amended
accordingly.


+  tree v = TREE_PURPOSE (t);
+
+  /* FIXME diagnostics: Ideally we should keep individual
+locations for all the variables in the var list to make the
+following errors more precise.  Perhaps
+c_parser_omp_var_list_parens() should construct a list of
+locations to go along with the var list.  */

Like in C++ patch, please avoid the comment, file a PR instead,
or just queue the work for GCC 6.


Will submit a PR.


+  /* Attempt to statically determine when the number isn't positive.  */
+  c = fold_build2_loc (expr_loc, LE_EXPR, boolean_type_node, t,
+  build_int_cst (TREE_TYPE (t), 0));

build_int_cst not aligned below expr_loc.


Fixed.


+  if (CAN_HAVE_LOCATION_P (c))
+   SET_EXPR_LOCATION (c, expr_loc);
+  if (c == boolean_true_node)
+   {
+ warning_at (expr_loc, 0,
+ %num_gangs% value must be positive);

This would fit perfectly on one line.


Fixed.


+  tree c, t;
+  location_t loc = c_parser_peek_token (parser)-location;
+
+  /* TODO XXX: FIX -1  (acc_async_noval).  */
+  t = build_int_cst (integer_type_node, -1);

Again, as in C++ patch, please avoid this.  Use gomp-constants.h,
or some enum, or explain what the -1 is, but avoid TODO XXX: FIX.


Fixed.


+  else
+{
+  t = c_fully_fold (t, false, NULL);
+}

Please avoid the {}s and reindent.


Fixed.




-/* OpenMP 4.0:
-   parallel
-   for
-   sections
-   taskgroup */
+  if (!INTEGRAL_TYPE_P (TREE_TYPE (t)))
+   {
+ c_parser_error (parser, expected integer expression);
+ return list;
+   }
  
-static tree

-c_parser_omp_clause_cancelkind (c_parser *parser ATTRIBUTE_UNUSED,
-   enum omp_clause_code code, tree list)
-{
-  tree c = build_omp_clause (c_parser_peek_token (parser)-location, code);
+  /* Attempt to statically determine when the number isn't positive.  */
+  c = fold_build2_loc (expr_loc, LE_EXPR, boolean_type_node, t,
+  build_int_cst (TREE_TYPE (t), 0));

I wonder if it wouldn't be better to just put the new OpenACC routines
into a new block of code, not stick it in between the OpenMP handling
routines, because diff apparently lost track and I'm afraid so will svn 
blame/git blame
and we'll lose history for the OpenMP changes.


There was a mistake in naming the function: 
c_parser_omp_clause_vector_length.

Once it was renamed to: c_parser_oacc_clause_vector_length, diff was able to
keep track.


+   case PRAGMA_OMP_CLAUSE_VECTOR_LENGTH:
+ clauses = c_parser_omp_clause_vector_length (parser, clauses);
+ c_name = vector_length;
+ break;

That is OpenACC clause, right?  Shouldn't the routine be called
c_parser_oacc_clause_vector_length?


Fixed.


+   case PRAGMA_OMP_CLAUSE_WAIT:
+ clauses = c_parser_oacc_clause_wait (parser, clauses);

E.g. c_parser_oacc_clause_wait is.


Fixed.




+  clauses =  c_parser_oacc_all_clauses (parser, OACC_DATA_CLAUSE_MASK,
+   #pragma acc data);

Too many spaces after =.


Fixed.


+/* OpenACC 2.0:
+   # pragma acc kernels oacc-kernels-clause[optseq] new-line
+ structured-block
+
+   LOC is the location of the #pragma token.

Again, what is LOC?


Fixed by removal of comment, along with other occurences.


+  clauses =  c_parser_oacc_all_clauses (parser, OACC_KERNELS_CLAUSE_MASK,
+   p_name);

See above.


Fixed.


+  c_parser_error (parser, enter
+ ? expected %data% in %#pragma acc enter data%
+ : expected %data% in %#pragma acc exit data%);
+  c_parser_skip_to_pragma_eol (parser);
+  return;
+}
+
+  const char *p = IDENTIFIER_POINTER (c_parser_peek_token (parser)-value);
+  if (strcmp (p, data) != 0)
+{
+  c_parser_error (parser, invalid pragma);

See the C++ patch review.


Fixed.


+  if (find_omp_clause (clauses, OMP_CLAUSE_MAP) == NULL_TREE)
+{
+  error_at (loc, enter
+   ? 

Re: [PATCH] OpenACC for C++ front end

[Jakub Jelinek]

On Wed, Nov 05, 2014 at 03:37:08PM -0600, James Norris wrote:
 2014-11-05  James Norris  jnor...@codesourcery.com
   Cesar Philippidis  ce...@codesourcery.com
   Thomas Schwinge  tho...@codesourcery.com
   Ilmir Usmanov  i.usma...@samsung.com

...

Please check formatting.  I see various spots with 8 spaces instead of tabs,
e.g. on
+  check_no_duplicate_clause (list, OMP_CLAUSE_VECTOR_LENGTH,
+vector_length, location);
even the alignment is wrong, I see calls without space before (:
+  if (args == NULL || args-length() == 0)
...
+ error(%wait% expression must be integral);
other spots where the alignment isn't right:
+static tree
+cp_parser_oacc_cache (cp_parser *parser,
+   cp_token *pragma_tok __attribute__((unused)))
(cp_token should be below cp_parser).  While at this,
__attribute__((unused)) should be replaced by ATTRIBUTE_UNUSED, or removing
the parameter name, or removing the parameter altogether even better.
For the formatting issues, you can easily look for them in the patch
(in lines starting with +), change the patch and apply interdiff to your
tree.

 --- a/gcc/c-family/c-omp.c
 +++ b/gcc/c-family/c-omp.c
 @@ -29,8 +29,47 @@ along with GCC; see the file COPYING3.  If not see
  #include c-pragma.h
  #include gimple-expr.h
  #include langhooks.h
 +#include omp-low.h

As Thomas? has said, you should include gomp-constants.h and use them in:

 +t = build_int_cst (integer_type_node, -2);  /* TODO: XXX FIX -2.  */

spots like this.

 --- a/gcc/c-family/c-pragma.c
 +++ b/gcc/c-family/c-pragma.c
 @@ -1180,6 +1180,16 @@ typedef struct
  static vecpragma_ns_name registered_pp_pragmas;
  
  struct omp_pragma_def { const char *name; unsigned int id; };
 +static const struct omp_pragma_def oacc_pragmas[] = {
 +  { data, PRAGMA_OACC_DATA },
 +  { enter, PRAGMA_OACC_ENTER_DATA },
 +  { exit, PRAGMA_OACC_EXIT_DATA },
 +  { kernels, PRAGMA_OACC_KERNELS },
 +  { loop, PRAGMA_OACC_LOOP },
 +  { parallel, PRAGMA_OACC_PARALLEL },
 +  { update, PRAGMA_OACC_UPDATE },
 +  { wait, PRAGMA_OACC_WAIT },

I'd avoid the , after the last element.

 @@ -1383,6 +1402,17 @@ c_invoke_pragma_handler (unsigned int id)
  void
  init_pragma (void)
  {
 +  if (flag_openacc)
 +{
 +  const int n_oacc_pragmas
 + = sizeof (oacc_pragmas) / sizeof (*oacc_pragmas);
 +  int i;
 +
 +  for (i = 0; i  n_oacc_pragmas; ++i)
 + cpp_register_deferred_pragma (parse_in, acc, oacc_pragmas[i].name,
 +   oacc_pragmas[i].id, true, true);
 +}
 +
if (flag_openmp)
  {
const int n_omp_pragmas = sizeof (omp_pragmas) / sizeof (*omp_pragmas);

Is -fopenmp -fopenacc tested not to run out of number of supported pragmas
by libcpp?

 @@ -65,23 +74,30 @@ typedef enum pragma_kind {
  } pragma_kind;
  
  
 -/* All clauses defined by OpenMP 2.5, 3.0, 3.1 and 4.0.
 +/* All clauses defined by OpenACC 2.0, and OpenMP 2.5, 3.0, 3.1, and 4.0.
 Used internally by both C and C++ parsers.  */
  typedef enum pragma_omp_clause {
PRAGMA_OMP_CLAUSE_NONE = 0,
  
PRAGMA_OMP_CLAUSE_ALIGNED,
 +  PRAGMA_OMP_CLAUSE_ASYNC,
PRAGMA_OMP_CLAUSE_COLLAPSE,
 +  PRAGMA_OMP_CLAUSE_COPY,
PRAGMA_OMP_CLAUSE_COPYIN,
 +  PRAGMA_OMP_CLAUSE_COPYOUT,
PRAGMA_OMP_CLAUSE_COPYPRIVATE,
 +  PRAGMA_OMP_CLAUSE_CREATE,
PRAGMA_OMP_CLAUSE_DEFAULT,
 +  PRAGMA_OMP_CLAUSE_DELETE,
PRAGMA_OMP_CLAUSE_DEPEND,
PRAGMA_OMP_CLAUSE_DEVICE,
 +  PRAGMA_OMP_CLAUSE_DEVICEPTR,
PRAGMA_OMP_CLAUSE_DIST_SCHEDULE,
PRAGMA_OMP_CLAUSE_FINAL,
PRAGMA_OMP_CLAUSE_FIRSTPRIVATE,
PRAGMA_OMP_CLAUSE_FOR,
PRAGMA_OMP_CLAUSE_FROM,
 +  PRAGMA_OMP_CLAUSE_HOST,
PRAGMA_OMP_CLAUSE_IF,
PRAGMA_OMP_CLAUSE_INBRANCH,
PRAGMA_OMP_CLAUSE_LASTPRIVATE,
 @@ -90,16 +106,24 @@ typedef enum pragma_omp_clause {
PRAGMA_OMP_CLAUSE_MERGEABLE,
PRAGMA_OMP_CLAUSE_NOTINBRANCH,
PRAGMA_OMP_CLAUSE_NOWAIT,
 +  PRAGMA_OMP_CLAUSE_NUM_GANGS,
PRAGMA_OMP_CLAUSE_NUM_TEAMS,
PRAGMA_OMP_CLAUSE_NUM_THREADS,
 +  PRAGMA_OMP_CLAUSE_NUM_WORKERS,
PRAGMA_OMP_CLAUSE_ORDERED,
PRAGMA_OMP_CLAUSE_PARALLEL,
 +  PRAGMA_OMP_CLAUSE_PRESENT,
 +  PRAGMA_OMP_CLAUSE_PRESENT_OR_COPY,
 +  PRAGMA_OMP_CLAUSE_PRESENT_OR_COPYIN,
 +  PRAGMA_OMP_CLAUSE_PRESENT_OR_COPYOUT,
 +  PRAGMA_OMP_CLAUSE_PRESENT_OR_CREATE,
PRAGMA_OMP_CLAUSE_PRIVATE,
PRAGMA_OMP_CLAUSE_PROC_BIND,
PRAGMA_OMP_CLAUSE_REDUCTION,
PRAGMA_OMP_CLAUSE_SAFELEN,
PRAGMA_OMP_CLAUSE_SCHEDULE,
PRAGMA_OMP_CLAUSE_SECTIONS,
 +  PRAGMA_OMP_CLAUSE_SELF,
PRAGMA_OMP_CLAUSE_SHARED,
PRAGMA_OMP_CLAUSE_SIMDLEN,
PRAGMA_OMP_CLAUSE_TASKGROUP,
 @@ -107,6 +131,8 @@ typedef enum pragma_omp_clause {
PRAGMA_OMP_CLAUSE_TO,
PRAGMA_OMP_CLAUSE_UNIFORM,
PRAGMA_OMP_CLAUSE_UNTIED,
 +  PRAGMA_OMP_CLAUSE_VECTOR_LENGTH,
 +  PRAGMA_OMP_CLAUSE_WAIT,

Like for CILK, I'd strongly prefer if for the clauses that are
specific to OpenACC only you'd use 

Re: [PATCH] OpenACC for C front end

[Jakub Jelinek]

On Wed, Nov 05, 2014 at 03:39:44PM -0600, James Norris wrote:
   * c-typeck.c (c_finish_oacc_parallel, c_finish_oacc_kernels,
   c_finish_oacc_data): New functions.
   (handle_omp_array_sections, c_finish_omp_clauses):

Handle should be on the above line, no need to wrap too early.

 @@ -9763,6 +9830,10 @@ c_parser_omp_clause_name (c_parser *parser)
 else if (!strcmp (from, p))
   result = PRAGMA_OMP_CLAUSE_FROM;
 break;
 + case 'h':
 +   if (!strcmp (host, p))
 + result = PRAGMA_OMP_CLAUSE_SELF;
 +   break;

Shouldn't this be PRAGMA_OMP_CLAUSE_HOST (PRAGMA_OACC_CLAUSE_HOST)
instead?  It is _HOST in the C++ patch, are there no C tests with
that clause covering it?

 +  tree v = TREE_PURPOSE (t);
 +
 +  /* FIXME diagnostics: Ideally we should keep individual
 +  locations for all the variables in the var list to make the
 +  following errors more precise.  Perhaps
 +  c_parser_omp_var_list_parens() should construct a list of
 +  locations to go along with the var list.  */

Like in C++ patch, please avoid the comment, file a PR instead,
or just queue the work for GCC 6.

 +  /* Attempt to statically determine when the number isn't positive.  */
 +  c = fold_build2_loc (expr_loc, LE_EXPR, boolean_type_node, t,
 +build_int_cst (TREE_TYPE (t), 0));

build_int_cst not aligned below expr_loc.

 +  if (CAN_HAVE_LOCATION_P (c))
 + SET_EXPR_LOCATION (c, expr_loc);
 +  if (c == boolean_true_node)
 + {
 +   warning_at (expr_loc, 0,
 +   %num_gangs% value must be positive);

This would fit perfectly on one line.

 +  tree c, t;
 +  location_t loc = c_parser_peek_token (parser)-location;
 +
 +  /* TODO XXX: FIX -1  (acc_async_noval).  */
 +  t = build_int_cst (integer_type_node, -1);

Again, as in C++ patch, please avoid this.  Use gomp-constants.h,
or some enum, or explain what the -1 is, but avoid TODO XXX: FIX.

 +  else
 +{
 +  t = c_fully_fold (t, false, NULL);
 +}

Please avoid the {}s and reindent.

 -/* OpenMP 4.0:
 -   parallel
 -   for
 -   sections
 -   taskgroup */
 +  if (!INTEGRAL_TYPE_P (TREE_TYPE (t)))
 + {
 +   c_parser_error (parser, expected integer expression);
 +   return list;
 + }
  
 -static tree
 -c_parser_omp_clause_cancelkind (c_parser *parser ATTRIBUTE_UNUSED,
 - enum omp_clause_code code, tree list)
 -{
 -  tree c = build_omp_clause (c_parser_peek_token (parser)-location, code);
 +  /* Attempt to statically determine when the number isn't positive.  */
 +  c = fold_build2_loc (expr_loc, LE_EXPR, boolean_type_node, t,
 +build_int_cst (TREE_TYPE (t), 0));

I wonder if it wouldn't be better to just put the new OpenACC routines
into a new block of code, not stick it in between the OpenMP handling
routines, because diff apparently lost track and I'm afraid so will svn 
blame/git blame
and we'll lose history for the OpenMP changes.

 + case PRAGMA_OMP_CLAUSE_VECTOR_LENGTH:
 +   clauses = c_parser_omp_clause_vector_length (parser, clauses);
 +   c_name = vector_length;
 +   break;

That is OpenACC clause, right?  Shouldn't the routine be called
c_parser_oacc_clause_vector_length?

 + case PRAGMA_OMP_CLAUSE_WAIT:
 +   clauses = c_parser_oacc_clause_wait (parser, clauses);

E.g. c_parser_oacc_clause_wait is.

 +  clauses =  c_parser_oacc_all_clauses (parser, OACC_DATA_CLAUSE_MASK,
 + #pragma acc data);

Too many spaces after =.

 +/* OpenACC 2.0:
 +   # pragma acc kernels oacc-kernels-clause[optseq] new-line
 + structured-block
 +
 +   LOC is the location of the #pragma token.

Again, what is LOC?

 +  clauses =  c_parser_oacc_all_clauses (parser, OACC_KERNELS_CLAUSE_MASK,
 + p_name);

See above.

 +  c_parser_error (parser, enter
 +   ? expected %data% in %#pragma acc enter data%
 +   : expected %data% in %#pragma acc exit data%);
 +  c_parser_skip_to_pragma_eol (parser);
 +  return;
 +}
 +
 +  const char *p = IDENTIFIER_POINTER (c_parser_peek_token (parser)-value);
 +  if (strcmp (p, data) != 0)
 +{
 +  c_parser_error (parser, invalid pragma);

See the C++ patch review.

 +  if (find_omp_clause (clauses, OMP_CLAUSE_MAP) == NULL_TREE)
 +{
 +  error_at (loc, enter
 + ? %#pragma acc enter data% has no data movement clause
 + : %#pragma acc exit data% has no data movement clause);

Similarly (though, in this case C++ had unconditional acc enter data).

 +  clauses =  c_parser_oacc_all_clauses (parser, OACC_PARALLEL_CLAUSE_MASK,
 + p_name);

See above.

 +  if (find_omp_clause (clauses, OMP_CLAUSE_MAP) == NULL_TREE)
 +{
 +  error_at (loc,
 + %#pragma acc update% must contain at least one 
 + %device% or %host/self% 

Re: [PATCH] OpenACC for C front end

[Joseph Myers]

On Wed, 5 Nov 2014, James Norris wrote:

 Hi!
 
 This patch represents the changes for OpenACC 2.0
 in the C front-end. At present these files will
 not compile as the changes for the middle end are
 not present.

So will things compile with the combination of this patch and the 
middle-end patch Thomas posted today?  If not, could you give a more 
detailed roadmap to the set of patches that need to be applied to current 
trunk to get this to compile?

Jakub has dealt with substantive review.  I'd add that testcases - for 
each diagnostic message in this patch, at least - should be included with 
the front-end patch.  (Execution tests can reasonably be included with the 
appropriate run-time library patch - as long as they *are* included.)

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH] OpenACC for C++ front end

[Thomas Schwinge]

Hi!

On Wed, 5 Nov 2014 21:41:02 +, Joseph Myers jos...@codesourcery.com wrote:
 I think the TODO: XXX FIX -2 and TODO XXX: FIX -1 comments need, at least, 
 more explanation of what the issue is and where the constants come from, 
 even if something is left until later to be fixed.

Such constants should go into include/gomp-constants.h.


Jim, for avoidance of doubt, please commit any such review changes to
gomp-4_0-branch, so that the (revised) patches that you submit and the
actual diff from the last merge of trunk into gomp-4_0-branch and
gomp-4_0-branch's current head are as similar as possible; preferably
you're submitting exactly (a relevant part of) that diff.


Grüße,
 Thomas


signature.asc
Description: PGP signature

Re: [PATCH] OpenACC for C++ front end

[Joseph Myers]

I think the TODO: XXX FIX -2 and TODO XXX: FIX -1 comments need, at least, 
more explanation of what the issue is and where the constants come from, 
even if something is left until later to be fixed.

-- 
Joseph S. Myers
jos...@codesourcery.com