diff mbox

[3/5] OpenACC tile clause support, C/C++ front-end parts

Message ID 56973ea6-3f8c-0035-8443-a14aa7b88437@mentor.com
State New
Headers show

Commit Message

Chung-Lin Tang Nov. 17, 2016, 9:34 a.m. UTC
On 2016/11/11 6:30 PM, Jakub Jelinek wrote:
> On Thu, Nov 10, 2016 at 06:46:16PM +0800, Chung-Lin Tang wrote:

>> 2016-XX-XX  Nathan Sidwell  <nathan@codesourcery.com>

>>

>>         c/

>>         * c-parser.c (c_parser_omp_clause_collapse): Disallow tile.

>>         (c_parser_oacc_clause_tile): Disallow collapse. Fix parsing and

>>         semantic checking.

>>         * c-parser.c (c_parser_omp_for_loop): Accept tiling constructs.

>>

>>         cp/

>> 	* parser.c (cp_parser_oacc_clause_tile): Disallow collapse.  Fix

>>         parsing.  Parse constant expression. Remove semantic checking.

>>         (cp_parser_omp_clause_collapse): Disallow tile.

>>         (cp_parser_omp_for_loop): Deal with tile clause.  Don't emit a

> 

> Similarly to the previous patch, some lines have spaces instead of tabs.

> 

>> 	parse error about missing for after already emitting one.

>> 	Use more conventional for idiom for unbounded loop.

>> 	* pt.c (tsubst_omp_clauses): Require integral constant expression

>> 	for COLLAPSE and TILE.  Remove broken TILE subst.

>> 	* semantics.c (finish_omp_clauses): Correct TILE semantic check.

>> 	(finish_omp_for): Deal with tile clause.

>>

>>         gcc/testsuite/

>>         * c-c++-common/goacc/loop-auto-1.c: Adjust and add additional

>>         case.

>>         * c-c++-common/goacc/loop-auto-2.c: New.

>>         * c-c++-common/goacc/tile.c: Include stdbool, fix expected errors.

>>         * g++.dg/goacc/template.C: Test tile subst.  Adjust erroneous

>>         uses.

>> 	* g++.dg/goacc/tile-1.C: Check tile subst.

>> 	* gcc.dg/goacc/loop-processing-1.c: Adjust dg-final pattern.

> 

>> +	  if (!INTEGRAL_TYPE_P (TREE_TYPE (expr))

>> +	      || TREE_CODE (expr) != INTEGER_CST

> 

> No need to test for INTEGER_CST, tree_fits_shwi_p will test that.

> 

>> +	      || !tree_fits_shwi_p (expr)

>> +	      || tree_to_shwi (expr) <= 0)

>>  	    {

>> -	      warning_at (expr_loc, 0,"%<tile%> value must be positive");

>> -	      expr = integer_one_node;

>> +	      error_at (expr_loc, "%<tile%> argument needs positive"

>> +			" integral constant");

>> +	      expr = integer_zero_node;

>>  	    }

>>  	}

> 

