Hi Aldy,
I have answered your questions below. But, I am attaching the patch
with the response to Jason Merrill's email. I am not attaching them here to
remove unnecessary duplication. I hope that's OK with you. If you would like it
otherwise please let me know and I can send them to you.
Thanks,
Balaji V. Iyer.
> -----Original Message-----
> From: Aldy Hernandez [mailto:[email protected]]
> Sent: Thursday, January 16, 2014 4:19 PM
> To: Iyer, Balaji V; Jakub Jelinek
> Cc: Jason Merrill; 'Jeff Law'; '[email protected]'; '[email protected]'
> Subject: Re: [PATCH] _Cilk_for for C and C++
>
> 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 "}".
>
Was not necessary. Fixed.
> Is it required that it be a long integer? Because I see no further checks for
> this.
>
Yes, I believe the language spec says the grainsize should be convertible to a
long int.
> > + 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.
Fixed.
>
> > + 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.
>
Fixed.
> > ==> 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).
>
I didn't fix this yet. I didn't understand how to add tests to check
pretty-printers...
>
> > static void
> > -c_parser_cilk_simd (c_parser *parser)
> > +c_parser_cilk_simd (c_parser *parser, tree grain)
>
> No documentation for grain.
>
Fixed.
> > + 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.
>
OK. Just my common practice of initialize all the variables when it is defined.
I have removed it.
> > 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.
>
OK. I have added a new parameter called grain and renamed the clauses_or_grain
to clauses.
> > +/* 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 :).
>
Renamed to declare_cilk_for_builtin.
> > @@ -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.
>
Your welcome.
> > @@ -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...
>
This removed as mentioned in email to Jason.
> > +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.
>
Got rid of the grainsize functions and just kept it as a clause in _Cilk_for
(as suggested by Jason).
> > +/* 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.
>
Fixed.
> "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?
>
Yup. I was going to do it after I finish getting _Cilk_for into GCC.
> > 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.
>
Yes, but it is kept as a safety measure. For some reason some one overwrites a
bit or something weird happens (like someone setting the #define wrong or
something similar), then the Cilk Plus flags will add another, albeit small,
layer of protection.
> > 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.
>
Removed from is_cilk_for_stmt.
> 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;
>
These both can't be same. _Cilk_for does not rely on OMP's scheduling.
> > +/* 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?
>
Fixed.
> > +
> > +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.
Removed.
>
> > 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?
I can do that, but doesn't enum store the same size as an int and so isn't 2
bools better space-wise?
>
> > + /* _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:
>
Fixed. Pointed to the note.
> > + 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?
>
That's not what I am doing here. I am setting the value chain expression for
the init, cond and incr temporary variables that I have created.
> > +/* 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
