diff mbox

Re: [OpenACC 4/11] C FE changes

Message ID 562A95C3.2040100@mentor.com
State New
Headers show

Commit Message

Cesar Philippidis Oct. 23, 2015, 8:17 p.m. UTC 
On 10/22/2015 01:22 AM, Jakub Jelinek wrote:
> On Wed, Oct 21, 2015 at 03:16:20PM -0400, Nathan Sidwell wrote:
>> 2015-10-20  Cesar Philippidis  <cesar@codesourcery.com>
>> 	    Thomas Schwinge  <thomas@codesourcery.com>
>> 	    James Norris  <jnorris@codesourcery.com>
>> 	    Joseph Myers  <joseph@codesourcery.com>
>> 	    Julian Brown  <julian@codesourcery.com>
>>
>> 	* c-parser.c (c_parser_oacc_shape_clause): New.
>> 	(c_parser_oacc_simple_clause): New.
>> 	(c_parser_oacc_all_clauses): Add auto, gang, seq, vector, worker.
>> 	(OACC_LOOP_CLAUSE_MASK): Add gang, worker, vector, auto, seq.
> 
> Ok, with one nit.
> 
>>  /* OpenACC:
>> +   gang [( gang_expr_list )]
>> +   worker [( expression )]
>> +   vector [( expression )] */
>> +
>> +static tree
>> +c_parser_oacc_shape_clause (c_parser *parser, pragma_omp_clause c_kind,
>> +			    const char *str, tree list)
> 
> I think it would be better to remove the c_kind argument and pass to this
> function omp_clause_code kind instead.  The callers are already in a big
> switch, with a separate call for each of the clauses.
> After all, e.g. for c_parser_oacc_simple_clause you already do it that way
> too.
> 
>> +{
>> +  omp_clause_code kind;
>> +  const char *id = "num";
>> +
>> +  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;
>> +    }
> 
> Then you can replace this switch with just if (kind == OMP_CLAUSE_VECTOR)
> id = "length";

Good idea, thanks. This patch also corrects the problems parsing weird
combinations of num, static and length arguments that you mentioned
elsewhere.

Is this OK for trunk?

Nathan, can you try out this patch with your updated patch set? I saw
some test cases getting stuck when expanding expand_GOACC_DIM_SIZE in on
the host compiler, which is wrong. I don't see that happening in
gomp-4_0-branch with this patch. Also, can you merge this patch along
with the c++ and new test case patches to trunk? I'll handle the gomp4
backport.

Cesar

Comments

Jakub Jelinek Oct. 23, 2015, 8:31 p.m. UTC | #1 
On Fri, Oct 23, 2015 at 01:17:07PM -0700, Cesar Philippidis wrote:
> Good idea, thanks. This patch also corrects the problems parsing weird
> combinations of num, static and length arguments that you mentioned
> elsewhere.
> 
> Is this OK for trunk?

I'd strongly prefer to see always patches accompanied by testcases.