>> @@ -14713,6 +14713,7 @@ tsubst_omp_clauses (tree clauses, enum c_omp_regio

>>        nc = copy_node (oc);

>>        OMP_CLAUSE_CHAIN (nc) = new_clauses;

>>        new_clauses = nc;

>> +      bool needs_ice = false;

>>  

>>        switch (OMP_CLAUSE_CODE (nc))

>>  	{

>> @@ -14742,10 +14743,16 @@ tsubst_omp_clauses (tree clauses, enum c_omp_regio

>>  	    = tsubst_omp_clause_decl (OMP_CLAUSE_DECL (oc), args, complain,

>>  				      in_decl);

>>  	  break;

>> +	case OMP_CLAUSE_COLLAPSE:

>> +	case OMP_CLAUSE_TILE:

>> +	  /* These clauses really need a positive integral constant

>> +	     expression, but we don't have a predicate for that

>> +	     (yet).  */

>> +	  needs_ice = true;

>> +	  /* FALLTHRU */

> 

> As I said earlier on gcc-patches, no need to change anything for

> OMP_CLAUSE_COLLAPSE, we require that the argument is a constant integer

> already at parsing time, it can't be e.g. a template integral parameter.

> And for OMP_CLAUSE_TILE, please avoid the needs_ice var, instead don't fall

> through into the tsubst_expr and copy it over and change the argument there

> instead, it is short enough.

> 

>> +		      if (TREE_CODE (t) != INTEGER_CST

>> +			  || !tree_fits_shwi_p (t)

> 

> Again, no need to check for INTEGER_CST when tree_fits_shwi_p will do that.

> 

> 	Jakub


Updated C/C++ front-end patches, adjusted as reviewed.

Thanks,
Chung-Lin

Comments

Jason Merrill Nov. 17, 2016, 7:55 p.m. UTC | #1
On 11/17/2016 04:34 AM, Chung-Lin Tang wrote:
> +	  /* These clauses really need a positive integral constant expression,

> +	     but we don't have a predicate for that (yet).  */


What do you mean?  Don't you check this in finish_omp_clauses after the 
tsubst?

Jason
Jakub Jelinek Nov. 18, 2016, 11:23 a.m. UTC | #2
On Thu, Nov 17, 2016 at 05:34:34PM +0800, Chung-Lin Tang wrote:
> Updated C/C++ front-end patches, adjusted as reviewed.


Jason is right, finish_omp_clauses will verify the tile operands
when !processing_template_decl are non-negative host INTEGER_CSTs,
so can't you just tsubst it like OMP_CLAUSE_COLLAPSE?  If the operand
is not a constant expression, presumably it will not be INTEGER_CST.
On the other side, OMP_CLAUSE_TILE has now 3 operands instead of just 1,
don't you need to do something during instantiation for the other 2
operands?

	Jakub
diff mbox

Patch

Index: c/c-parser.c
===================================================================
--- c/c-parser.c	(revision 241809)
+++ c/c-parser.c	(working copy)
@@ -11010,6 +11010,7 @@  c_parser_omp_clause_collapse (c_parser *parser, tr
   location_t loc;
 
   check_no_duplicate_clause (list, OMP_CLAUSE_COLLAPSE, "collapse");
+  check_no_duplicate_clause (list, OMP_CLAUSE_TILE, "tile");
 
   loc = c_parser_peek_token (parser)->location;
   if (c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>"))
@@ -11920,10 +11921,11 @@  static tree
 c_parser_oacc_clause_tile (c_parser *parser, tree list)
 {
   tree c, expr = error_mark_node;
-  location_t loc, expr_loc;
+  location_t loc;
   tree tile = NULL_TREE;
 
   check_no_duplicate_clause (list, OMP_CLAUSE_TILE, "tile");
+  check_no_duplicate_clause (list, OMP_CLAUSE_COLLAPSE, "collapse");
 
   loc = c_parser_peek_token (parser)->location;
   if (!c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>"))
@@ -11931,16 +11933,19 @@  c_parser_oacc_clause_tile (c_parser *parser, tree
 
   do
     {
+      if (tile && !c_parser_require (parser, CPP_COMMA, "expected %<,%>"))
+	return list;
+
       if (c_parser_next_token_is (parser, CPP_MULT)
 	  && (c_parser_peek_2nd_token (parser)->type == CPP_COMMA
 	      || c_parser_peek_2nd_token (parser)->type == CPP_CLOSE_PAREN))
 	{
 	  c_parser_consume_token (parser);
-	  expr = integer_minus_one_node;
+	  expr = integer_zero_node;
 	}
       else
 	{
-	  expr_loc = c_parser_peek_token (parser)->location;
+	  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);
 	  expr = cexpr.value;
@@ -11952,28 +11957,19 @@  c_parser_oacc_clause_tile (c_parser *parser, tree
 	      return list;
 	    }
 
-	  if (!INTEGRAL_TYPE_P (TREE_TYPE (expr)))
-	    {
-	      c_parser_error (parser, "%<tile%> value must be integral");
-	      return list;
-	    }
-
 	  expr = c_fully_fold (expr, false, NULL);
 
-	  /* Attempt to statically determine when expr isn't positive.  */
-	  c = fold_build2_loc (expr_loc, LE_EXPR, boolean_type_node, expr,
-			       build_int_cst (TREE_TYPE (expr), 0));
-	  protected_set_expr_location (c, expr_loc);
-	  if (c == boolean_true_node)
+	  if (!INTEGRAL_TYPE_P (TREE_TYPE (expr))
+	      || !tree_fits_shwi_p (expr)
+	      || tree_to_shwi (expr) <= 0)
 	    {
-	      warning_at (expr_loc, 0,"%<tile%> value must be positive");
-	      expr = integer_one_node;
+	      error_at (expr_loc, "%<tile%> argument needs positive"
+			" integral constant");
+	      expr = integer_zero_node;
 	    }
 	}
 
       tile = tree_cons (NULL_TREE, expr, tile);
-      if (c_parser_next_token_is (parser, CPP_COMMA))
-	c_parser_consume_token (parser);
     }
   while (c_parser_next_token_is_not (parser, CPP_CLOSE_PAREN));
 
@@ -14899,11 +14895,17 @@  c_parser_omp_for_loop (location_t loc, c_parser *p
   bool fail = false, open_brace_parsed = false;
   int i, collapse = 1, ordered = 0, count, nbraces = 0;
   location_t for_loc;
+  bool tiling = false;
   vec<tree, va_gc> *for_block = make_tree_vector ();
 
   for (cl = clauses; cl; cl = OMP_CLAUSE_CHAIN (cl))
     if (OMP_CLAUSE_CODE (cl) == OMP_CLAUSE_COLLAPSE)
       collapse = tree_to_shwi (OMP_CLAUSE_COLLAPSE_EXPR (cl));
+    else if (OMP_CLAUSE_CODE (cl) == OMP_CLAUSE_TILE)
+      {
+	tiling = true;
+	collapse = list_length (OMP_CLAUSE_TILE_LIST (cl));
+      }
     else if (OMP_CLAUSE_CODE (cl) == OMP_CLAUSE_ORDERED
 	     && OMP_CLAUSE_ORDERED_EXPR (cl))
       {
@@ -14933,7 +14935,7 @@  c_parser_omp_for_loop (location_t loc, c_parser *p
 	  pc = &OMP_CLAUSE_CHAIN (*pc);
     }
 
-  gcc_assert (collapse >= 1 && ordered >= 0);
+  gcc_assert (tiling || (collapse >= 1 && ordered >= 0));
   count = ordered ? ordered : collapse;
 
   declv = make_tree_vec (count);
Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 241809)
+++ cp/pt.c	(working copy)
@@ -14742,6 +14742,13 @@  tsubst_omp_clauses (tree clauses, enum c_omp_regio
 	    = tsubst_omp_clause_decl (OMP_CLAUSE_DECL (oc), args, complain,
 				      in_decl);
 	  break;
+	case OMP_CLAUSE_TILE:
+	  /* These clauses really need a positive integral constant expression,
+	     but we don't have a predicate for that (yet).  */
+	  OMP_CLAUSE_OPERAND (nc, 0)
+	    = tsubst_expr (OMP_CLAUSE_OPERAND (oc, 0), args, complain, in_decl,
+			   /*integral_constant_expression_p=*/true);
+	  break;
 	case OMP_CLAUSE_IF:
 	case OMP_CLAUSE_NUM_THREADS:
 	case OMP_CLAUSE_SCHEDULE:
@@ -14766,7 +14773,7 @@  tsubst_omp_clauses (tree clauses, enum c_omp_regio
 	case OMP_CLAUSE_ASYNC:
 	case OMP_CLAUSE_WAIT:
 	  OMP_CLAUSE_OPERAND (nc, 0)
-	    = tsubst_expr (OMP_CLAUSE_OPERAND (oc, 0), args, complain, 
+	    = tsubst_expr (OMP_CLAUSE_OPERAND (oc, 0), args, complain,
 			   in_decl, /*integral_constant_expression_p=*/false);
 	  break;
 	case OMP_CLAUSE_REDUCTION:
@@ -14836,19 +14843,6 @@  tsubst_omp_clauses (tree clauses, enum c_omp_regio
 	case OMP_CLAUSE_AUTO:
 	case OMP_CLAUSE_SEQ:
 	  break;
-	case OMP_CLAUSE_TILE:
-	  {
-	    tree lnc, loc;
-	    for (lnc = OMP_CLAUSE_TILE_LIST (nc),
-		   loc = OMP_CLAUSE_TILE_LIST (oc);
-		 loc;
-		 loc = TREE_CHAIN (loc), lnc = TREE_CHAIN (lnc))
-	      {
-		TREE_VALUE (lnc) = tsubst_expr (TREE_VALUE (loc), args,
-						complain, in_decl, false);
-	      }
-	  }
-	  break;
 	default:
 	  gcc_unreachable ();
 	}
Index: cp/semantics.c
===================================================================
--- cp/semantics.c	(revision 241809)
+++ cp/semantics.c	(working copy)
@@ -7086,7 +7086,8 @@  finish_omp_clauses (tree clauses, enum c_omp_regio
 	      else if (!type_dependent_expression_p (t)
 		       && !INTEGRAL_TYPE_P (TREE_TYPE (t)))
 		{
-		  error ("%<tile%> value must be integral");
+		  error_at (OMP_CLAUSE_LOCATION (c),
+			    "%<tile%> argument needs integral type");
 		  remove = true;
 		}
 	      else
@@ -7094,14 +7095,16 @@  finish_omp_clauses (tree clauses, enum c_omp_regio
 		  t = mark_rvalue_use (t);
 		  if (!processing_template_decl)
 		    {
+		      /* Zero is used to indicate '*', we permit you
+			 to get there via an ICE of value zero.  */
 		      t = maybe_constant_value (t);
-		      if (TREE_CODE (t) == INTEGER_CST
-			  && tree_int_cst_sgn (t) != 1
-			  && t != integer_minus_one_node)
+		      if (!tree_fits_shwi_p (t)
+			  || tree_to_shwi (t) < 0)
 			{
-			  warning_at (OMP_CLAUSE_LOCATION (c), 0,
-				      "%<tile%> value must be positive");
-			  t = integer_one_node;
+			  error_at (OMP_CLAUSE_LOCATION (c),
+				    "%<tile%> argument needs positive "
+				    "integral constant");
+			  remove = true;
 			}
 		    }
 		  t = fold_build_cleanup_point_expr (TREE_TYPE (t), t);
@@ -8000,11 +8003,19 @@  finish_omp_for (location_t locus, enum tree_code c
   gcc_assert (TREE_VEC_LENGTH (declv) == TREE_VEC_LENGTH (incrv));
   if (TREE_VEC_LENGTH (declv) > 1)
     {
-      tree c = find_omp_clause (clauses, OMP_CLAUSE_COLLAPSE);
+      tree c;
+
+      c = find_omp_clause (clauses, OMP_CLAUSE_TILE);
       if (c)
-	collapse = tree_to_shwi (OMP_CLAUSE_COLLAPSE_EXPR (c));
-      if (collapse != TREE_VEC_LENGTH (declv))
-	ordered = TREE_VEC_LENGTH (declv);
+	collapse = list_length (OMP_CLAUSE_TILE_LIST (c));
+      else
+	{
+	  c = find_omp_clause (clauses, OMP_CLAUSE_COLLAPSE);
+	  if (c)
+	    collapse = tree_to_shwi (OMP_CLAUSE_COLLAPSE_EXPR (c));
+	  if (collapse != TREE_VEC_LENGTH (declv))
+	    ordered = TREE_VEC_LENGTH (declv);
+	}
     }
   for (i = 0; i < TREE_VEC_LENGTH (declv); i++)
     {
Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 241809)
+++ cp/parser.c	(working copy)
@@ -30885,30 +30885,33 @@  cp_parser_oacc_clause_tile (cp_parser *parser, loc
   tree c, expr = error_mark_node;
   tree tile = NULL_TREE;
 
+  /* Collapse and tile are mutually exclusive.  (The spec doesn't say
+     so, but the spec authors never considered such a case and have
+     differing opinions on what it might mean, including 'not
+     allowed'.)  */
   check_no_duplicate_clause (list, OMP_CLAUSE_TILE, "tile", clause_loc);
+  check_no_duplicate_clause (list, OMP_CLAUSE_COLLAPSE, "collapse",
+			     clause_loc);
 
   if (!cp_parser_require (parser, CPP_OPEN_PAREN, RT_OPEN_PAREN))
     return list;
 
   do
     {
+      if (tile && !cp_parser_require (parser, CPP_COMMA, RT_COMMA))
+	return list;
+      
       if (cp_lexer_next_token_is (parser->lexer, CPP_MULT)
 	  && (cp_lexer_nth_token_is (parser->lexer, 2, CPP_COMMA)
 	      || cp_lexer_nth_token_is (parser->lexer, 2, CPP_CLOSE_PAREN)))
 	{
 	  cp_lexer_consume_token (parser->lexer);
-	  expr = integer_minus_one_node;
+	  expr = integer_zero_node;
 	}
       else
-	expr = cp_parser_assignment_expression (parser, NULL, false, false);
+	expr = cp_parser_constant_expression (parser);
 
-      if (expr == error_mark_node)
-	return list;
-
       tile = tree_cons (NULL_TREE, expr, tile);
-
-      if (cp_lexer_next_token_is (parser->lexer, CPP_COMMA))
-	cp_lexer_consume_token (parser->lexer);
     }
   while (cp_lexer_next_token_is_not (parser->lexer, CPP_CLOSE_PAREN));
 
@@ -31021,6 +31024,7 @@  cp_parser_omp_clause_collapse (cp_parser *parser,
     }
 
   check_no_duplicate_clause (list, OMP_CLAUSE_COLLAPSE, "collapse", location);
+  check_no_duplicate_clause (list, OMP_CLAUSE_TILE, "tile", location);
   c = build_omp_clause (loc, OMP_CLAUSE_COLLAPSE);
   OMP_CLAUSE_CHAIN (c) = list;
   OMP_CLAUSE_COLLAPSE_EXPR (c) = num;
@@ -34027,10 +34031,16 @@  cp_parser_omp_for_loop (cp_parser *parser, enum tr
   int i, collapse = 1, ordered = 0, count, nbraces = 0;
   vec<tree, va_gc> *for_block = make_tree_vector ();
   auto_vec<tree, 4> orig_inits;
+  bool tiling = false;
 
   for (cl = clauses; cl; cl = OMP_CLAUSE_CHAIN (cl))
     if (OMP_CLAUSE_CODE (cl) == OMP_CLAUSE_COLLAPSE)
       collapse = tree_to_shwi (OMP_CLAUSE_COLLAPSE_EXPR (cl));
+    else if (OMP_CLAUSE_CODE (cl) == OMP_CLAUSE_TILE)
+      {
+	tiling = true;
+	collapse = list_length (OMP_CLAUSE_TILE_LIST (cl));
+      }
     else if (OMP_CLAUSE_CODE (cl) == OMP_CLAUSE_ORDERED
 	     && OMP_CLAUSE_ORDERED_EXPR (cl))
       {
@@ -34060,7 +34070,7 @@  cp_parser_omp_for_loop (cp_parser *parser, enum tr
 	  pc = &OMP_CLAUSE_CHAIN (*pc);
     }
 
-  gcc_assert (collapse >= 1 && ordered >= 0);
+  gcc_assert (tiling || (collapse >= 1 && ordered >= 0));
   count = ordered ? ordered : collapse;
 
   declv = make_tree_vec (count);
@@ -34079,13 +34089,15 @@  cp_parser_omp_for_loop (cp_parser *parser, enum tr
       if (code != CILK_FOR
 	  && !cp_lexer_next_token_is_keyword (parser->lexer, RID_FOR))
 	{
-	  cp_parser_error (parser, "for statement expected");
+	  if (!collapse_err)
+	    cp_parser_error (parser, "for statement expected");
 	  return NULL;
 	}
       if (code == CILK_FOR
 	  && !cp_lexer_next_token_is_keyword (parser->lexer, RID_CILK_FOR))
 	{
-	  cp_parser_error (parser, "_Cilk_for statement expected");
+	  if (!collapse_err)
+	    cp_parser_error (parser, "_Cilk_for statement expected");
 	  return NULL;
 	}
       loc = cp_lexer_consume_token (parser->lexer)->location;
@@ -34245,7 +34257,7 @@  cp_parser_omp_for_loop (cp_parser *parser, enum tr
 	 nested.  Hopefully the final version clarifies this.
 	 For now handle (multiple) {'s and empty statements.  */
       cp_parser_parse_tentatively (parser);
-      do
+      for (;;)
 	{
 	  if (cp_lexer_next_token_is_keyword (parser->lexer, RID_FOR))
 	    break;
@@ -34260,14 +34272,13 @@  cp_parser_omp_for_loop (cp_parser *parser, enum tr
 	  else
 	    {
 	      loc = cp_lexer_peek_token (parser->lexer)->location;
-	      error_at (loc, "not enough collapsed for loops");
+	      error_at (loc, "not enough for loops to collapse");
 	      collapse_err = true;
 	      cp_parser_abort_tentative_parse (parser);
 	      declv = NULL_TREE;
 	      break;
 	    }
 	}
-      while (1);
 
       if (declv)
 	{