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
