This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: PING: Fwd: Re: [patch] implement Cilk Plus simd loops on trunk

On 10/18/2013 02:48 PM, Aldy Hernandez wrote:
This has the potential of throwing my mind for a spin.  Can I do this as
a followup, and keep it simple for now?


+          else if (!TREE_TYPE (e) || !TREE_CONSTANT (e)
+               || !INTEGRAL_TYPE_P (TREE_TYPE (e)))
+        cp_parser_error (parser,
+                 "step size must be an integer constant");

Can't the step size be a value-dependent expression like a template
non-type parameter?

Fixed.  I also found some nuances in how we were (NOT) handling
variables as the standard mentioned, both for C and C++.  Fixed as well,
and added testcases when appropriate.

+	      else if (TREE_TYPE (e)
+		       && INTEGRAL_TYPE_P (TREE_TYPE (e))
+		       && (TREE_CONSTANT (e)
+			   || DECL_P (e)))
+		step_size = e;

This still doesn't seem like it allows arbitrary dependent constant-expressions. In a template, I'm pretty sure not everything that will end up being a constant-expression is marked TREE_CONSTANT.

You want to check type_dependent_expression_p and value_dependent_expression_p.

+  if (this_pre_body)
+    {
+      this_pre_body = pop_stmt_list (this_pre_body);
+      if (*pre_body)
+	{
+	  tree t = *pre_body;
+	  *pre_body = push_stmt_list ();
+	  add_stmt (t);
+	  add_stmt (this_pre_body);
+	  *pre_body = pop_stmt_list (*pre_body);
+	}
+      else
+	*pre_body = this_pre_body;
+    }

I think you can just use

append_to_statement_list (this_pre_body, pre_body)

+  if (for_keyword == RID_FOR)
+    decl = cp_parser_simd_for_init_statement (parser, &init, &pre_body);
+  if (decl == error_mark_node)

Don't you get a warning about decl possibly being used uninitialized here? I think you want

  /* FIXME when _Cilk_for is implemented.  */
  decl = NULL_TREE;

I guess the assert previously protected you from the uninitialized warning.

+  if (cp_lexer_next_token_is (parser->lexer, CPP_COMMA))
+    {
+      error_at (loc, "%s-loop initializer cannot have multiple variable "
+		"declarations", for_keyword == RID_FOR ? "for" : "_Cilk_for");
+      cp_parser_skip_to_end_of_statement (parser);
+      valid = false;
+    }
+  if (!valid)
+    {
+      /* Skip to the semicolon ending the init.  */
+      cp_parser_skip_to_end_of_statement (parser);
+    }
+  if (!cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON))
+    return error_mark_node;

I think this should move into cp_parser_simd_for_init_statement or cp_parser_omp_for_loop_init.

Out of curiosity, how are OpenMP and Cilk+ for loops so different that they can't share a parsing function?

+/* Adjust any clauses to match the requirements for OpenMP.  */

I guess the idea here is that the clauses start out as Cilk+ clauses? The comment should make that clearer.

-  if (flag_openmp == 0)
+  if (!flag_openmp && !flag_enable_cilkplus)

Seems like there should be an internal flag to enable the common internals.

+/* Cilk Plus - #pragma simd [clause1 ... clauseN]
+   Operands like for OMP_FOR.  */
+DEFTREECODE (CILK_SIMD, "cilk_simd", tcc_statement, 6)

Could this just be a flag on OMP_SIMD?


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]