On Fri, Oct 04, 2024 at 03:56:01PM +0100, Kwok Cheung Yeung wrote:
> This patch modifies the C and C++ parsers to accept an iterator as a map
> type modifier, storing it in the OMP_CLAUSE_ITERATOR argument of the clause.
> When finishing clauses, any clauses generated from a clause with iterators
> also has the iterator applied to them.
>
> During gimplification, check_omp_map_iterators is called to check that all
> iterator variables are referenced at some point with a clause.
> Gimplification of the clause decl and size are delayed until iterator
> expansion as they may reference iterator variables.
Any kind of delaying of gimplification feels wrong.
You can arrange for the iterator var to be kept as is, or certain forms of
trees still be allowed through, but arbitrary expressions in there is
definitely wrong.
One could have map(iterator(i=0:1), to: y[foo (bar (i))]) or similar, and
you don't want to gimplify the calls or worse say some FE-ish trees after
gimplification.
> +/* Callback for walk_tree to find a VAR_DECL (stored in DATA) in the
> + tree TP. */
> +
> +static tree
> +find_var_decl (tree *tp, int *, void *data)
Please put omp_ somewhere in the name.
> +{
> + tree t = *tp;
> +
> + if (TREE_CODE (t) == VAR_DECL && t == (tree) data)
TREE_CODE (x) == VAR_DECL should be VAR_P (x), but why
are you testing it when you do t == (tree) data?
That alone should be enough, no?
> + for (tree it = OMP_CLAUSE_ITERATORS (c); it; it = TREE_CHAIN (it))
> + {
> + tree var = TREE_VEC_ELT (it, 0);
> + tree t = walk_tree (&OMP_CLAUSE_DECL (c), find_var_decl, var, NULL);
> + if (t == NULL_TREE)
> + t = walk_tree (&OMP_CLAUSE_SIZE (c), find_var_decl, var, NULL);
> + if (t == NULL_TREE)
> + {
> + error_at (OMP_CLAUSE_LOCATION (c),
> + "iterator variable %qD not used in clause expression",
> + var);
Where do you see in OpenMP standard a restriction that iterator variable has
to be used in the clause expression?
Sure, the iterators without it are kind of pointless, but unless there is
something in the standard that says it is invalid, we shouldn't reject it.
E.g. one can use iterator which has a single iteration, then why would one
use the iterator in the expression (sure, why would one use the iterator in
that case), or zero iterations.
> @@ -14168,7 +14217,11 @@ gimplify_adjust_omp_clauses (gimple_seq *pre_p,
> gimple_seq body, tree *list_p,
> : TYPE_SIZE_UNIT (TREE_TYPE (decl));
> }
> gimplify_omp_ctxp = ctx->outer_context;
> - if (gimplify_expr (&OMP_CLAUSE_SIZE (c), pre_p, NULL,
> + if (OMP_CLAUSE_ITERATORS (c))
> + /* Gimplify the OMP_CLAUSE_SIZE later, when the iterator is
> + gimplified. */
> + ;
See above. At least partial gimplification is a must IMHO.
> + else if (gimplify_expr (&OMP_CLAUSE_SIZE (c), pre_p, NULL,
> is_gimple_val, fb_rvalue) == GS_ERROR)
> {
> gimplify_omp_ctxp = ctx;
> @@ -14333,6 +14386,11 @@ gimplify_adjust_omp_clauses (gimple_seq *pre_p,
> gimple_seq body, tree *list_p,
> if (code == OMP_TARGET && OMP_CLAUSE_MAP_IN_REDUCTION (c))
> break;
>
> + /* Do not gimplify the declaration yet for clauses with
> + iterators. */
> + if (OMP_CLAUSE_ITERATORS (c))
> + break;
Likewise.
> --- a/gcc/omp-low.cc
> +++ b/gcc/omp-low.cc
> @@ -12607,6 +12607,163 @@ lower_omp_taskreg (gimple_stmt_iterator *gsi_p,
> omp_context *ctx)
> }
> }
>
> +extern tree compute_iterator_count (tree it, gimple_seq *pre_p);
> +extern tree *build_iterator_loop (tree it, gimple_seq *pre_p, tree
> *last_bind);
Such declarations belong to some header file.
Jakub
