Re: [OpenACC 5/11] C++ FE changes

2015-10-27 Thread Nathan Sidwell 

On 10/27/15 00:59, Jakub Jelinek wrote:

On Mon, Oct 26, 2015 at 03:35:20PM -0700, Cesar Philippidis wrote:

I used that generic message for all of those clauses except for _GANG,
_WORKER and _VECTOR. The gang clause, at the very least, needed it to
disambiguate the static and num arguments. If you want I can handle
_WORKER and _VECTOR with the generic message. I only included it because
those arguments are optional, whereas they are mandatory for the other
clauses.

Is this patch OK for trunk?


Ok.


this is the patch that I've committed.

nathan


2015-10-27  Cesar Philippidis  
	Thomas Schwinge  
	James Norris  
	Joseph Myers  
	Julian Brown  
	Nathan Sidwell 
	Bernd Schmidt  

	gcc/cp/
	* parser.c (cp_parser_omp_clause_name): Add auto, gang, seq,
	vector, worker.
	(cp_parser_oacc_simple_clause): New.
	(cp_parser_oacc_shape_clause): New.
	(cp_parser_oacc_all_clauses): Add auto, gang, seq, vector, worker.
	(OACC_LOOP_CLAUSE_MASK): Likewise.
	* semantics.c (finish_omp_clauses): Add auto, gang, seq, vector,
	worker. Unify the handling of teams, tasks and vector_length with
	the other loop shape clauses.

2015-10-27  Nathan Sidwell 
	Cesar Philippidis  

	gcc/testsuite/
	* g++.dg/g++.dg/gomp/pr33372-1.C: Adjust diagnostic.
	* gcc/testsuite/g++.dg/gomp/pr33372-3.C: Likewise.
			  
Index: gcc/cp/semantics.c
===
--- gcc/cp/semantics.c	(revision 229443)
+++ gcc/cp/semantics.c	(working copy)
@@ -5965,14 +5965,76 @@ finish_omp_clauses (tree clauses, bool a
 	  OMP_CLAUSE_FINAL_EXPR (c) = t;
 	  break;
 
+	case OMP_CLAUSE_GANG:
+	  /* Operand 1 is the gang static: argument.  */
+	  t = OMP_CLAUSE_OPERAND (c, 1);
+	  if (t != NULL_TREE)
+	{
+	  if (t == error_mark_node)
+		remove = true;
+	  else if (!type_dependent_expression_p (t)
+		   && !INTEGRAL_TYPE_P (TREE_TYPE (t)))
+		{
+		  error ("% static expression must be integral");
+		  remove = true;
+		}
+	  else
+		{
+		  t = mark_rvalue_use (t);
+		  if (!processing_template_decl)
+		{
+		  t = maybe_constant_value (t);
+		  if (TREE_CODE (t) == INTEGER_CST
+			  && tree_int_cst_sgn (t) != 1
+			  && t != integer_minus_one_node)
+			{
+			  warning_at (OMP_CLAUSE_LOCATION (c), 0,
+  "% static value must be"
+  "positive");
+			  t = integer_one_node;
+			}
+		}
+		  t = fold_build_cleanup_point_expr (TREE_TYPE (t), t);
+		}
+	  OMP_CLAUSE_OPERAND (c, 1) = t;
+	}
+	  /* Check operand 0, the num argument.  */
+
+	case OMP_CLAUSE_WORKER:
+	case OMP_CLAUSE_VECTOR:
+	  if (OMP_CLAUSE_OPERAND (c, 0) == NULL_TREE)
+	break;
+
+	case OMP_CLAUSE_NUM_TASKS:
+	case OMP_CLAUSE_NUM_TEAMS:
 	case OMP_CLAUSE_NUM_THREADS:
-	  t = OMP_CLAUSE_NUM_THREADS_EXPR (c);
+	case OMP_CLAUSE_NUM_GANGS:
+	case OMP_CLAUSE_NUM_WORKERS:
+	case OMP_CLAUSE_VECTOR_LENGTH:
+	  t = OMP_CLAUSE_OPERAND (c, 0);
 	  if (t == error_mark_node)
 	remove = true;
 	  else if (!type_dependent_expression_p (t)
 		   && !INTEGRAL_TYPE_P (TREE_TYPE (t)))
 	{
-	  error ("num_threads expression must be integral");
+	 switch (OMP_CLAUSE_CODE (c))
+		{
+		case OMP_CLAUSE_GANG:
+		  error_at (OMP_CLAUSE_LOCATION (c),
+			"% num expression must be integral"); break;
+		case OMP_CLAUSE_VECTOR:
+		  error_at (OMP_CLAUSE_LOCATION (c),
+			"% length expression must be integral");
+		  break;
+		case OMP_CLAUSE_WORKER:
+		  error_at (OMP_CLAUSE_LOCATION (c),
+			"% num expression must be integral");
+		  break;
+		default:
+		  error_at (OMP_CLAUSE_LOCATION (c),
+			"%qs expression must be integral",
+			omp_clause_code_name[OMP_CLAUSE_CODE (c)]);
+		}
 	  remove = true;
 	}
 	  else
@@ -5984,13 +6046,33 @@ finish_omp_clauses (tree clauses, bool a
 		  if (TREE_CODE (t) == INTEGER_CST
 		  && tree_int_cst_sgn (t) != 1)
 		{
-		  warning_at (OMP_CLAUSE_LOCATION (c), 0,
-  "% value must be positive");
+		  switch (OMP_CLAUSE_CODE (c))
+			{
+			case OMP_CLAUSE_GANG:
+			  warning_at (OMP_CLAUSE_LOCATION (c), 0,
+  "% num value must be positive");
+			  break;
+			case OMP_CLAUSE_VECTOR:
+			  warning_at (OMP_CLAUSE_LOCATION (c), 0,
+  "% length value must be"
+  "positive");
+			  break;
+			case OMP_CLAUSE_WORKER:
+			  warning_at (OMP_CLAUSE_LOCATION (c), 0,
+  "% num value must be"
+  "positive");
+			  break;
+			default:
+			  warning_at (OMP_CLAUSE_LOCATION (c), 0,
+  "%qs value must be positive",
+  omp_clause_code_name
+  [OMP_CLAUSE_CODE (c)]);
+			}
 		  t = integer_one_node;
 		}
 		  t = fold_build_cleanup_point_expr (TREE_TYPE (t), t);
 		}
-	  OMP_CLAUSE_NUM_THREADS_EXPR (c) = t;
+	  OMP_CLAUSE_OPERAND (c, 0) = t;
 	}
 	  break;
 
