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

Reply via email to