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:al...@redhat.com] > Sent: Thursday, January 16, 2014 4:19 PM > To: Iyer, Balaji V; Jakub Jelinek > Cc: Jason Merrill; 'Jeff Law'; 'gcc-patches@gcc.gnu.org'; 'r...@redhat.com' > 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