@@ -6062,35 +6144,6 @@ finish_omp_clauses (tree clauses, bool a
 	}
 	  break;
 
-	case OMP_CLAUSE_NUM_TEAMS:
-	  t = OMP_CLAUSE_NUM_TEAMS_EXPR (c);
-	  if (t == error_mark_node)

Re: [OpenACC 5/11] C++ FE changes

2015-10-27 Thread Jakub Jelinek 
On Mon, Oct 26, 2015 at 03:35:20PM -0700, Cesar Philippidis wrote:
> I used that generic message for all of those clauses except for _GANG,
> _WORKER and _VECTOR. The gang clause, at the very least, needed it to
> disambiguate the static and num arguments. If you want I can handle
> _WORKER and _VECTOR with the generic message. I only included it because
> those arguments are optional, whereas they are mandatory for the other
> clauses.
> 
> Is this patch OK for trunk?

Ok.

Jakub

Re: [OpenACC 5/11] C++ FE changes

2015-10-26 Thread Cesar Philippidis 
On 10/26/2015 03:20 AM, Jakub Jelinek wrote:
> On Sat, Oct 24, 2015 at 02:11:41PM -0700, Cesar Philippidis wrote:

>> --- a/gcc/cp/semantics.c
>> +++ b/gcc/cp/semantics.c
>> @@ -5911,6 +5911,31 @@ finish_omp_clauses (tree clauses, bool allow_fields, 
>> bool declare_simd)
>>  bitmap_set_bit (&firstprivate_head, DECL_UID (t));
>>goto handle_field_decl;
>>  
>> +case OMP_CLAUSE_GANG:
>> +case OMP_CLAUSE_VECTOR:
>> +case OMP_CLAUSE_WORKER:
>> +  /* Operand 0 is the num: or length: argument.  */
>> +  t = OMP_CLAUSE_OPERAND (c, 0);
>> +  if (t == NULL_TREE)
>> +break;
>> +
>> +  if (!processing_template_decl)
>> +t = fold_build_cleanup_point_expr (TREE_TYPE (t), t);
>> +  OMP_CLAUSE_OPERAND (c, 0) = t;
>> +
>> +  if (OMP_CLAUSE_CODE (c) != OMP_CLAUSE_GANG)
>> +break;
> 
> I think it would be better to do the Operand 1 stuff first for
> case OMP_CLAUSE_GANG: only, and then have /* FALLTHRU */ into
> case OMP_CLAUSE_{VECTOR,WORKER}: which would handle the first argument.
> 
> You should add testing that the operand has INTEGRAL_TYPE_P type
> (except that for processing_template_decl it can be
> type_dependent_expression_p instead of INTEGRAL_TYPE_P).
>
> Also, the if (t == NULL_TREE) stuff looks fishy, because e.g. right now
> if you have OMP_CLAUSE_GANG gang (static: expr) or similar,
> you wouldn't wrap the expr into cleanup point.
> So, instead it should be
>   if (t)
> {
>   if (t == error_mark_node)
>   remove = true;
>   else if (!type_dependent_expression_p (t)
>&& !INTEGRAL_TYPE_P (TREE_TYPE (t)))
>   {
> error_at (OMP_CLAUSE_LOCATION (c), ...);
> remove = true;
> }
>   else
>   {
> t = mark_rvalue_use (t);
> if (!processing_template_decl)
>   t = fold_build_cleanup_point_expr (TREE_TYPE (t), t);
> OMP_CLAUSE_OPERAND (c, 0) = t;
>   }
> }
> or so.  Also, can the expressions be arbitrary integers, or just
> non-negative, or positive?  If it is INTEGER_CST, that is something that
> could be checked here too.

I ended up handling with with OMP_CLAUSE_NUM_*, since they all require
positive integer expressions. The only exception was OMP_CLAUSE_GANG
which has two optional arguments.

