Here are a few things.
+ if (g_expr.value && TREE_CODE (g_expr.value) == C_MAYBE_CONST_EXPR)
+ {
+ error_at (input_location, "cannot convert grain to long integer.\n");
+ c_parser_skip_to_pragma_eol (parser);
+ }
Remove final period. Also, where's the testcase? Also, there seems to
be spurious white space after the "}".
Is it required that it be a long integer? Because I see no further
checks for this.
+ c_token *token = c_parser_peek_token (parser);
+ if (token && token->type == CPP_KEYWORD
+ && token->keyword == RID_CILK_FOR)
It doesn't look like c_parser_peek_token() ever returns NULL, so no need
to check for token != 0.
+ tree grain = convert_to_integer (long_integer_type_node,
+ g_expr.value);
+ if (grain && grain != error_mark_node)
+ c_parser_cilk_simd (parser, grain);
No need to check grain != 0 here either.
==> a.c.003t.original <==
;; Function main (null)
;; enabled by -tree-original
{
int i;
int i;
<<< Unknown tree: cilk_for
#pragma omp parallel
{
{
Found with -fdump-tree-all. You should handle the cilk_for tree code in
the pretty printers, and add corresponding test(s).
static void
-c_parser_cilk_simd (c_parser *parser)
+c_parser_cilk_simd (c_parser *parser, tree grain)
No documentation for grain.
+ tree clauses = NULL_TREE;
+
+ if (!is_cilk_for)
+ clauses = c_parser_cilk_all_clauses (parser);
+ else
+ clauses = grain;
First set of clauses=NULL_TREE is useless.
static tree
c_parser_omp_for_loop (location_t loc, c_parser *parser, enum tree_code code,
- tree clauses, tree *cclauses)
+ tree clauses_or_grain, tree *cclauses)
Don't overload clauses and grainsize into one argument. Add another
argument. Also, document said argument.
+/* Returns a FUNCTION_DECL of type TYPE whose name is *NAME. */
+
+static tree
+cilk_declare_looper (const char *name, tree type, enum built_in_function code)
+{
I think you should document that it's creating a suitable built-in, not
just creating a FUNCTION_DECL. Also, plesae document argument `code'.
And call this function something more meaningful, like
"cilkrts_decalre_builtin" or "cilk_declare_for_builtin", but definitely
not looper :).
@@ -1192,6 +1199,9 @@ dump_gimple_omp_for (pretty_printer *buffer, gimple gs,
int spc, int flags)
case GE_EXPR:
pp_greater_equal (buffer);
break;
+ case NE_EXPR:
+ pp_string (buffer, "!=");
+ break;
Thank you :). That was probably my oversight on the pragma simd work.
@@ -6603,6 +6614,11 @@ gimplify_omp_for (tree *expr_p, gimple_seq *pre_p)
}
for_body = NULL;
+ if (flag_enable_cilkplus && TREE_CODE (for_stmt) == CILK_FOR)
+ {
+ tree it = TREE_VEC_ELT (OMP_FOR_INIT (for_stmt), 0);
+ gimplify_and_add (it, &for_pre_body);
+ }
And what Jason said for all the special casing for CILK_FOR in this
function...
+static inline void
+gimple_cilk_for_set_grain (tree grain, gimple gs)
+{
+ const gimple_statement_omp_for *omp_for_stmt =
+ as_a <gimple_statement_omp_for> (gs);
+ omp_for_stmt->iter[0].grain = grain;
+}
Can we leave the grainsize in the clause, or does it have to reside in
an auxiliary data structure? It seems weird as is, since you're only
setting grain for the first element. I think Jason mentioned something
similar.
+/* A structure with necessary elements from _Cilk_for statement. This
+ struct. node is passed in to WALK_STMT_INFO->INFO. */
+typedef struct cilk_for_information {
"{" should be in a separate line.
"for a Cilk_for statement", not "from Cilk_for". No abbreviation on
struct. Either remove the period, or spell it out. Also s/in to/to/.
I'm not a C++ expert, but my understanding was that in C++ you don't
need a typedef to use the following structure by name
(cilk_for_information). So you can just declare "struct
cilk_for_information {...}" and instantiate it with just
"cilk_for_information some_instance". If that's the case, get rid of
typedef.
+ if (flag_enable_cilkplus
BTW, weren't you going to change this to flag_cilkplus or something in
some past follow-up?
fd->sched_kind = OMP_CLAUSE_SCHEDULE_STATIC;
fd->chunk_size = NULL_TREE;
+ if (flag_enable_cilkplus
+ && gimple_omp_for_kind (fd->for_stmt) == GF_OMP_FOR_KIND_CILKFOR)
+ fd->sched_kind = OMP_CLAUSE_SCHEDULE_CILKFOR;
I believe most of the flag_enable_cilkplus checks in omp-low.c can be
removed, especially the ones related to syntax. You shouldn't be
getting any cilk constructs this late if cilkplus was not enabled. For
that matter, we don't check flag_openmp in this file throughout, we only
check for it at the gates.
bool is_cilk_for = (flag_enable_cilkplus && outer_ctx
&& is_cilk_for_stmt (outer_ctx->stmt, &ind_var));
Although here you could probably leave it, since it would avoid
traversing outer_ctx->stmt. And speak of which, since you're checking
flag_enable_cilkplus in the caller, why also check it in
is_cilk_for_stmt? Do it all in the callee IMO.
Also, you should probably combine boths initializations of sched_kind
into the same if:
if (gimple_omp_for_kind (fd->for_stmt) == GF_OMP_FOR_KIND_CILKFOR)
fd->sched_kind = OMP_CLAUSE_SCHEDULE_CILKFOR;
else
fd->sched_kind = OMP_CLAUSE_SCHEDULE_STATIC;
+/* Returns the type of the induction variable for the child function for
+ _Cilk_for and the types for _high and _low variables based on TYPE. */
high, low, type, what? I don't get it. Can you rewrite the
documentation to make it clearer what this does?
+
+static tree
+cilk_for_check_loop_diff_type (tree type)
+{
+ if (type == integer_type_node)
+ return type;
+ else if (TYPE_PRECISION (type) <= TYPE_PRECISION (uint32_type_node))
+ {
+ if (TYPE_UNSIGNED (type))
+ return uint32_type_node;
+ else
+ return integer_type_node;
+ }
+ else
+ {
+ if (TYPE_UNSIGNED (type))
+ return uint64_type_node;
+ else
+ return long_long_integer_type_node;
+ }
+ gcc_unreachable ();
+}
No need for a gcc_unreachable(). You have a final `else'; you'll
clearly never reach the unreachable.
static void
-create_omp_child_function (omp_context *ctx, bool task_copy)
+create_omp_child_function (omp_context *ctx, bool task_copy,
+ bool is_cilk_for, tree cilk_var_type)
{
tree decl, type, name, t;
-
- name = create_omp_child_function_name (task_copy);
+
+ name = create_omp_child_function_name (task_copy, is_cilk_for);
It looks like task_copy and is_cilk_for never coexist throughout.
Perhaps this is a candidate for an enum?
+ /* _Cilk_for's child function requires two extra parameters called
+ __low and __high that are set the by Cilk runtime when it calls this
+ function. */
+ if (is_cilk_for)
+ {
+ t = build_decl (DECL_SOURCE_LOCATION (decl),
+ PARM_DECL, get_identifier ("__high"), cilk_var_type);
Perhaps you should add a similar comment here too:
+ else if (is_cilk_for)
+ type = build_function_type_list (void_type_node, ptr_type_node,
+ cilk_var_type, cilk_var_type, NULL_TREE);
+ /* In _Cilk_for, the increment, start and final values
+ are stored in the clause inserted by gimplify_omp_for.
+ This value is used by the child function to find the
+ appropriate induction value function based on the
+ high and low parameters of the child function.
+ Now, we need to store the decl value expressions here so
+ that we can easily access them. */
+ if (flag_enable_cilkplus
+ && (is_cilk_loop_var (var, "__cilk_init")
+ || is_cilk_loop_var (var, "__cilk_cond")
+ || is_cilk_loop_var (var, "__cilk_incr")))
+ SET_DECL_VALUE_EXPR (var, x);
This looks weird. I'll let Jakub and Jason comment on whether this is
the correct place to put this information. Can't you check somehow if
the loop is a Cilk_for loop without having to look at the variable names
themselves?
+/* Returns true if T is a tree whose code is COMPONENT_REF and its field
+ matches D_F_NAME and the data argument matches D_ARG_NAME. */
+
+static bool
+cilk_find_field_value (tree t, tree d_arg_name, tree d_f_name)
+{
+ if (TREE_CODE (t) == COMPONENT_REF)
+ {
+ tree arg = TREE_OPERAND (t, 0);
+ tree field = TREE_OPERAND (t, 1);
+ if (TREE_CODE (arg) == ADDR_EXPR || TREE_CODE (arg) == MEM_REF)
+ arg = TREE_OPERAND (arg, 0);
+ if (DECL_NAME (arg) && DECL_NAME (field)
+ && !strcmp (IDENTIFIER_POINTER (d_arg_name),
+ IDENTIFIER_POINTER (DECL_NAME (arg)))
+ && !strcmp (IDENTIFIER_POINTER (d_f_name),
+ IDENTIFIER_POINTER (DECL_NAME (field))))
Also weird. Do we really need to look at the identifier pointer itself?
Again, I'll let Jason and/or Jakub comment.
I'll let Jakub comment on the functional parts of the omp-low.c parts,
but I would prefer that a lot of big functions in omp-low.c that only
pertain to Cilk Plus, be moved to a cilk specific file. For example,
expand_cilk_for_body() and helpers.
Aldy
