On Thu, Apr 28, 2016 at 10:42:49AM -0700, Cesar Philippidis wrote: > > That said, the above names are just weird, it is non-obvious > > what they mean at all. What is C_ORT_NONE for? We surely don't > > have any clauses that aren't OpenMP, nor Cilk+, nor OpenACC > > (ok, maybe the simd attribute, but donno if it ever calls the > > *finish_omp_clauses functions). > > *parser_clik_for was just passing is_omp/allow_fields = false.
Sure, because it is Cilk+, not OpenMP. > > @@ -17597,7 +17597,7 @@ c_parser_cilk_for (c_parser *parser, tree grain, bool > *if_p) > tree clauses = build_omp_clause (EXPR_LOCATION (grain), > OMP_CLAUSE_SCHEDULE); > OMP_CLAUSE_SCHEDULE_KIND (clauses) = OMP_CLAUSE_SCHEDULE_CILKFOR; > OMP_CLAUSE_SCHEDULE_CHUNK_EXPR (clauses) = grain; > - clauses = c_finish_omp_clauses (clauses, false); > + clauses = c_finish_omp_clauses (clauses, 0); > > tree block = c_begin_compound_stmt (true); > tree sb = push_stmt_list (); The above is wrong, it should have been C_ORT_CILK. It will not change anything on the behavior of c_finish_omp_clauses - _Cilk_for only has OMP_CLAUSE_SCHEDULE, is_cilk is right now tested only on OMP_CLAUSE_LINEAR - but it is desirable for consistency and clarity. > @@ -17663,7 +17663,7 @@ c_parser_cilk_for (c_parser *parser, tree grain, bool > *if_p) > OMP_CLAUSE_OPERAND (c, 0) > = cilk_for_number_of_iterations (omp_for); > OMP_CLAUSE_CHAIN (c) = clauses; > - OMP_PARALLEL_CLAUSES (omp_par) = c_finish_omp_clauses (c, true); > + OMP_PARALLEL_CLAUSES (omp_par) = c_finish_omp_clauses (c, C_ORT_OMP); > add_stmt (omp_par); This is wrong too, it should be C_ORT_CILK. Again, it shouldn't change anything, the clauses in that case are OMP_CLAUSE_FIRSTPRIVATE, OMP_CLAUSE_PRIVATE and OMP_CLAUSE__CILK_FOR_COUNT_, the latter unique to _Cilk_for, the former not, but with simple decls in them and nothing should depend on that for c_finish_omp_clauses. > -extern tree c_finish_omp_clauses (tree, bool, bool = false, bool = false); > +extern tree c_finish_omp_clauses (tree, unsigned int); I think it would be better to assign an enum value also for the C_ORT_OMP | C_ORT_DECLARE_SIMD (C_ORT_OMP_DECLARE_SIMD), and just use the enum type instead of unsigned int as the type, both in the proto and in c_finish_omp_clauses definition. > @@ -12496,8 +12496,7 @@ c_find_omp_placeholder_r (tree *tp, int *, void *data) > Remove any elements from the list that are invalid. */ > > tree > -c_finish_omp_clauses (tree clauses, bool is_omp, bool declare_simd, > - bool is_cilk) > +c_finish_omp_clauses (tree clauses, unsigned int ort) > { > bitmap_head generic_head, firstprivate_head, lastprivate_head; > bitmap_head aligned_head, map_head, map_field_head; > @@ -12509,6 +12508,9 @@ c_finish_omp_clauses (tree clauses, bool is_omp, bool > declare_simd, > tree *nowait_clause = NULL; > bool ordered_seen = false; > tree schedule_clause = NULL_TREE; > + bool is_omp = ort & C_ORT_OMP; > + bool declare_simd = ort & C_ORT_DECLARE_SIMD; > + bool is_cilk = ort & C_ORT_CILK; I think I'd prefer replacing those flags with the ort & ... tests in all places where they are used. > -extern tree finish_omp_clauses (tree, bool, bool = > false, > - bool = false); > +extern tree finish_omp_clauses (tree, unsigned int); See above. > @@ -32580,9 +32580,9 @@ cp_parser_omp_all_clauses (cp_parser *parser, > omp_clause_mask mask, > if (finish_p) > { > if ((mask & (OMP_CLAUSE_MASK_1 << PRAGMA_OMP_CLAUSE_UNIFORM)) != 0) > - return finish_omp_clauses (clauses, false, true); > + return finish_omp_clauses (clauses, C_ORT_DECLARE_SIMD); This should have been C_ORT_OMP | C_ORT_DECLARE_SIMD or better yet C_ORT_OMP_DECLARE_SIMD, see above. > @@ -37771,7 +37771,7 @@ cp_parser_cilk_for (cp_parser *parser, tree grain, > bool *if_p) > tree clauses = build_omp_clause (EXPR_LOCATION (grain), > OMP_CLAUSE_SCHEDULE); > OMP_CLAUSE_SCHEDULE_KIND (clauses) = OMP_CLAUSE_SCHEDULE_CILKFOR; > OMP_CLAUSE_SCHEDULE_CHUNK_EXPR (clauses) = grain; > - clauses = finish_omp_clauses (clauses, false); > + clauses = finish_omp_clauses (clauses, 0); See above. > --- a/gcc/cp/pt.c > +++ b/gcc/cp/pt.c > @@ -9585,7 +9585,7 @@ tsubst_attribute (tree t, tree *decl_p, tree args, > clauses = tsubst_omp_clauses (clauses, true, false, args, > complain, in_decl); > c_omp_declare_simd_clauses_to_decls (*decl_p, clauses); > - clauses = finish_omp_clauses (clauses, false, true); > + clauses = finish_omp_clauses (clauses, C_ORT_DECLARE_SIMD); > tree parms = DECL_ARGUMENTS (*decl_p); > clauses > = c_omp_declare_simd_clauses_to_numbers (parms, clauses); > @@ -14749,7 +14749,8 @@ tsubst_omp_clauses (tree clauses, bool declare_simd, > bool allow_fields, > new_clauses = nreverse (new_clauses); > if (!declare_simd) > { > - new_clauses = finish_omp_clauses (new_clauses, allow_fields); > + unsigned int ort = allow_fields ? C_ORT_OMP : 0; > + new_clauses = finish_omp_clauses (new_clauses, ort); > if (linear_no_step) > for (nc = new_clauses; nc; nc = OMP_CLAUSE_CHAIN (nc)) > if (nc == linear_no_step) tsubst_omp_clause should have similar enum argument instead of the multiple bools. > @@ -5803,6 +5802,9 @@ finish_omp_clauses (tree clauses, bool allow_fields, > bool declare_simd, > bool branch_seen = false; > bool copyprivate_seen = false; > bool ordered_seen = false; > + bool allow_fields = ort & C_ORT_OMP; > + bool declare_simd = ort & C_ORT_DECLARE_SIMD; > + bool is_cilk = ort & C_ORT_CILK; > > bitmap_obstack_initialize (NULL); > bitmap_initialize (&generic_head, &bitmap_default_obstack); See above. Note that for allow_fields this would be really (ort & C_ORT_OMP_DECLARE_SIMD) == C_ORT_OMP because fields aren't allowed in #pragma omp declare simd > @@ -8342,7 +8344,7 @@ finish_omp_for (location_t locus, enum tree_code code, > tree declv, > OMP_CLAUSE_OPERAND (c, 0) > = cilk_for_number_of_iterations (omp_for); > OMP_CLAUSE_CHAIN (c) = clauses; > - OMP_PARALLEL_CLAUSES (omp_par) = finish_omp_clauses (c, false); > + OMP_PARALLEL_CLAUSES (omp_par) = finish_omp_clauses (c, 0); See above, this should have been C_ORT_CILK too. Jakub