>>else if (!type_dependent_expression_p (t)
>> && !INTEGRAL_TYPE_P (TREE_TYPE (t)))
>>  {
>> -  error ("num_threads expression must be integral");
>> + switch (OMP_CLAUSE_CODE (c))
>> +{
>> +case OMP_CLAUSE_NUM_TASKS:
>> +  error ("% expression must be integral"); break;
>> +case OMP_CLAUSE_NUM_TEAMS:
>> +  error ("% expression must be integral"); break;
>> +case OMP_CLAUSE_NUM_THREADS:
>> +  error ("% expression must be integral"); break;
>> +case OMP_CLAUSE_NUM_GANGS:
>> +  error ("% expression must be integral"); break;
>> +case OMP_CLAUSE_NUM_WORKERS:
>> +  error ("% expression must be integral");
>> +  break;
>> +case OMP_CLAUSE_VECTOR_LENGTH:
>> +  error ("% expression must be integral");
>> +  break;
> 
> When touching these, can you please use error_at (OMP_CLAUSE_LOCATION (c),
> instead of error ( ?

Done

>> +default:
>> +  error ("invalid argument");
> 
> What invalid argument?  I'd say that is clearly gcc_unreachable (); case.
> 
> But, I think it would be better to just use
>   error_at (OMP_CLAUSE_LOCATION (c), "%qs expression must be integral",
>   omp_clause_code_name[c]);

I used that generic message for all of those clauses except for _GANG,
_WORKER and _VECTOR. The gang clause, at the very least, needed it to
disambiguate the static and num arguments. If you want I can handle
_WORKER and _VECTOR with the generic message. I only included it because
those arguments are optional, whereas they are mandatory for the other
clauses.

Is this patch OK for trunk?

Cesar

2015-10-26  Cesar Philippidis  
	Thomas Schwinge  
	James Norris  
	Joseph Myers  
	Julian Brown  
	Nathan Sidwell 
	Bernd Schmidt  

	gcc/cp/
	* parser.c (cp_parser_omp_clause_name): Add auto, gang, seq,
	vector, worker.
	(cp_parser_oacc_simple_clause): New.
	(cp_parser_oacc_shape_clause): New.
	(cp_parser_oacc_all_clauses): Add auto, gang, seq, vector, worker.
	(OACC_LOOP_CLAUSE_MASK): Likewise.
	* semantics.c (finish_omp_clauses): Add auto, gang, seq, vector,
	worker. Unify the handling of teams, tasks and vector_length with
	the other loop shape clauses.

2015-10-26  Nathan Sidwell 
	Cesar Philippidis  

	gcc/testsuite/
	* g++.dg/g++.dg/gomp/pr33372-1.C: Adjust diagnostic.
	* gcc/testsuite/g++.dg/gomp/pr33372-3.C: Likewise.
			  

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 7555bf3..5d07487 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -29064,7 +29064,9 @@ cp_parser

Re: [OpenACC 5/11] C++ FE changes

2015-10-26 Thread Jakub Jelinek 
On Sat, Oct 24, 2015 at 02:11:41PM -0700, Cesar Philippidis wrote:
> @@ -29582,6 +29592,144 @@ cp_parser_oacc_data_clause_deviceptr (cp_parser 
> *parser, tree list)
>return list;
>  }
>  
> +/* OpenACC 2.0:
> +   auto
> +   independent
> +   nohost
> +   seq */
> +
> +static tree
> +cp_parser_oacc_simple_clause (cp_parser *ARG_UNUSED (parser),
> +   enum omp_clause_code code,
> +   tree list, location_t location)
> +{
> +  check_no_duplicate_clause (list, code, omp_clause_code_name[code], 
> location);
> +  tree c = build_omp_clause (location, code);
> +  OMP_CLAUSE_CHAIN (c) = list;
> +  return c;

Here the PARSER argument is unconditionally unused, I'd use what is used
elsewhere, i.e. cp_parser * /* parser */,

> +   idx = 1;
> +   if (ops[idx] != NULL)
> + {
> +   cp_parser_error (parser, "too many % arguements");

Typo, arguments.

> --- a/gcc/cp/semantics.c
> +++ b/gcc/cp/semantics.c
> @@ -5911,6 +5911,31 @@ finish_omp_clauses (tree clauses, bool allow_fields, 
> bool declare_simd)
>   bitmap_set_bit (&firstprivate_head, DECL_UID (t));
> goto handle_field_decl;
>  
> + case OMP_CLAUSE_GANG:
> + case OMP_CLAUSE_VECTOR:
> + case OMP_CLAUSE_WORKER:
> +   /* Operand 0 is the num: or length: argument.  */
> +   t = OMP_CLAUSE_OPERAND (c, 0);
> +   if (t == NULL_TREE)
> + break;
> +
> +   if (!processing_template_decl)
> + t = fold_build_cleanup_point_expr (TREE_TYPE (t), t);
> +   OMP_CLAUSE_OPERAND (c, 0) = t;
> +
> +   if (OMP_CLAUSE_CODE (c) != OMP_CLAUSE_GANG)
> + break;

I think it would be better to do the Operand 1 stuff first for
case OMP_CLAUSE_GANG: only, and then have /* FALLTHRU */ into
case OMP_CLAUSE_{VECTOR,WORKER}: which would handle the first argument.

You should add testing that the operand has INTEGRAL_TYPE_P type
(except that for processing_template_decl it can be
type_dependent_expression_p instead of INTEGRAL_TYPE_P).

Also, the if (t == NULL_TREE) stuff looks fishy, because e.g. right now
if you have OMP_CLAUSE_GANG gang (static: expr) or similar,
you wouldn't wrap the expr into cleanup point.
So, instead it should be
  if (t)
{
  if (t == error_mark_node)
remove = true;
  else if (!type_dependent_expression_p (t)
   && !INTEGRAL_TYPE_P (TREE_TYPE (t)))
{
  error_at (OMP_CLAUSE_LOCATION (c), ...);
  remove = true;
}
  else
{
  t = mark_rvalue_use (t);
  if (!processing_template_decl)
t = fold_build_cleanup_point_expr (TREE_TYPE (t), t);
  OMP_CLAUSE_OPERAND (c, 0) = t;
}
}
or so.  Also, can the expressions be arbitrary integers, or just
non-negative, or positive?  If it is INTEGER_CST, that is something that
could be checked here too.

> else if (!type_dependent_expression_p (t)
>  && !INTEGRAL_TYPE_P (TREE_TYPE (t)))
>   {
> -   error ("num_threads expression must be integral");
> +  switch (OMP_CLAUSE_CODE (c))
> + {
> + case OMP_CLAUSE_NUM_TASKS:
> +   error ("% expression must be integral"); break;
> + case OMP_CLAUSE_NUM_TEAMS:
> +   error ("% expression must be integral"); break;
> + case OMP_CLAUSE_NUM_THREADS:
> +   error ("% expression must be integral"); break;
> + case OMP_CLAUSE_NUM_GANGS:
> +   error ("% expression must be integral"); break;
> + case OMP_CLAUSE_NUM_WORKERS:
> +   error ("% expression must be integral");
> +   break;
> + case OMP_CLAUSE_VECTOR_LENGTH:
> +   error ("% expression must be integral");
> +   break;

When touching these, can you please use error_at (OMP_CLAUSE_LOCATION (c),
instead of error ( ?

> + default:
> +   error ("invalid argument");

What invalid argument?  I'd say that is clearly gcc_unreachable (); case.

But, I think it would be better to just use
  error_at (OMP_CLAUSE_LOCATION (c), "%qs expression must be integral",
omp_clause_code_name[c]);

> -   warning_at (OMP_CLAUSE_LOCATION (c), 0,
> -   "% value must be positive");
> +   switch (OMP_CLAUSE_CODE (c))
> + {
> + case OMP_CLAUSE_NUM_TASKS:
> +   warning_at (OMP_CLAUSE_LOCATION (c), 0,
> +   "% value must be positive");
> +   break;
> + case OMP_CLAUSE_NUM_TEAMS:
> +   warning_at (OMP_CLAUSE_LOCATION (c), 0,
> +   "% value must be positive");
> +   break;
> + case OMP_CLAUSE_NUM_THREADS:
> +   warning_at (OMP_CLAUSE_LOCATION (c),

Re: [OpenACC 5/11] C++ FE changes

2015-10-24 Thread Cesar Philippidis 
On 10/23/2015 07:37 PM, Cesar Philippidis wrote:
> On 10/23/2015 01:25 PM, Cesar Philippidis wrote:
>> On 10/22/2015 01:52 AM, Jakub Jelinek wrote:
>>> On Wed, Oct 21, 2015 at 03:18:55PM -0400, Nathan Sidwell wrote:
 This patch is the C++ changes matching the C ones of patch 4.  In
 finish_omp_clauses, the gang, worker, & vector clauses are handled the same
 as OpenMP's 'num_threads' clause.  One change to num_threads is the
 augmentation of a diagnostic to add %<...%>  markers to the clause name.
>>>
>>> Indeed, lots of older OpenMP diagnostics is missing %<...%> markers around
>>> keywords.  Something to fix eventually.
>>
>> I updated omp tasks and teams in semantics.c.
>>
 2015-10-20  Cesar Philippidis  
Thomas Schwinge  
James Norris  
Joseph Myers  
Julian Brown  
Nathan Sidwell 

* parser.c (cp_parser_omp_clause_name): Add auto, gang, seq,
vector, worker.
(cp_parser_oacc_simple_clause): New.
(cp_parser_oacc_shape_clause): New.
>>>
>>> What I've said for the C FE patch, plus:
>>>
 +if (cp_lexer_next_token_is (lexer, CPP_NAME)
 +|| cp_lexer_next_token_is (lexer, CPP_KEYWORD))
 +  {
 +tree name_kind = cp_lexer_peek_token (lexer)->u.value;
 +const char *p = IDENTIFIER_POINTER (name_kind);
 +if (kind == OMP_CLAUSE_GANG && strcmp ("static", p) == 0)
>>>
>>> As static is a keyword, wouldn't it be better to just handle that case
>>> using cp_lexer_next_token_is_keyword (lexer, RID_STATIC)?
>>>
>>> Also, what is the exact grammar of the shape arguments?
>>> Would be nice to describe the grammar, in the grammar you just say
>>> expression, at least for vector/worker, which is clearly not accurate.
>>>
>>> It seems the intent is that num: or length: or static: is optional, right?
>>> But if that is the case, you should treat those as parsed only if followed
>>> by :.  While static is a keyword, so you can't have a variable called like
>>> that, having vector(length) or vector(num) should not be rejected.
>>> So, I would have expected that it should test if it is RID_STATIC
>>> followed by CPP_COLON (and only in that case consume those tokens),
>>> or CPP_NAME of id followed by CPP_COLON (and only in that case consume those
>>> tokens), otherwise parse it as assignment expression.
>>
>> That function now peeks ahead to look for a colon, so now it can handle
>> variables with the name of clause keywords.
>>
>>> The C FE may have similar issue.  Plus of course there should be testsuite
>>> coverage for all the weird cases.
>>
>> I included a new test in a different patch because it's common to both c
>> and c++.
>>
 +  case OMP_CLAUSE_GANG:
 +  case OMP_CLAUSE_VECTOR:
 +  case OMP_CLAUSE_WORKER:
 +/* Operand 0 is the num: or length: argument.  */
 +t = OMP_CLAUSE_OPERAND (c, 0);
 +if (t == NULL_TREE)
 +  break;
 +
 +t = maybe_convert_cond (t);
>>>
>>> Can you explain the maybe_convert_cond calls (in both cases here,
>>> plus the preexisting in OMP_CLAUSE_VECTOR_LENGTH)?
>>> The reason why it is used for OpenMP if and final clauses is that those have
>>> a condition argument, either the condition is zero or non-zero (so
>>> effectively it is turned into a bool).
>>> But aren't the gang/vector/worker/vector_length arguments integers rather
>>> than conditions?  I'd expect that finish_omp_clauses should verify
>>> those operands are indeed integral expressions (if that is the requirement
>>> in the standard), as it is something that for C++ can't be verified during
>>> parsing, if arbitrary expressions are parsed there.
>>
>> It's probably a copy-and-paste error. This functionality was added
>> incrementally. I removed that check.
>>
 @@ -5959,32 +5990,58 @@ finish_omp_clauses (tree clauses, bool a
  break;
  
case OMP_CLAUSE_NUM_THREADS:
 -t = OMP_CLAUSE_NUM_THREADS_EXPR (c);
 -if (t == error_mark_node)
 -  remove = true;
 -else if (!type_dependent_expression_p (t)
 - && !INTEGRAL_TYPE_P (TREE_TYPE (t)))
 -  {
 -error ("num_threads expression must be integral");
 -remove = true;
 -  }
 -else
 -  {
 -t = mark_rvalue_use (t);
 -if (!processing_template_decl)
 -  {
 -t = maybe_constant_value (t);
 -if (TREE_CODE (t) == INTEGER_CST
 -&& tree_int_cst_sgn (t) != 1)
 -  {
 -warning_at (OMP_CLAUSE_LOCATION (c), 0,
 -"% value must be positive");
 -t = integer_one_node;
 -  }
 -t = fold_build_cleanup_point_expr (TREE_TYPE (t), t);
 -  }
 -OMP_CLAUSE_NUM_THREADS_EXPR (c) = t;
 -  }
 +  case OMP_CLAUSE_NUM_GANGS:
 +  case OMP_

Re: [OpenACC 5/11] C++ FE changes

2015-10-23 Thread Cesar Philippidis 
On 10/23/2015 01:25 PM, Cesar Philippidis wrote:
> On 10/22/2015 01:52 AM, Jakub Jelinek wrote:
>> On Wed, Oct 21, 2015 at 03:18:55PM -0400, Nathan Sidwell wrote:
>>> This patch is the C++ changes matching the C ones of patch 4.  In
>>> finish_omp_clauses, the gang, worker, & vector clauses are handled the same
>>> as OpenMP's 'num_threads' clause.  One change to num_threads is the
>>> augmentation of a diagnostic to add %<...%>  markers to the clause name.
>>
>> Indeed, lots of older OpenMP diagnostics is missing %<...%> markers around
>> keywords.  Something to fix eventually.
> 
> I updated omp tasks and teams in semantics.c.
> 
>>> 2015-10-20  Cesar Philippidis  
>>> Thomas Schwinge  
>>> James Norris  
>>> Joseph Myers  
>>> Julian Brown  
>>> Nathan Sidwell 
>>>
>>> * parser.c (cp_parser_omp_clause_name): Add auto, gang, seq,
>>> vector, worker.
>>> (cp_parser_oacc_simple_clause): New.
>>> (cp_parser_oacc_shape_clause): New.
>>
>> What I've said for the C FE patch, plus:
>>
>>> + if (cp_lexer_next_token_is (lexer, CPP_NAME)
>>> + || cp_lexer_next_token_is (lexer, CPP_KEYWORD))
>>> +   {
>>> + tree name_kind = cp_lexer_peek_token (lexer)->u.value;
>>> + const char *p = IDENTIFIER_POINTER (name_kind);
>>> + if (kind == OMP_CLAUSE_GANG && strcmp ("static", p) == 0)
>>
>> As static is a keyword, wouldn't it be better to just handle that case
>> using cp_lexer_next_token_is_keyword (lexer, RID_STATIC)?
>>
>> Also, what is the exact grammar of the shape arguments?
>> Would be nice to describe the grammar, in the grammar you just say
>> expression, at least for vector/worker, which is clearly not accurate.
>>
>> It seems the intent is that num: or length: or static: is optional, right?
>> But if that is the case, you should treat those as parsed only if followed
>> by :.  While static is a keyword, so you can't have a variable called like
>> that, having vector(length) or vector(num) should not be rejected.
>> So, I would have expected that it should test if it is RID_STATIC
>> followed by CPP_COLON (and only in that case consume those tokens),
>> or CPP_NAME of id followed by CPP_COLON (and only in that case consume those
>> tokens), otherwise parse it as assignment expression.
> 
> That function now peeks ahead to look for a colon, so now it can handle
> variables with the name of clause keywords.
> 
>> The C FE may have similar issue.  Plus of course there should be testsuite
>> coverage for all the weird cases.
> 
> I included a new test in a different patch because it's common to both c
> and c++.
> 
>>> +   case OMP_CLAUSE_GANG:
>>> +   case OMP_CLAUSE_VECTOR:
>>> +   case OMP_CLAUSE_WORKER:
>>> + /* Operand 0 is the num: or length: argument.  */
>>> + t = OMP_CLAUSE_OPERAND (c, 0);
>>> + if (t == NULL_TREE)
>>> +   break;
>>> +
>>> + t = maybe_convert_cond (t);
>>
>> Can you explain the maybe_convert_cond calls (in both cases here,
>> plus the preexisting in OMP_CLAUSE_VECTOR_LENGTH)?
>> The reason why it is used for OpenMP if and final clauses is that those have
>> a condition argument, either the condition is zero or non-zero (so
>> effectively it is turned into a bool).
>> But aren't the gang/vector/worker/vector_length arguments integers rather
>> than conditions?  I'd expect that finish_omp_clauses should verify
>> those operands are indeed integral expressions (if that is the requirement
>> in the standard), as it is something that for C++ can't be verified during
>> parsing, if arbitrary expressions are parsed there.
> 
> It's probably a copy-and-paste error. This functionality was added
> incrementally. I removed that check.
> 
>>> @@ -5959,32 +5990,58 @@ finish_omp_clauses (tree clauses, bool a
>>>   break;
>>>  
>>> case OMP_CLAUSE_NUM_THREADS:
>>> - t = OMP_CLAUSE_NUM_THREADS_EXPR (c);
>>> - if (t == error_mark_node)
>>> -   remove = true;
>>> - else if (!type_dependent_expression_p (t)
>>> -  && !INTEGRAL_TYPE_P (TREE_TYPE (t)))
>>> -   {
>>> - error ("num_threads expression must be integral");
>>> - remove = true;
>>> -   }
>>> - else
>>> -   {
>>> - t = mark_rvalue_use (t);
>>> - if (!processing_template_decl)
>>> -   {
>>> - t = maybe_constant_value (t);
>>> - if (TREE_CODE (t) == INTEGER_CST
>>> - && tree_int_cst_sgn (t) != 1)
>>> -   {
>>> - warning_at (OMP_CLAUSE_LOCATION (c), 0,
>>> - "% value must be positive");
>>> - t = integer_one_node;
>>> -   }
>>> - t = fold_build_cleanup_point_expr (TREE_TYPE (t), t);
>>> -   }
>>> - OMP_CLAUSE_NUM_THREADS_EXPR (c) = t;
>>> -   }
>>> +   case OMP_CLAUSE_NUM_GANGS:
>>> +   case OMP_CLAUSE_NUM_WORKERS:
>>> +   case OMP_CLAUSE_VECTOR_LENGTH:
>>
>> If you are already merging some of the

Re: Re: [OpenACC 5/11] C++ FE changes

2015-10-23 Thread Cesar Philippidis 
On 10/22/2015 01:52 AM, Jakub Jelinek wrote:
> On Wed, Oct 21, 2015 at 03:18:55PM -0400, Nathan Sidwell wrote:
>> This patch is the C++ changes matching the C ones of patch 4.  In
>> finish_omp_clauses, the gang, worker, & vector clauses are handled the same
>> as OpenMP's 'num_threads' clause.  One change to num_threads is the
>> augmentation of a diagnostic to add %<...%>  markers to the clause name.
> 
> Indeed, lots of older OpenMP diagnostics is missing %<...%> markers around
> keywords.  Something to fix eventually.

I updated omp tasks and teams in semantics.c.

>> 2015-10-20  Cesar Philippidis  
>>  Thomas Schwinge  
>>  James Norris  
>>  Joseph Myers  
>>  Julian Brown  
>>  Nathan Sidwell 
>>
>>  * parser.c (cp_parser_omp_clause_name): Add auto, gang, seq,
>>  vector, worker.
>>  (cp_parser_oacc_simple_clause): New.
>>  (cp_parser_oacc_shape_clause): New.
> 
> What I've said for the C FE patch, plus:
> 
>> +  if (cp_lexer_next_token_is (lexer, CPP_NAME)
>> +  || cp_lexer_next_token_is (lexer, CPP_KEYWORD))
>> +{
>> +  tree name_kind = cp_lexer_peek_token (lexer)->u.value;
>> +  const char *p = IDENTIFIER_POINTER (name_kind);
>> +  if (kind == OMP_CLAUSE_GANG && strcmp ("static", p) == 0)
> 
> As static is a keyword, wouldn't it be better to just handle that case
> using cp_lexer_next_token_is_keyword (lexer, RID_STATIC)?
> 
> Also, what is the exact grammar of the shape arguments?
> Would be nice to describe the grammar, in the grammar you just say
> expression, at least for vector/worker, which is clearly not accurate.
> 
> It seems the intent is that num: or length: or static: is optional, right?
> But if that is the case, you should treat those as parsed only if followed
> by :.  While static is a keyword, so you can't have a variable called like
> that, having vector(length) or vector(num) should not be rejected.
> So, I would have expected that it should test if it is RID_STATIC
> followed by CPP_COLON (and only in that case consume those tokens),
> or CPP_NAME of id followed by CPP_COLON (and only in that case consume those
> tokens), otherwise parse it as assignment expression.

That function now peeks ahead to look for a colon, so now it can handle
variables with the name of clause keywords.

> The C FE may have similar issue.  Plus of course there should be testsuite
> coverage for all the weird cases.

I included a new test in a different patch because it's common to both c
and c++.

>> +case OMP_CLAUSE_GANG:
>> +case OMP_CLAUSE_VECTOR:
>> +case OMP_CLAUSE_WORKER:
>> +  /* Operand 0 is the num: or length: argument.  */
>> +  t = OMP_CLAUSE_OPERAND (c, 0);
>> +  if (t == NULL_TREE)
>> +break;
>> +
>> +  t = maybe_convert_cond (t);
> 
> Can you explain the maybe_convert_cond calls (in both cases here,
> plus the preexisting in OMP_CLAUSE_VECTOR_LENGTH)?
> The reason why it is used for OpenMP if and final clauses is that those have
> a condition argument, either the condition is zero or non-zero (so
> effectively it is turned into a bool).
> But aren't the gang/vector/worker/vector_length arguments integers rather
> than conditions?  I'd expect that finish_omp_clauses should verify
> those operands are indeed integral expressions (if that is the requirement
> in the standard), as it is something that for C++ can't be verified during
> parsing, if arbitrary expressions are parsed there.

It's probably a copy-and-paste error. This functionality was added
incrementally. I removed that check.

>> @@ -5959,32 +5990,58 @@ finish_omp_clauses (tree clauses, bool a
>>break;
>>  
>>  case OMP_CLAUSE_NUM_THREADS:
>> -  t = OMP_CLAUSE_NUM_THREADS_EXPR (c);
>> -  if (t == error_mark_node)
>> -remove = true;
>> -  else if (!type_dependent_expression_p (t)
>> -   && !INTEGRAL_TYPE_P (TREE_TYPE (t)))
>> -{
>> -  error ("num_threads expression must be integral");
>> -  remove = true;
>> -}
>> -  else
>> -{
>> -  t = mark_rvalue_use (t);
>> -  if (!processing_template_decl)
>> -{
>> -  t = maybe_constant_value (t);
>> -  if (TREE_CODE (t) == INTEGER_CST
>> -  && tree_int_cst_sgn (t) != 1)
>> -{
>> -  warning_at (OMP_CLAUSE_LOCATION (c), 0,
>> -  "% value must be positive");
>> -  t = integer_one_node;
>> -}
>> -  t = fold_build_cleanup_point_expr (TREE_TYPE (t), t);
>> -}
>> -  OMP_CLAUSE_NUM_THREADS_EXPR (c) = t;
>> -}
>> +case OMP_CLAUSE_NUM_GANGS:
>> +case OMP_CLAUSE_NUM_WORKERS:
>> +case OMP_CLAUSE_VECTOR_LENGTH:
> 
> If you are already merging some of the similar handling, please
> handle OMP_CLAUSE_NUM_TEAMS and OMP_CLAUSE_NUM_TASKS the same way.

I did that, but I also ha

Re: [OpenACC 5/11] C++ FE changes

2015-10-22 Thread Jakub Jelinek 
On Wed, Oct 21, 2015 at 03:18:55PM -0400, Nathan Sidwell wrote:
> This patch is the C++ changes matching the C ones of patch 4.  In
> finish_omp_clauses, the gang, worker, & vector clauses are handled the same
> as OpenMP's 'num_threads' clause.  One change to num_threads is the
> augmentation of a diagnostic to add %<...%>  markers to the clause name.

Indeed, lots of older OpenMP diagnostics is missing %<...%> markers around
keywords.  Something to fix eventually.

> 2015-10-20  Cesar Philippidis  
>   Thomas Schwinge  
>   James Norris  
>   Joseph Myers  
>   Julian Brown  
>   Nathan Sidwell 
> 
>   * parser.c (cp_parser_omp_clause_name): Add auto, gang, seq,
>   vector, worker.
>   (cp_parser_oacc_simple_clause): New.
>   (cp_parser_oacc_shape_clause): New.

What I've said for the C FE patch, plus:

> +   if (cp_lexer_next_token_is (lexer, CPP_NAME)
> +   || cp_lexer_next_token_is (lexer, CPP_KEYWORD))
> + {
> +   tree name_kind = cp_lexer_peek_token (lexer)->u.value;
> +   const char *p = IDENTIFIER_POINTER (name_kind);
> +   if (kind == OMP_CLAUSE_GANG && strcmp ("static", p) == 0)

As static is a keyword, wouldn't it be better to just handle that case
using cp_lexer_next_token_is_keyword (lexer, RID_STATIC)?

Also, what is the exact grammar of the shape arguments?
Would be nice to describe the grammar, in the grammar you just say
expression, at least for vector/worker, which is clearly not accurate.

It seems the intent is that num: or length: or static: is optional, right?
But if that is the case, you should treat those as parsed only if followed
by :.  While static is a keyword, so you can't have a variable called like
that, having vector(length) or vector(num) should not be rejected.
So, I would have expected that it should test if it is RID_STATIC
followed by CPP_COLON (and only in that case consume those tokens),
or CPP_NAME of id followed by CPP_COLON (and only in that case consume those
tokens), otherwise parse it as assignment expression.

The C FE may have similar issue.  Plus of course there should be testsuite
coverage for all the weird cases.

> + case OMP_CLAUSE_GANG:
> + case OMP_CLAUSE_VECTOR:
> + case OMP_CLAUSE_WORKER:
> +   /* Operand 0 is the num: or length: argument.  */
> +   t = OMP_CLAUSE_OPERAND (c, 0);
> +   if (t == NULL_TREE)
> + break;
> +
> +   t = maybe_convert_cond (t);

Can you explain the maybe_convert_cond calls (in both cases here,
plus the preexisting in OMP_CLAUSE_VECTOR_LENGTH)?
The reason why it is used for OpenMP if and final clauses is that those have
a condition argument, either the condition is zero or non-zero (so
effectively it is turned into a bool).
But aren't the gang/vector/worker/vector_length arguments integers rather
than conditions?  I'd expect that finish_omp_clauses should verify
those operands are indeed integral expressions (if that is the requirement
in the standard), as it is something that for C++ can't be verified during
parsing, if arbitrary expressions are parsed there.

> @@ -5959,32 +5990,58 @@ finish_omp_clauses (tree clauses, bool a
> break;
>  
>   case OMP_CLAUSE_NUM_THREADS:
> -   t = OMP_CLAUSE_NUM_THREADS_EXPR (c);
> -   if (t == error_mark_node)
> - remove = true;
> -   else if (!type_dependent_expression_p (t)
> -&& !INTEGRAL_TYPE_P (TREE_TYPE (t)))
> - {
> -   error ("num_threads expression must be integral");
> -   remove = true;
> - }
> -   else
> - {
> -   t = mark_rvalue_use (t);
> -   if (!processing_template_decl)
> - {
> -   t = maybe_constant_value (t);
> -   if (TREE_CODE (t) == INTEGER_CST
> -   && tree_int_cst_sgn (t) != 1)
> - {
> -   warning_at (OMP_CLAUSE_LOCATION (c), 0,
> -   "% value must be positive");
> -   t = integer_one_node;
> - }
> -   t = fold_build_cleanup_point_expr (TREE_TYPE (t), t);
> - }
> -   OMP_CLAUSE_NUM_THREADS_EXPR (c) = t;
> - }
> + case OMP_CLAUSE_NUM_GANGS:
> + case OMP_CLAUSE_NUM_WORKERS:
> + case OMP_CLAUSE_VECTOR_LENGTH:

If you are already merging some of the similar handling, please
handle OMP_CLAUSE_NUM_TEAMS and OMP_CLAUSE_NUM_TASKS the same way.

Jakub

Re: [OpenACC 5/11] C++ FE changes

2015-10-21 Thread Nathan Sidwell 
This patch is the C++ changes matching the C ones of patch 4.  In 
finish_omp_clauses, the gang, worker, & vector clauses are handled the same as 
OpenMP's 'num_threads' clause.  One change to num_threads is the augmentation of 
a diagnostic to add %<...%>  markers to the clause name.


nathan

2015-10-20  Cesar Philippidis  
	Thomas Schwinge  
	James Norris  
	Joseph Myers  
	Julian Brown  
	Nathan Sidwell 

	* parser.c (cp_parser_omp_clause_name): Add auto, gang, seq,
	vector, worker.
	(cp_parser_oacc_simple_clause): New.
	(cp_parser_oacc_shape_clause): New.
	(cp_parser_oacc_all_clauses): Add auto, gang, seq, vector, worker.
	(OACC_LOOP_CLAUSE_MASK): Likewise.
	* semantics.c (finish_omp_clauses): Add auto, gang, seq, vector,
	worker.

2015-10-20  Nathan Sidwell 

	gcc/testsuite/
	* g++.dg/g++.dg/gomp/pr33372-1.C: Adjust diagnostic.

Index: gcc/cp/parser.c
===
--- gcc/cp/parser.c	(revision 228969)
+++ gcc/cp/parser.c	(working copy)
@@ -29058,7 +29058,9 @@ cp_parser_omp_clause_name (cp_parser *pa
 {
   pragma_omp_clause result = PRAGMA_OMP_CLAUSE_NONE;
 
-  if (cp_lexer_next_token_is_keyword (parser->lexer, RID_IF))
+  if (cp_lexer_next_token_is_keyword (parser->lexer, RID_AUTO))
+result = PRAGMA_OACC_CLAUSE_AUTO;
+  else if (cp_lexer_next_token_is_keyword (parser->lexer, RID_IF))
 result = PRAGMA_OMP_CLAUSE_IF;
   else if (cp_lexer_next_token_is_keyword (parser->lexer, RID_DEFAULT))
 result = PRAGMA_OMP_CLAUSE_DEFAULT;
@@ -29116,7 +29118,9 @@ cp_parser_omp_clause_name (cp_parser *pa
 	result = PRAGMA_OMP_CLAUSE_FROM;
 	  break;
 	case 'g':
-	  if (!strcmp ("grainsize", p))
+	  if (!strcmp ("gang", p))
+	result = PRAGMA_OACC_CLAUSE_GANG;
+	  else if (!strcmp ("grainsize", p))
 	result = PRAGMA_OMP_CLAUSE_GRAINSIZE;
 	  break;
 	case 'h':
@@ -29206,6 +29210,8 @@ cp_parser_omp_clause_name (cp_parser *pa
 	result = PRAGMA_OMP_CLAUSE_SECTIONS;
 	  else if (!strcmp ("self", p))
 	result = PRAGMA_OACC_CLAUSE_SELF;
+	  else if (!strcmp ("seq", p))
+	result = PRAGMA_OACC_CLAUSE_SEQ;
 	  else if (!strcmp ("shared", p))
 	result = PRAGMA_OMP_CLAUSE_SHARED;
 	  else if (!strcmp ("simd", p))
@@ -29232,7 +29238,9 @@ cp_parser_omp_clause_name (cp_parser *pa
 	result = PRAGMA_OMP_CLAUSE_USE_DEVICE_PTR;
 	  break;
 	case 'v':
-	  if (!strcmp ("vector_length", p))
+	  if (!strcmp ("vector", p))
+	result = PRAGMA_OACC_CLAUSE_VECTOR;
+	  else if (!strcmp ("vector_length", p))
 	result = PRAGMA_OACC_CLAUSE_VECTOR_LENGTH;
 	  else if (flag_cilkplus && !strcmp ("vectorlength", p))
 	result = PRAGMA_CILK_CLAUSE_VECTORLENGTH;
@@ -29240,6 +29248,8 @@ cp_parser_omp_clause_name (cp_parser *pa
 	case 'w':
 	  if (!strcmp ("wait", p))
 	result = PRAGMA_OACC_CLAUSE_WAIT;
+	  else if (!strcmp ("worker", p))
+	result = PRAGMA_OACC_CLAUSE_WORKER;
 	  break;
 	}
 }
@@ -29576,6 +29586,160 @@ cp_parser_oacc_data_clause_deviceptr (cp
   return list;
 }
 
+/* OpenACC 2.0:
+   auto
+   independent
+   nohost
+   seq */
+
+static tree
+cp_parser_oacc_simple_clause (cp_parser *ARG_UNUSED (parser),
+			  enum omp_clause_code code,
+			  tree list, location_t location)
+{
+  check_no_duplicate_clause (list, code, omp_clause_code_name[code], location);
+  tree c = build_omp_clause (location, code);
+  OMP_CLAUSE_CHAIN (c) = list;
+  return c;
+}
+
+/* OpenACC:
+   gang [( gang_expr_list )]
+   worker [( expression )]
+   vector [( expression )] */
+
+static tree
+cp_parser_oacc_shape_clause (cp_parser *parser, pragma_omp_clause c_kind,
+			 const char *str, tree list)
+{
+  omp_clause_code kind;
+  const char *id = "num";
+  cp_lexer *lexer = parser->lexer;
+
+  switch (c_kind)
+{
+default:
+  gcc_unreachable ();
+case PRAGMA_OACC_CLAUSE_GANG:
+  kind = OMP_CLAUSE_GANG;
+  break;
+case PRAGMA_OACC_CLAUSE_VECTOR:
+  kind = OMP_CLAUSE_VECTOR;
+  id = "length";
+  break;
+case PRAGMA_OACC_CLAUSE_WORKER:
+  kind = OMP_CLAUSE_WORKER;
+  break;
+}
+
+  tree op0 = NULL_TREE, op1 = NULL_TREE;
+  location_t loc = cp_lexer_peek_token (lexer)->location;
+
+  if (cp_lexer_next_token_is (lexer, CPP_OPEN_PAREN))
+{
+  tree *op_to_parse = &op0;
+  cp_lexer_consume_token (lexer);
+
+  do
+	{
+	  if (cp_lexer_next_token_is (lexer, CPP_NAME)
+	  || cp_lexer_next_token_is (lexer, CPP_KEYWORD))
+	{
+	  tree name_kind = cp_lexer_peek_token (lexer)->u.value;
+	  const char *p = IDENTIFIER_POINTER (name_kind);
+	  if (kind == OMP_CLAUSE_GANG && strcmp ("static", p) == 0)
+		{
+		  cp_lexer_consume_token (lexer);
+		  if (!cp_parser_require (parser, CPP_COLON, RT_COLON))
+		{
+		  cp_parser_skip_to_closing_parenthesis (parser, false,
+			 false, true);
+		  return list;
+		}
+		  op_to_parse = &op1;
+		  if (cp_lexer_next_token_is (lexer, CPP_MULT))
+		{
+		  if (*op_to_parse != NULL_TREE)