Patchwork [cilkplus] cleanup C++ pragma simd implementation

login
register
mail settings
Submitter Aldy Hernandez
Date June 7, 2013, 5:12 p.m.
Message ID <51B21489.7010806@redhat.com>
Download mbox | patch
Permalink /patch/249765/
State New
Headers show

Comments

Aldy Hernandez - June 7, 2013, 5:12 p.m.
The attached patch cleans up the C++ pragma simd implementation to share 
more things with the C front-end, particularly the type checking.  For 
instance, I got rid of all the specialized parsing routines to just use 
generic cp_parser_*expression* and then perform the actual typechecking 
in the C shared routines.

I also reworded some of the tests to match the C FE so we can share the 
tests there too.

There are also some fixes scattered throughout, all in an effort to pass 
all the c-c++-common Cilk Plus pragma simd tests.  Almost there... :).

One bug I found that I don't know how to handle is:

@@ -30315,6 +30319,9 @@ cp_parser_simd_for_init_statement (cp_parser 
*parser, tree *init,
  	  && CLASS_TYPE_P (TREE_TYPE (decl)))
  	{
  	  tree rhs, new_expr;
+	  // ?? FIXME: I don't see any definition for *init in this
+	  // code path. ??
+	  gcc_unreachable ();
  	  cp_parser_parse_definitely (parser);
  	  cp_parser_require (parser, CPP_EQ, RT_EQ);

AFAICT, *init is not initialized in this code path and the caller uses 
it.  Balaji, what's the intent here?

Committing to the aldyh/cilk-in-gomp branch.
commit 51d8e99dd59ec4beb523101a35eedd21f2d439a4
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Fri Jun 7 12:06:12 2013 -0500

    Clean up C++ implementation of pragma simd to use shared C/C++ type
    checking.
    
    Change error messages to match C front-end when possible.
    
    Misc fixes.

Patch

diff --git a/gcc/c-family/c-cilkplus.c b/gcc/c-family/c-cilkplus.c
index 84f1645..c02851e 100644
--- a/gcc/c-family/c-cilkplus.c
+++ b/gcc/c-family/c-cilkplus.c
@@ -207,12 +207,16 @@  c_check_cilk_loop_body (tree body)
 /* Validate a _Cilk_for construct (or a #pragma simd for loop, which
    has the same syntactic restrictions).  Returns TRUE if there were
    no errors, FALSE otherwise.  LOC is the location of the for.  DECL
-   is the controlling variable.  COND is the condition.  INCR is the
-   increment expression.  BODY is the body of the LOOP.  */
+   is the controlling variable.  COND is the condition.  INCRP is a
+   pointer the increment expression (in case, the increment needs to
+   be canonicalized).  BODY is the body of the LOOP.  */
 
 static bool
-c_check_cilk_loop (location_t loc, tree decl, tree cond, tree incr, tree body)
+c_check_cilk_loop (location_t loc, tree decl, tree cond, tree *incrp,
+		   tree body)
 {
+  tree incr = *incrp;
+
   if (decl == error_mark_node
       || cond == error_mark_node 
       || incr == error_mark_node
@@ -284,10 +288,11 @@  c_check_cilk_loop (location_t loc, tree decl, tree cond, tree incr, tree body)
       return false;
     }
 
-  /* Validate the increment.  */
+  /* Validate and canonicalize the increment.  */
   incr = c_check_cilk_loop_incr (loc, decl, incr);
   if (incr == error_mark_node)
     return false;
+  *incrp = incr;
 
   if (!c_check_cilk_loop_body (body))
     return false;
@@ -325,7 +330,7 @@  c_finish_cilk_simd_loop (location_t loc,
 {
   location_t rhs_loc;
 
-  if (!c_check_cilk_loop (loc, decl, cond, incr, body))
+  if (!c_check_cilk_loop (loc, decl, cond, &incr, body))
     return NULL;
 
   /* In the case of "for (int i = 0...)", init will be a decl.  It should
@@ -345,7 +350,11 @@  c_finish_cilk_simd_loop (location_t loc,
       init = build_modify_expr (loc, decl, NULL_TREE, NOP_EXPR, rhs_loc,
 				init, NULL_TREE);
     }
-  gcc_assert (TREE_CODE (init) == MODIFY_EXPR);
+
+  // The C++ parser just gives us the rhs.
+  if (TREE_CODE (init) != MODIFY_EXPR)
+    init = build2 (MODIFY_EXPR, void_type_node, decl, init);
+
   gcc_assert (TREE_OPERAND (init, 0) == decl);
 
   tree initv = make_tree_vec (1);
diff --git a/gcc/cp/ChangeLog.cilkplus b/gcc/cp/ChangeLog.cilkplus
index 7254b81..e7f7596 100644
--- a/gcc/cp/ChangeLog.cilkplus
+++ b/gcc/cp/ChangeLog.cilkplus
@@ -1,9 +1,12 @@ 
 2013-05-21  Balaji V. Iyer  <balaji.v.iyer@intel.com>
+	    Aldy Hernandez  <aldyh@redhat.com>
 
 	* cp-tree.h (p_simd_valid_stmts_in_body_p): New prototype.
+	(finish_cilk_for_cond): Likewise.
 	* parser.h (IN_CILK_P_SIMD_FOR): New #define.
 	* Make-lang.in (CXX_AND_OBJCXX_OBJS): Added new obj-file cp-cilkplus.o
 	* cp-cilkplus.c: New file.
+	* semantics.c (finish_cilk_for_cond): New.
 	* parser.c (cp_parser_pragma): Added a PRAGMA_CILK_SIMD case.
 	(cp_parser_cilk_simd_vectorlength): New function.
 	(cp_parser_cilk_simd_linear): Likewise.
diff --git a/gcc/cp/cp-cilkplus.c b/gcc/cp/cp-cilkplus.c
index f387e2f..e6bdf8a 100644
--- a/gcc/cp/cp-cilkplus.c
+++ b/gcc/cp/cp-cilkplus.c
@@ -46,9 +46,6 @@  cpp_validate_cilk_plus_loop_aux (tree *tp, int *walk_subtrees, void *data)
   if (!tp || !*tp)
     return NULL_TREE;
 
-  // Call generic C version.
-  (void) c_validate_cilk_plus_loop (tp, walk_subtrees, data);
-  
   if (TREE_CODE (*tp) == THROW_EXPR)
     {
       error_at (loc, "throw expressions are not allowed inside loops "
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 97e4e3c..ba088a5 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -5790,6 +5790,7 @@  extern void finish_omp_taskyield		(void);
 extern void finish_omp_taskgroup		(tree);
 extern void finish_omp_cancel			(tree);
 extern void finish_omp_cancellation_point	(tree);
+extern tree finish_cilk_for_cond		(tree);
 extern tree begin_transaction_stmt		(location_t, tree *, int);
 extern void finish_transaction_stmt		(tree, tree, int, tree);
 extern tree build_transaction_expr		(location_t, tree, int, tree);
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 68a277b..c93ea9e 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -29899,7 +29899,11 @@  cp_parser_pragma (cp_parser *parser, enum pragma_context context)
 
     case PRAGMA_CILK_SIMD:
       if (context == pragma_external)
-	goto bad_stmt;
+	{
+	  error_at (pragma_tok->location,
+		    "%<#pragma simd%> must be inside a function");
+	  break;
+	}
       cp_parser_cilk_simd_construct (parser, pragma_tok);
       return true;
 
@@ -30198,7 +30202,7 @@  cp_parser_cilk_simd_construct (cp_parser *parser, cp_token *pragma_token)
   if (cp_lexer_next_token_is_not_keyword (parser->lexer, RID_FOR))
     {
       error_at (cp_lexer_peek_token (parser->lexer)->location,
-		"expected for-statement after %<#pragma simd%> clauses");
+		"for statement expected");
       return;
     }
 
@@ -30229,7 +30233,7 @@  cp_parser_simd_for_init_statement (cp_parser *parser, tree *init,
   tree this_pre_body = push_stmt_list ();
   if (token->type == CPP_SEMICOLON)
     {
-      error_at (loc, "for-loop initializer must declare variable");
+      error_at (loc, "expected iteration declaration");
       return error_mark_node;
     }
   cp_parser_parse_tentatively (parser);
@@ -30315,6 +30319,9 @@  cp_parser_simd_for_init_statement (cp_parser *parser, tree *init,
 	  && CLASS_TYPE_P (TREE_TYPE (decl)))
 	{
 	  tree rhs, new_expr;
+	  // ?? FIXME: I don't see any definition for *init in this
+	  // code path. ??
+	  gcc_unreachable ();
 	  cp_parser_parse_definitely (parser);
 	  cp_parser_require (parser, CPP_EQ, RT_EQ);
 	  rhs = cp_parser_assignment_expression (parser, false, NULL);
@@ -30338,151 +30345,6 @@  cp_parser_simd_for_init_statement (cp_parser *parser, tree *init,
   return decl;
 }
   
-
-/* Parses the increment expresion for a cilk_for or for statement with
-   #pragma simd.  */
-
-static tree
-cp_parser_cilk_for_expression_iterator (cp_parser *parser)
-{
-  cp_token *token = cp_lexer_peek_token (parser->lexer);
-  tree name = NULL_TREE, expr = NULL_TREE;
-  enum tree_code t_code = NOP_EXPR;
-
-  if (token->type == CPP_SEMICOLON)
-    {
-      error_at (token->location, "missing loop expression");
-      return error_mark_node;
-    }
-  if (token->type == CPP_PLUS_PLUS || token->type == CPP_MINUS_MINUS)
-    {
-      cp_lexer_consume_token (parser->lexer);
-      token = cp_lexer_peek_token (parser->lexer);
-      t_code = token->type == CPP_PLUS_PLUS ? PREINCREMENT_EXPR
-	: PREDECREMENT_EXPR;
-    }
-
-  if (token->type != CPP_NAME)
-    {
-      error_at (token->location, "invalid loop expression");
-      cp_parser_skip_to_end_of_statement (parser);
-      return error_mark_node;
-    }
-
-  name = cp_parser_lookup_name (parser, token->u.value, none_type, false, false,
-				false, NULL, token->location);
-  if (name == error_mark_node)
-    return error_mark_node;
-
-  /* If name is not a declaration, then the loop is not valid.  */
-  if (!DECL_P (name))
-    {
-      error_at (token->location, "invalid loop increment expression");
-      return error_mark_node;
-    }
-  cp_lexer_consume_token (parser->lexer);
-  token = cp_lexer_peek_token (parser->lexer);
-
-  if (t_code != NOP_EXPR)
-    {
-      if (token->type != CPP_CLOSE_PAREN)
-	{
-	  error_at (token->location, "invalid loop expression");
-	  return error_mark_node;
-	}
-      return build2 (t_code, void_type_node, name, NULL_TREE);
-    }
-
-  if (token->type == CPP_CLOSE_PAREN)
-    {
-      error_at (token->location,
-		"loop expression must modify control variable");
-      return error_mark_node;
-    }
-
-  cp_lexer_consume_token (parser->lexer);
-  if (token->type == CPP_PLUS_PLUS || token->type == CPP_MINUS_MINUS)
-    return build2 (token->type == CPP_PLUS_PLUS ? POSTINCREMENT_EXPR
-		   : POSTDECREMENT_EXPR, void_type_node, name, NULL_TREE);
-  else if (token->type == CPP_EQ)
-    {
-      sorry ("loop with = operator");
-      return error_mark_node;
-    }
-  else if (token->type == CPP_PLUS_EQ || token->type == CPP_MINUS_EQ)
-    t_code = token->type == CPP_PLUS_EQ  ? PLUS_EXPR : MINUS_EXPR;
-  else if (token->type == CPP_MOD_EQ || token->type == CPP_XOR_EQ
-	   || token->type == CPP_DIV_EQ || token->type == CPP_AND_EQ
-	   || token->type == CPP_OR_EQ || token->type == CPP_AND_EQ
-	   || token->type == CPP_LSHIFT_EQ || token->type == CPP_RSHIFT_EQ)
-    {
-      error_at (token->location, "invalid loop increment operation");
-      return error_mark_node;
-    }
-  else
-    {
-      error_at (token->location, "invalid loop expression");
-      return error_mark_node;
-    }
-  expr = cp_parser_binary_expression (parser, false, false, PREC_NOT_OPERATOR,
-				      NULL);
-  if (expr == error_mark_node)
-    return expr;
-
-  return build2 (MODIFY_EXPR, void_type_node, name,
-		 build2 (t_code, TREE_TYPE (name), name, expr));
-}
-
-/* Parses the condition for a for-loop with pragma simd or _Cilk_for
-   loop.  */
-
-static tree
-cp_parser_cilk_for_condition (cp_parser *parser)
-{
-  tree lhs, rhs;
-  enum tree_code code = ERROR_MARK;
-
-  lhs = cp_parser_binary_expression (parser, false, false,
-				     PREC_SHIFT_EXPRESSION, NULL);
-  switch (cp_lexer_peek_token (parser->lexer)->type)
-    {
-    case CPP_NOT_EQ:
-      code = NE_EXPR;
-      break;    
-    case CPP_LESS:
-      code = LT_EXPR;
-      break;
-    case CPP_LESS_EQ:
-      code = LE_EXPR;
-      break;
-    case CPP_GREATER_EQ:
-      code = GE_EXPR;
-      break;
-    case CPP_GREATER:
-      code = GT_EXPR;
-      break;
-    case CPP_EQ_EQ:
-      error_at (cp_lexer_peek_token (parser->lexer)->location,
-		"equality test not permitted in the Cilk_for loop");
-      break;
-    default:
-      error_at (cp_lexer_peek_token (parser->lexer)->location,
-		"missing comparison operator in the loop condition");
-    }
-  cp_lexer_consume_token (parser->lexer);
-
-  rhs = cp_parser_binary_expression (parser, false, false,
-				     PREC_SHIFT_EXPRESSION, NULL);
-  parser->scope = NULL_TREE;
-  parser->qualifying_scope = NULL_TREE;
-  parser->object_scope = NULL_TREE;
-
-  if (code == ERROR_MARK || lhs == error_mark_node || rhs == error_mark_node)
-    return error_mark_node;
-
-  return build2 (code, boolean_type_node, lhs, rhs);
-}
-   
 /* Top-level function to parse _Cilk_for and for statements.  */
 
 static tree
@@ -30552,12 +30414,14 @@  cp_parser_cilk_for (cp_parser *parser, enum rid for_keyword, tree clauses)
     return error_mark_node;
   if (cp_lexer_next_token_is (parser->lexer, CPP_SEMICOLON))
     {
-      error_at (loc, "%s-loop requires a condition",
-		for_keyword == RID_FOR ? "for" : "_Cilk_for");
+      error_at (loc, "missing condition");
       cond = error_mark_node;
     }
   else
-    cond = cp_parser_cilk_for_condition (parser);
+    {
+      cond = cp_parser_condition (parser);
+      cond = finish_cilk_for_cond (cond);
+    }
 
   if (cond == error_mark_node)
     valid = false;
@@ -30565,12 +30429,11 @@  cp_parser_cilk_for (cp_parser *parser, enum rid for_keyword, tree clauses)
   
   if (cp_lexer_next_token_is (parser->lexer, CPP_CLOSE_PAREN))
     {
-      error_at (loc, "%s-loop requires an increment expression",
-		for_keyword == RID_FOR ? "for" : "_Cilk_for");
+      error_at (loc, "missing increment");
       incr_expr = error_mark_node;
     }
   else
-    incr_expr = cp_parser_cilk_for_expression_iterator (parser);
+    incr_expr = cp_parser_expression (parser, false, NULL);
   
   if (incr_expr == error_mark_node)
     {
@@ -30592,10 +30455,8 @@  cp_parser_cilk_for (cp_parser *parser, enum rid for_keyword, tree clauses)
 
   if (for_keyword == RID_FOR)
     {
-      tree initv, incrv, condv, declv, omp_simd_node, body = NULL_TREE;
-
       parser->in_statement = IN_CILK_P_SIMD_FOR;
-      body = push_stmt_list ();
+      tree body = push_stmt_list ();
       cp_parser_statement (parser, NULL_TREE, false, NULL);
       body = pop_stmt_list (body);
 
@@ -30604,23 +30465,16 @@  cp_parser_cilk_for (cp_parser *parser, enum rid for_keyword, tree clauses)
 	 nodes, just return an error mark node.  */
       if (!cpp_validate_cilk_plus_loop (body))
 	return error_mark_node;
-      
-      /* Now pass all the information into finish_omp_for.  */
-      initv = make_tree_vec (1);
-      condv = make_tree_vec (1);
-      incrv = make_tree_vec (1);
-      declv = make_tree_vec (1);
-      TREE_VEC_ELT (initv, 0) = init;
-      TREE_VEC_ELT (condv, 0) = cond;
-      TREE_VEC_ELT (incrv, 0) = incr_expr;
-      TREE_VEC_ELT (declv, 0) = decl;
-      omp_simd_node = finish_omp_for (loc, OMP_SIMD, declv, initv, condv,
-				      incrv, body, pre_body, clauses);
-      return omp_simd_node;
+
+      return c_finish_cilk_simd_loop (loc, decl, init, cond, incr_expr,
+				      body, clauses);
     }
   else
-    /* Fix this when _Cilk_for is added into the mix.  */
-    return NULL_TREE;
+    {
+      /* Handle _Cilk_for here when implemented.  */
+      gcc_unreachable ();
+      return NULL_TREE;
+    }
 }
 
 #include "gt-cp-parser.h"
diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index 9c5dc74..16e2472 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -6054,6 +6054,13 @@  finish_omp_cancellation_point (tree clauses)
   finish_expr_stmt (stmt);
 }
 
+/* Perform any canonicalization of the conditional in a Cilk for loop.  */
+tree
+finish_cilk_for_cond (tree cond)
+{
+  return maybe_convert_cond (cond);
+}
+
 /* Begin a __transaction_atomic or __transaction_relaxed statement.
    If PCOMPOUND is non-null, this is for a function-transaction-block, and we
    should create an extra compound stmt.  */
diff --git a/gcc/testsuite/c-c++-common/cilk-plus/PS/for4.c b/gcc/testsuite/c-c++-common/cilk-plus/PS/for4.c
new file mode 100644
index 0000000..2da8235
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/cilk-plus/PS/for4.c
@@ -0,0 +1,14 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O3 -fcilkplus" } */
+
+int *a, *c;
+
+void foo()
+{
+  int i, j;
+
+  // Pointers are OK.
+  #pragma simd
+  for (int *i=c; i < c; ++i)
+    *a = '5';
+}
diff --git a/gcc/testsuite/c-c++-common/cilk-plus/PS/for5.c b/gcc/testsuite/c-c++-common/cilk-plus/PS/for5.c
new file mode 100644
index 0000000..7075a44
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/cilk-plus/PS/for5.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O3 -fcilkplus" } */
+
+int *a, *b;
+
+void foo()
+{
+#pragma simd
+  for (int i=100; i; --i)
+    a[i] = b[i];
+}