> +	  loc = c_parser_peek_token (parser)->location;
> +	  op_to_parse = &op0;
> +
> +	  if ((c_parser_next_token_is (parser, CPP_NAME)
> +	       || c_parser_next_token_is (parser, CPP_KEYWORD))
> +	      && c_parser_peek_2nd_token (parser)->type == CPP_COLON)
> +	    {
> +	      tree name_kind = c_parser_peek_token (parser)->value;
> +	      const char *p = IDENTIFIER_POINTER (name_kind);

I think I'd prefer not to peek at this at all if it is RID_STATIC,
so perhaps just have (and name_kind is weird):
	      else
		{
		  tree val = c_parser_peek_token (parser)->value;
		  if (strcmp (id, IDENTIFIER_POINTER (val)) == 0)
		    {
		      c_parser_consume_token (parser);  /* id  */
		      c_parser_consume_token (parser);  /* ':'  */
		    }
		  else
		    {
...
		    }
		}
?

> +	      if (kind == OMP_CLAUSE_GANG
> +		  && c_parser_next_token_is_keyword (parser, RID_STATIC))
> +		{
> +		  c_parser_consume_token (parser); /* static  */
> +		  c_parser_consume_token (parser); /* ':'  */
> +
> +		  op_to_parse = &op1;
> +		  if (c_parser_next_token_is (parser, CPP_MULT))
> +		    {
> +		      c_parser_consume_token (parser);
> +		      *op_to_parse = integer_minus_one_node;
> +
> +		      /* Consume a comma if present.  */
> +		      if (c_parser_next_token_is (parser, CPP_COMMA))
> +			c_parser_consume_token (parser);

Doesn't this mean that you happily parse
gang (static: * abc)
or
gang (static:*num:1)
etc.?  I'd say the comma should be non-optional (i.e. either accept
CPP_COMMA, or CPP_CLOSE_PARENT, but nothing else) in that case (at least,
when in OpenMP grammar something is *-list it is meant to be comma
separated).

> +	  /* Consume a comma if present.  */
> +	  if (c_parser_next_token_is (parser, CPP_COMMA))
> +	    c_parser_consume_token (parser);

Similarly this means
gang (num: 5 static: *)
is accepted.  If it is valid, then again it should have testsuite coverage.

	Jakub
Jakub Jelinek Oct. 23, 2015, 9:25 p.m. UTC | #2 
On Fri, Oct 23, 2015 at 10:31:55PM +0200, Jakub Jelinek wrote:
> Doesn't this mean that you happily parse
> gang (static: * abc)
> or
> gang (static:*num:1)
> etc.?  I'd say the comma should be non-optional (i.e. either accept
> CPP_COMMA, or CPP_CLOSE_PARENT, but nothing else) in that case (at least,
> when in OpenMP grammar something is *-list it is meant to be comma
> separated).

Looking at the OpenACC standard, gang-arg-list is indeed a comma separated
list of gang-arg, so the above are not valid, so you really should just
error out and skip to close paren if at that spot isn't a CPP_COMMA or
CPP_CLOSE_PAREN.  And for vector/worker arguments, which don't accept a
*-list, IMNSHO you shouldn't even try to accept CPP_COMMA, just require
CPP_CLOSE_PAREN.

> 
> > +	  /* Consume a comma if present.  */
> > +	  if (c_parser_next_token_is (parser, CPP_COMMA))
> > +	    c_parser_consume_token (parser);
> 
> Similarly this means
> gang (num: 5 static: *)
> is accepted.  If it is valid, then again it should have testsuite coverage.

	Jakub
Nathan Sidwell Oct. 23, 2015, 9:25 p.m. UTC | #3 
On 10/23/15 16:17, Cesar Philippidis wrote:

> Nathan, can you try out this patch with your updated patch set? I saw
> some test cases getting stuck when expanding expand_GOACC_DIM_SIZE in on
> the host compiler, which is wrong. I don't see that happening in
> gomp-4_0-branch with this patch. Also, can you merge this patch along
> with the c++ and new test case patches to trunk? I'll handle the gomp4
> backport.

Wilco.


nathan
Cesar Philippidis Oct. 23, 2015, 9:31 p.m. UTC | #4 
On 10/23/2015 01:31 PM, Jakub Jelinek wrote:
> On Fri, Oct 23, 2015 at 01:17:07PM -0700, Cesar Philippidis wrote:
>> Good idea, thanks. This patch also corrects the problems parsing weird
>> combinations of num, static and length arguments that you mentioned
>> elsewhere.
>>
>> Is this OK for trunk?
> 
> I'd strongly prefer to see always patches accompanied by testcases.
> 
>> +	  loc = c_parser_peek_token (parser)->location;
>> +	  op_to_parse = &op0;
>> +
>> +	  if ((c_parser_next_token_is (parser, CPP_NAME)
>> +	       || c_parser_next_token_is (parser, CPP_KEYWORD))
>> +	      && c_parser_peek_2nd_token (parser)->type == CPP_COLON)
>> +	    {
>> +	      tree name_kind = c_parser_peek_token (parser)->value;
>> +	      const char *p = IDENTIFIER_POINTER (name_kind);
> 
> I think I'd prefer not to peek at this at all if it is RID_STATIC,
> so perhaps just have (and name_kind is weird):
> 	      else
> 		{
> 		  tree val = c_parser_peek_token (parser)->value;
> 		  if (strcmp (id, IDENTIFIER_POINTER (val)) == 0)
> 		    {
> 		      c_parser_consume_token (parser);  /* id  */
> 		      c_parser_consume_token (parser);  /* ':'  */
> 		    }
> 		  else
> 		    {
> ...
> 		    }
> 		}
> ?

My plan over here was try and catch any arguments with a colon. But that
fell threw because...

>> +	      if (kind == OMP_CLAUSE_GANG
>> +		  && c_parser_next_token_is_keyword (parser, RID_STATIC))
>> +		{
>> +		  c_parser_consume_token (parser); /* static  */
>> +		  c_parser_consume_token (parser); /* ':'  */
>> +
>> +		  op_to_parse = &op1;
>> +		  if (c_parser_next_token_is (parser, CPP_MULT))
>> +		    {
>> +		      c_parser_consume_token (parser);
>> +		      *op_to_parse = integer_minus_one_node;
>> +
>> +		      /* Consume a comma if present.  */
>> +		      if (c_parser_next_token_is (parser, CPP_COMMA))
>> +			c_parser_consume_token (parser);
> 
> Doesn't this mean that you happily parse
> gang (static: * abc)
> or
> gang (static:*num:1)
> etc.?  I'd say the comma should be non-optional (i.e. either accept
> CPP_COMMA, or CPP_CLOSE_PARENT, but nothing else) in that case (at least,
> when in OpenMP grammar something is *-list it is meant to be comma
> separated).

I'm not handling commas properly. My next patch is going to handle the
static argument separately.

>> +	  /* Consume a comma if present.  */
>> +	  if (c_parser_next_token_is (parser, CPP_COMMA))
>> +	    c_parser_consume_token (parser);
> 
> Similarly this means
> gang (num: 5 static: *)
> is accepted.  If it is valid, then again it should have testsuite coverage.

I'll include a test case for this with the next patch.

Cesar
Nathan Sidwell Oct. 25, 2015, 2:03 p.m. UTC | #5 
On 10/23/15 17:25, Nathan Sidwell wrote:
> On 10/23/15 16:17, Cesar Philippidis wrote:
>
>> Nathan, can you try out this patch with your updated patch set? I saw
>> some test cases getting stuck when expanding expand_GOACC_DIM_SIZE in on
>> the host compiler, which is wrong. I don't see that happening in
>> gomp-4_0-branch with this patch. Also, can you merge this patch along
>> with the c++ and new test case patches to trunk? I'll handle the gomp4
>> backport.
>
> Wilco.

testing  your patch on trunk along with my IFN_UNIQUE changes shows good results.

nathan
diff mbox

Patch

2015-10-20  Cesar Philippidis  <cesar@codesourcery.com>
	    Thomas Schwinge  <thomas@codesourcery.com>
	    James Norris  <jnorris@codesourcery.com>
	    Joseph Myers  <joseph@codesourcery.com>
	    Julian Brown  <julian@codesourcery.com>
	    Bernd Schmidt  <bschmidt@redhat.com>

	* c-parser.c (c_parser_oacc_shape_clause): New.
	(c_parser_oacc_simple_clause): New.
	(c_parser_oacc_all_clauses): Add auto, gang, seq, vector, worker.
	(OACC_LOOP_CLAUSE_MASK): Add gang, worker, vector, auto, seq.


diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index c8c6a2d..1e3c333 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -11188,6 +11188,142 @@  c_parser_omp_clause_num_workers (c_parser *parser, tree list)
 }
 
 /* OpenACC:
+
+    gang [( gang-arg-list )]
+    worker [( [num:] int-expr )]
+    vector [( [length:] int-expr )]
+
+  where gang-arg is one of:
+
+    [num:] int-expr
+    static: size-expr
+
+  and size-expr may be:
+
+    *
+    int-expr
+*/
+
+static tree
+c_parser_oacc_shape_clause (c_parser *parser, omp_clause_code kind,
+			    const char *str, tree list)
+{
+  const char *id = "num";
+
+  if (kind == OMP_CLAUSE_VECTOR)
+    id = "length";
+
+  tree op0 = NULL_TREE, op1 = NULL_TREE;
+  location_t loc = c_parser_peek_token (parser)->location;
+
+  if (c_parser_next_token_is (parser, CPP_OPEN_PAREN))
+    {
+      tree *op_to_parse = &op0;
+      c_parser_consume_token (parser);
+
+      do
+	{
+	  loc = c_parser_peek_token (parser)->location;
+	  op_to_parse = &op0;
+
+	  if ((c_parser_next_token_is (parser, CPP_NAME)
+	       || c_parser_next_token_is (parser, CPP_KEYWORD))
+	      && c_parser_peek_2nd_token (parser)->type == CPP_COLON)
+	    {
+	      tree name_kind = c_parser_peek_token (parser)->value;
+	      const char *p = IDENTIFIER_POINTER (name_kind);
+	      if (kind == OMP_CLAUSE_GANG
+		  && c_parser_next_token_is_keyword (parser, RID_STATIC))
+		{
+		  c_parser_consume_token (parser); /* static  */
+		  c_parser_consume_token (parser); /* ':'  */
+
+		  op_to_parse = &op1;
+		  if (c_parser_next_token_is (parser, CPP_MULT))
+		    {
+		      c_parser_consume_token (parser);
+		      *op_to_parse = integer_minus_one_node;
+
+		      /* Consume a comma if present.  */
+		      if (c_parser_next_token_is (parser, CPP_COMMA))
+			c_parser_consume_token (parser);
+
+		      continue;
+		    }
+		}
+	      else if (strcmp (id, p) == 0)
+		{
+		  c_parser_consume_token (parser);  /* id  */
+		  c_parser_consume_token (parser);  /* ':'  */
+		}
+	      else
+		{
+		  if (kind == OMP_CLAUSE_GANG)
+		    c_parser_error (parser, "expected %<num%> or %<static%>");
+		  else if (kind == OMP_CLAUSE_VECTOR)
+		    c_parser_error (parser, "expected %<length%>");
+		  else
+		    c_parser_error (parser, "expected %<num%>");
+		  c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, 0);
+		  return list;
+		}
+	    }
+
+	  if (*op_to_parse != NULL_TREE)
+	    {
+	      c_parser_error (parser, "unexpected argument");
+	      c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, 0);
+	      return list;
+	    }
+
+	  tree expr = c_parser_expr_no_commas (parser, NULL).value;
+	  if (expr == error_mark_node)
+	    {
+	      c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, 0);
+	      return list;
+	    }
+
+	  mark_exp_read (expr);
+	  *op_to_parse = expr;
+
+	  /* Consume a comma if present.  */
+	  if (c_parser_next_token_is (parser, CPP_COMMA))
+	    c_parser_consume_token (parser);
+	}
+      while (!c_parser_next_token_is (parser, CPP_CLOSE_PAREN));
+      c_parser_consume_token (parser);
+    }
+
+  check_no_duplicate_clause (list, kind, str);
+
+  tree c = build_omp_clause (loc, kind);
+  if (op0)
+    OMP_CLAUSE_OPERAND (c, 0) = op0;
+  if (op1)
+    OMP_CLAUSE_OPERAND (c, 1) = op1;
+  OMP_CLAUSE_CHAIN (c) = list;
+  return c;
+}
+
+/* OpenACC:
+   auto
+   independent
+   nohost
+   seq */
+
+static tree
+c_parser_oacc_simple_clause (c_parser *parser ATTRIBUTE_UNUSED,
+			     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);
+  OMP_CLAUSE_CHAIN (c) = list;
+
+  return c;
+}
+
+/* OpenACC:
    async [( int-expr )] */
 
 static tree
