On Fri, Jun 18, 2021 at 10:25:16PM +0800, Chung-Lin Tang wrote:
Note, you'll need to rebase your patch, it clashes with
r12-1768-g7619d33471c10fe3d149dcbb701d99ed3dd23528.
Sorry for that. And sorry for patch review delay.
> --- a/gcc/c/c-typeck.c
> +++ b/gcc/c/c-typeck.c
> @@ -13104,6 +13104,12 @@ handle_omp_array_sections_1 (tree c, tree t,
> vec<tree> &types,
> return error_mark_node;
> }
> t = TREE_OPERAND (t, 0);
> + if ((ort == C_ORT_ACC || ort == C_ORT_OMP)
Map clauses never appear on declare simd, so
(ort == C_ORT_ACC || ort == C_ORT_OMP)
previously meant always and since the in_reduction change is incorrect
(as C_ORT_OMP_TARGET is used for target construct but not for
e.g. target data* or target update).
> + && TREE_CODE (t) == MEM_REF)
So please just use if (TREE_CODE (t) == MEM_REF)
or explain when it shouldn't trigger.
> @@ -14736,6 +14743,11 @@ c_finish_omp_clauses (tree clauses, enum
> c_omp_region_type ort)
> {
> while (TREE_CODE (t) == COMPONENT_REF)
> t = TREE_OPERAND (t, 0);
> + if (TREE_CODE (t) == MEM_REF)
> + {
> + t = TREE_OPERAND (t, 0);
> + STRIP_NOPS (t);
> + }
This doesn't look correct. At least the parsing (and the spec AFAIK)
doesn't ensure that if there is ->, it must come before all the dots.
So, if one uses map (s->x.y) the above would work, but if map (s->x.y->z) or
map (s.a->b->c->d->e) is used, it wouldn't. I'd expect a single
while loop that looks through COMPONENT_REFs and MEM_REFs as they appear.
Maybe the handle_omp_array_sections_1 MEM_REF case too?
Or do you want to have it done incrementally, start with supporting only
a single -> first before all the dots and later on add support for the rest?
I think the 5.0 and especially 5.1 wording basically says that map clause
operand is arbitrary lvalue expression that includes array section support
too, so eventually we should just have somewhere in parsing scope a bool
whether OpenMP array sections are allowed or not, add OMP_ARRAY_REF or
similar tree code for those and after parsing the expression, ensure
array sections appear only where they can appear and for a subset of the
lvalue expressions where we have decl plus series of -> field or . field
or [ index ] or [ array section stuff ] handle those specially.
That arbitrary lvalue can certainly be done incrementally.
map (foo(123)->a.b[3]->c.d[:7]) and the like.
> if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
> && OMP_CLAUSE_MAP_IMPLICIT (c)
> && (bitmap_bit_p (&map_head, DECL_UID (t))
> @@ -14802,6 +14814,15 @@ c_finish_omp_clauses (tree clauses, enum
> c_omp_region_type ort)
> bias) to zero here, so it is not set erroneously to the pointer
> size later on in gimplify.c. */
> OMP_CLAUSE_SIZE (c) = size_zero_node;
> + indir_component_ref_p = false;
> + if ((ort == C_ORT_ACC || ort == C_ORT_OMP)
Same comment about ort tests.
> + && TREE_CODE (t) == COMPONENT_REF
> + && TREE_CODE (TREE_OPERAND (t, 0)) == MEM_REF)
> + {
> + t = TREE_OPERAND (TREE_OPERAND (t, 0), 0);
> + indir_component_ref_p = true;
> + STRIP_NOPS (t);
> + }
Again, this can handle only a single ->
> @@ -42330,16 +42328,10 @@ cp_parser_omp_target (cp_parser *parser, cp_token
> *pragma_tok,
> cclauses[C_OMP_CLAUSE_SPLIT_TARGET] = tc;
> }
> }
> - tree stmt = make_node (OMP_TARGET);
> - TREE_TYPE (stmt) = void_type_node;
> - OMP_TARGET_CLAUSES (stmt) = cclauses[C_OMP_CLAUSE_SPLIT_TARGET];
> - c_omp_adjust_map_clauses (OMP_TARGET_CLAUSES (stmt), true);
> - OMP_TARGET_BODY (stmt) = body;
> - OMP_TARGET_COMBINED (stmt) = 1;
> - SET_EXPR_LOCATION (stmt, pragma_tok->location);
> - add_stmt (stmt);
> - pc = &OMP_TARGET_CLAUSES (stmt);
> - goto check_clauses;
> + c_omp_adjust_map_clauses (cclauses[C_OMP_CLAUSE_SPLIT_TARGET], true);
> + finish_omp_target (pragma_tok->location,
> + cclauses[C_OMP_CLAUSE_SPLIT_TARGET], body, true);
What is the advantage of finish_omp_target. Perhaps the check_clauses label
can be renamed and more things common to both paths moved after the label if
needed, but as long as it isn't something also called during instantiation,
I find it cleaner to do it in cp_parser_omp_target at one place.
The reason for e.g. finish_omp_parallel is that it is called from both
parsing and instantiation.
> --- a/gcc/cp/semantics.c
> +++ b/gcc/cp/semantics.c
> @@ -4990,6 +4990,9 @@ handle_omp_array_sections_1 (tree c, tree t, vec<tree>
> &types,
> {
> if (error_operand_p (t))
> return error_mark_node;
> + if ((ort == C_ORT_ACC || ort == C_ORT_OMP)
See above about ort.
Declare simd only allows uniform, linear, aligned, simdlen, inbranch and
notinbranch clauses and none of those support array sections.
> + && TREE_CODE (t) == FIELD_DECL)
> + t = finish_non_static_data_member (t, NULL_TREE, NULL_TREE);
handle_omp_array_sections_1 already has recent:
if (TREE_CODE (t) == FIELD_DECL
&& (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_AFFINITY
|| OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND))
ret = finish_non_static_data_member (t, NULL_TREE, NULL_TREE);
so shouldn't that be extended to map/to/from clauses too?
And I guess we should check reduction/in_reduction/task_reduction clauses
too.
> @@ -9003,6 +9037,493 @@ finish_omp_construct (enum tree_code code, tree body,
> tree clauses)
> return add_stmt (stmt);
> }
>
> +/* Used to walk OpenMP target directive body. */
> +
> +struct omp_target_walk_data
> +{
> + tree current_object;
> + bool this_expr_accessed;
> +
> + hash_map<tree, tree> ptr_members_accessed;
> + hash_set<tree> lambda_objects_accessed;
> +
> + tree current_closure;
> + hash_set<tree> closure_vars_accessed;
> +
> + hash_set<tree> local_decls;
> +};
> +
> +static tree
> +finish_omp_target_clauses_r (tree *tp, int *walk_subtrees, void *ptr)
> +{
> + tree t = *tp;
> + struct omp_target_walk_data *data = (struct omp_target_walk_data *) ptr;
> + tree current_object = data->current_object;
> + tree current_closure = data->current_closure;
This is something that we'll eventually need to do e.g. for declare mapper
in all the 3 FEs, gather what variables might need to be mapped and
for all their types look up the mappers (recursively for nested types and
for all types mentioned in those declare mappers etc.) and remember that
somehow until gimplification.
If it is only preliminary and covers might appear rather than appears,
I think it should be fine. What this routine does is ultimate, if you see
this somewhere, you say it is accessed, if you see a lambda, again, it has
to be accessed etc. I'm afraid that is unsafe though.
The IL at this point isn't folded yet, one could have sizeof (this) or other
unevaluated context appear there, or something could appear in a private clause
on some inner construct that doesn't imply an access on the outer target, etc.
So, I think either this function would need to be more careful, especially
for nested OpenMP constructs, or can't it be done through langhooks at
gimplification time when we should know exactly what appears and what
doesn't in the body?
> + if (TREE_TYPE(t) && LAMBDA_TYPE_P (TREE_TYPE (t)))
Formatting, missing space before (.
> + for (hash_set<tree>::iterator i = data.closure_vars_accessed.begin ();
> + i != data.closure_vars_accessed.end (); ++i)
> + {
> + tree orig_decl = *i;
> + tree closure_expr = DECL_VALUE_EXPR (orig_decl);
> +
> + if (TREE_CODE (TREE_TYPE (orig_decl)) == POINTER_TYPE)
> + {
> + /* this-pointer is processed outside this loop. */
> + if (operand_equal_p (closure_expr, omp_target_this_expr))
> + continue;
> +
> + tree c = build_omp_clause (loc, OMP_CLAUSE_MAP);
> + OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_ALLOC);
> + OMP_CLAUSE_DECL (c)
> + = build_indirect_ref (loc, closure_expr, RO_UNARY_STAR);
> + OMP_CLAUSE_SIZE (c) = size_zero_node;
> + OMP_CLAUSE_MAP_MAYBE_ZERO_LENGTH_ARRAY_SECTION (c) = 1;
> + new_clauses.safe_push (c);
> +
> + c = build_omp_clause (loc, OMP_CLAUSE_MAP);
> + OMP_CLAUSE_SET_MAP_KIND
> + (c, GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION);
> + OMP_CLAUSE_DECL (c) = closure_expr;
> + OMP_CLAUSE_SIZE (c) = size_zero_node;
> + new_clauses.safe_push (c);
> + }
> + else if (TREE_CODE (TREE_TYPE (orig_decl)) == REFERENCE_TYPE)
> + {
> + tree c = build_omp_clause (loc, OMP_CLAUSE_MAP);
> + OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_TO);
> + OMP_CLAUSE_DECL (c)
> + = build1 (INDIRECT_REF,
> + TREE_TYPE (TREE_TYPE (closure_expr)),
> + closure_expr);
> + OMP_CLAUSE_SIZE (c)
> + = TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (closure_expr)));
> + new_clauses.safe_push (c);
> +
> + c = build_omp_clause (loc, OMP_CLAUSE_MAP);
> + OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_ALWAYS_POINTER);
> + OMP_CLAUSE_DECL (c) = closure_expr;
> + OMP_CLAUSE_SIZE (c) = size_zero_node;
> + new_clauses.safe_push (c);
Is it guaranteed everything added here can't have an explicit map clause
already?
Jakub
