Tejas Belagod <tejas.bela...@arm.com> writes:
> The target clause in OpenMP is used to offload loop kernels to accelarator
> peripeherals. target's 'map' clause is used to move data from and to the
> accelarator. When the data is SVE type, it may not be suitable because of
> various reasons i.e. the two SVE targets may not agree on vector size or
> some targets don't support variable vector size. This makes SVE unsuitable
> for use in OMP's 'map' clause. This patch diagnoses all such cases and issues
> an error where SVE types are not suitable.
>
> Co-authored-by: Andrea Corallo <andrea.cora...@arm.com>
>
> gcc/ChangeLog:
>
> * target.h (type_context_kind): Add new context kinds for target
> clauses.
> (omp_type_context): Query if the context is of OMP kind.
> * config/aarch64/aarch64-sve-builtins.cc (verify_type_context): Diagnose
> SVE types for a given OpenMP context.
> (omp_type_context): New.
> * gimplify.cc (omp_notice_variable): Diagnose implicitly-mapped SVE
> objects in OpenMP regions.
> (gimplify_scan_omp_clauses): Diagnose SVE types for various target
> clauses.
This is a review of the AArch64 bits only. Sorry for not getting to it
earlier.
> ---
> gcc/config/aarch64/aarch64-sve-builtins.cc | 34 ++++++++++++++++-
> gcc/gimplify.cc | 43 +++++++++++++++++++++-
> gcc/target.h | 37 ++++++++++++++++++-
> 3 files changed, 111 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/config/aarch64/aarch64-sve-builtins.cc
> b/gcc/config/aarch64/aarch64-sve-builtins.cc
> index 5d2062726d6..e145689c355 100644
> --- a/gcc/config/aarch64/aarch64-sve-builtins.cc
> +++ b/gcc/config/aarch64/aarch64-sve-builtins.cc
> @@ -5182,7 +5182,11 @@ bool
> verify_type_context (location_t loc, type_context_kind context,
> const_tree type, bool silent_p)
> {
> - if (!sizeless_type_p (type))
> + const_tree tmp = type;
> + if (omp_type_context (context) && POINTER_TYPE_P (type))
> + tmp = strip_pointer_types (tmp);
> +
> + if (!sizeless_type_p (tmp))
> return true;
>
> switch (context)
> @@ -5242,6 +5246,34 @@ verify_type_context (location_t loc, type_context_kind
> context,
> if (!silent_p)
> error_at (loc, "capture by copy of SVE type %qT", type);
> return false;
> +
> + case TCTX_OMP_MAP:
> + if (!silent_p)
> + error_at (loc, "SVE type %qT not allowed in map clause", type);
> + return false;
> +
> + case TCTX_OMP_MAP_IMP_REF:
> + if (!silent_p)
> + error ("cannot reference %qT object types in target region", type);
> + return false;
> +
> + case TCTX_OMP_PRIVATE:
> + if (!silent_p)
> + error_at (loc, "SVE type %qT not allowed in target private clause",
> type);
Nit: long line. Same for the others below.
> + return false;
> +
> + case TCTX_OMP_FIRSTPRIVATE:
> + if (!silent_p)
> + error_at (loc, "SVE type %qT not allowed in target firstprivate
> clause", type);
> + return false;
> +
> + case TCTX_OMP_DEVICE_ADDR:
> + if (!silent_p)
> + error_at (loc, "SVE type %qT not allowed in target device clauses",
> type);
Is the final error message accurate? This TCTX value is used for clauses
like use_device_addr, which AIUI are part of "target data". Maybe something
like "SVE type %qT is not a valid device storage type"?
But TBH I know little about omp, so I hope someone else can suggest
something better.
Otherwise the AArch64 bits look good to me, to the extent that I'm
able to review them.
Thanks,
Richard
> + return false;
> +
> + default:
> + break;
> }
> gcc_unreachable ();
> }
> diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
> index a03ca8cf4ee..47635bb3119 100644
> --- a/gcc/gimplify.cc
> +++ b/gcc/gimplify.cc
> @@ -9081,11 +9081,20 @@ omp_notice_variable (struct gimplify_omp_ctx *ctx,
> tree decl, bool in_code)
> | GOVD_MAP_ALLOC_ONLY)) == flags)
> {
> tree type = TREE_TYPE (decl);
> + location_t loc = DECL_SOURCE_LOCATION (decl);
>
> if (gimplify_omp_ctxp->target_firstprivatize_array_bases
> && omp_privatize_by_reference (decl))
> type = TREE_TYPE (type);
> - if (!omp_mappable_type (type))
> +
> + if (!verify_type_context (loc, TCTX_OMP_MAP_IMP_REF, type))
> + {
> + /* Check if TYPE can appear in a target region.
> + verify_type_context has already issued an error if it
> + can't. */
> + nflags |= GOVD_MAP | GOVD_EXPLICIT;
> + }
> + else if (!omp_mappable_type (type))
> {
> error ("%qD referenced in target region does not have "
> "a mappable type", decl);
> @@ -12815,6 +12824,8 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq
> *pre_p,
> unsigned int flags;
> tree decl;
> auto_vec<omp_addr_token *, 10> addr_tokens;
> + tree op = NULL_TREE;
> + location_t loc = OMP_CLAUSE_LOCATION (c);
>
> if (grp_end && c == OMP_CLAUSE_CHAIN (grp_end))
> {
> @@ -12822,6 +12833,36 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq
> *pre_p,
> grp_end = NULL_TREE;
> }
>
> + if (code == OMP_TARGET
> + || code == OMP_TARGET_DATA
> + || code == OMP_TARGET_ENTER_DATA
> + || code == OMP_TARGET_EXIT_DATA)
> + /* Do some target-specific type checks for map operands. */
> + switch (OMP_CLAUSE_CODE (c))
> + {
> + case OMP_CLAUSE_MAP:
> + op = OMP_CLAUSE_OPERAND (c, 0);
> + verify_type_context (loc, TCTX_OMP_MAP, TREE_TYPE (op));
> + break;
> + case OMP_CLAUSE_PRIVATE:
> + op = OMP_CLAUSE_OPERAND (c, 0);
> + verify_type_context (loc, TCTX_OMP_PRIVATE, TREE_TYPE (op));
> + break;
> + case OMP_CLAUSE_FIRSTPRIVATE:
> + op = OMP_CLAUSE_OPERAND (c, 0);
> + verify_type_context (loc, TCTX_OMP_FIRSTPRIVATE, TREE_TYPE (op));
> + break;
> + case OMP_CLAUSE_IS_DEVICE_PTR:
> + case OMP_CLAUSE_USE_DEVICE_ADDR:
> + case OMP_CLAUSE_USE_DEVICE_PTR:
> + case OMP_CLAUSE_HAS_DEVICE_ADDR:
> + op = OMP_CLAUSE_OPERAND (c, 0);
> + verify_type_context (loc, TCTX_OMP_DEVICE_ADDR, TREE_TYPE (op));
> + break;
> + default:
> + break;
> + }
> +
> switch (OMP_CLAUSE_CODE (c))
> {
> case OMP_CLAUSE_PRIVATE:
> diff --git a/gcc/target.h b/gcc/target.h
> index 3e1ee68a341..618c9c03614 100644
> --- a/gcc/target.h
> +++ b/gcc/target.h
> @@ -274,7 +274,24 @@ enum type_context_kind {
> TCTX_EXCEPTIONS,
>
> /* Capturing objects of type T by value in a closure. */
> - TCTX_CAPTURE_BY_COPY
> + TCTX_CAPTURE_BY_COPY,
> +
> + /* Objects of type T appearing in OpenMP map clause. */
> + TCTX_OMP_MAP,
> +
> + /* Objects of type T appearing in OpenMP target region
> + without explicit map. */
> + TCTX_OMP_MAP_IMP_REF,
> +
> + /* Objects of type T appearing in OpenMP private clause. */
> + TCTX_OMP_PRIVATE,
> +
> + /* Objects of type T appearing in OpenMP firstprivate clause. */
> + TCTX_OMP_FIRSTPRIVATE,
> +
> + /* Objects of type T appearing in OpenMP device clauses. */
> + TCTX_OMP_DEVICE_ADDR
> +
> };
>
> enum poly_value_estimate_kind
> @@ -331,6 +348,24 @@ mode_can_transfer_bits (machine_mode mode)
> return true;
> }
>
> +/* Return true if OpenMP context types. */
> +
> +inline bool
> +omp_type_context (type_context_kind context)
> +{
> + switch (context)
> + {
> + case TCTX_OMP_MAP:
> + case TCTX_OMP_MAP_IMP_REF:
> + case TCTX_OMP_PRIVATE:
> + case TCTX_OMP_FIRSTPRIVATE:
> + case TCTX_OMP_DEVICE_ADDR:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> #ifdef GCC_TM_H
>
> #ifndef CUMULATIVE_ARGS_MAGIC