@@ -12393,6 +12529,11 @@  c_parser_oacc_all_clauses (c_parser *parser, omp_clause_mask mask,
 	  clauses = c_parser_oacc_clause_async (parser, clauses);
 	  c_name = "async";
 	  break;
+	case PRAGMA_OACC_CLAUSE_AUTO:
+	  clauses = c_parser_oacc_simple_clause (parser, OMP_CLAUSE_AUTO,
+						clauses);
+	  c_name = "auto";
+	  break;
 	case PRAGMA_OACC_CLAUSE_COLLAPSE:
 	  clauses = c_parser_omp_clause_collapse (parser, clauses);
 	  c_name = "collapse";
@@ -12429,6 +12570,11 @@  c_parser_oacc_all_clauses (c_parser *parser, omp_clause_mask mask,
 	  clauses = c_parser_omp_clause_firstprivate (parser, clauses);
 	  c_name = "firstprivate";
 	  break;
+	case PRAGMA_OACC_CLAUSE_GANG:
+	  c_name = "gang";
+	  clauses = c_parser_oacc_shape_clause (parser, OMP_CLAUSE_GANG,
+						c_name, clauses);
+	  break;
 	case PRAGMA_OACC_CLAUSE_HOST:
 	  clauses = c_parser_oacc_data_clause (parser, c_kind, clauses);
 	  c_name = "host";
@@ -12477,6 +12623,16 @@  c_parser_oacc_all_clauses (c_parser *parser, omp_clause_mask mask,
 	  clauses = c_parser_oacc_data_clause (parser, c_kind, clauses);
 	  c_name = "self";
 	  break;
+	case PRAGMA_OACC_CLAUSE_SEQ:
+	  clauses = c_parser_oacc_simple_clause (parser, OMP_CLAUSE_SEQ,
+						clauses);
+	  c_name = "seq";
+	  break;
+	case PRAGMA_OACC_CLAUSE_VECTOR:
+	  c_name = "vector";
+	  clauses = c_parser_oacc_shape_clause (parser, OMP_CLAUSE_VECTOR,
+						c_name,	clauses);
+	  break;
 	case PRAGMA_OACC_CLAUSE_VECTOR_LENGTH:
 	  clauses = c_parser_omp_clause_vector_length (parser, clauses);
 	  c_name = "vector_length";
@@ -12485,6 +12641,11 @@  c_parser_oacc_all_clauses (c_parser *parser, omp_clause_mask mask,
 	  clauses = c_parser_oacc_clause_wait (parser, clauses);
 	  c_name = "wait";
 	  break;
+	case PRAGMA_OACC_CLAUSE_WORKER:
+	  c_name = "worker";
+	  clauses = c_parser_oacc_shape_clause (parser, OMP_CLAUSE_WORKER,
+						c_name, clauses);
+	  break;
 	default:
 	  c_parser_error (parser, "expected %<#pragma acc%> clause");
 	  goto saw_error;
@@ -13015,6 +13176,11 @@  c_parser_oacc_enter_exit_data (c_parser *parser, bool enter)
 
 #define OACC_LOOP_CLAUSE_MASK						\
 	( (OMP_CLAUSE_MASK_1 << PRAGMA_OACC_CLAUSE_COLLAPSE)		\
+	| (OMP_CLAUSE_MASK_1 << PRAGMA_OACC_CLAUSE_GANG)		\
+	| (OMP_CLAUSE_MASK_1 << PRAGMA_OACC_CLAUSE_WORKER)		\
+	| (OMP_CLAUSE_MASK_1 << PRAGMA_OACC_CLAUSE_VECTOR)		\
+	| (OMP_CLAUSE_MASK_1 << PRAGMA_OACC_CLAUSE_AUTO)		\
+	| (OMP_CLAUSE_MASK_1 << PRAGMA_OACC_CLAUSE_SEQ)			\
 	| (OMP_CLAUSE_MASK_1 << PRAGMA_OACC_CLAUSE_REDUCTION) )
 
 static tree