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: [PATCH] _Cilk_for for C and C++

On 11/25/2013 11:03 PM, Iyer, Balaji V wrote:

On a broad note, I think there's a lot of OpenMP code you could be reusing here rather than writing it all again. And that way Cilk code will benefit from improvements to OpenMP handling, and vice versa. It probably makes sense to turn Cilk_for into an OMP_FOR loop, and then gimplify into GIMPLE_OMP_FOR, rather than create a new tree code and handle everything at the tree level. But I don't know the OMP code well enough to suggest exactly how that would work.

Finer-grained comments:

+  tree exp = build_new_op (EXPR_LOCATION (op1), code, flags, op0, op1,
+			   NULL_TREE, NULL, tf_none);
+  if (exp == error_mark_node)
+    exp = build_x_modify_expr (EXPR_LOCATION (op1), op0, code, op1, tf_none);
+  if (exp && exp != error_mark_node)
+    return exp;

I thought you were changing this?

+/* Handler for iterator to compute the loop variable.  ADD_OP indicates
+   whether we need a '+' or '-' operation. LOW indicates the starting point
+   and LOOP_VAR is the induction variable.  This functin returns an
+   INIT_EXPR.  */

This comment still doesn't document VAR2.


+  tree exp = build_new_op (loc, add_op, 0, low, loop_var, NULL_TREE, 0,
+			   tf_none);
+  gcc_assert (exp != error_mark_node);
+  exp = cp_build_modify_expr (var2, INIT_EXPR, exp, tf_warning_or_error);

Looking at online Cilk documentation I see:

The increment expression must add to or subtract from the control variable using one of the following supported operations:
++ (prefix or postfix)
-- (prefix or postfix)

From this, I think people would expect the increment to use a user-defined operator+=/-=/++/--, but your code above uses operator+/- instead.

+		    "-fcilkplus must be enabled t use %<_Cilk_for%>");


+cp_parser_cilk_for (cp_parser *parser, tree grain)

Please reuse cp_parser_omp_for, like Aldy did for #pragma simd (cp_parser_cilk_simd) rather than write yet another for-statement parser. This should reduce the patch size quite a bit.

+      if (context == pragma_external)
+	{
+	  error_at (pragma_tok->location,
+		    "%<#pragma cilk grainsize%> may only be be used inside a "
+		    "function");
+	  break;
+	}
+      /* Ignore the pragma if Cilk Plus is not enabled.  */
+      if (flag_enable_cilkplus)
+	{
+	  cp_parser_cilk_grainsize (parser, pragma_tok);
+	  return true;
+	}

Do you mean to fall through to the default case if Cilk+ is not enabled?

+	tmp = CILK_FOR_COND (t);
+	  {
+	    tree op0 = RECUR (TREE_OPERAND (tmp, 0));
+	    tree op1 = RECUR (TREE_OPERAND (tmp, 1));
+	    tmp = build2 (TREE_CODE (tmp), boolean_type_node, op0, op1);
+	  }
+	CILK_FOR_COND (stmt) = tmp;

Why not just recur into CILK_FOR_COND?

+	tmp = CILK_FOR_EXPR (t);
+	if (TREE_CODE (tmp) == MODIFY_EXPR)
+	  {
+	    tree lhs = TREE_OPERAND (tmp, 0);
+	    tree rhs = TREE_OPERAND (tmp, 1);
+	    lhs = RECUR (lhs);
+	    rhs = build2 (TREE_CODE (rhs), TREE_TYPE (lhs),
+			  RECUR (TREE_OPERAND (rhs, 0)),
+			  RECUR (TREE_OPERAND (rhs, 1)));
+	    tmp = build2 (MODIFY_EXPR, void_type_node, lhs, rhs);
+	  }
+	else
+	  tmp = build2 (TREE_CODE (tmp), void_type_node,
+			RECUR (TREE_OPERAND (tmp, 0)),
+			RECUR (TREE_OPERAND (tmp, 1)));
+	finish_for_expr (tmp, stmt);



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