Re: [PATCH v3 3/3] AArch64: Diagnose OpenMP offloading when SVE types involved.

[Tejas Belagod]

On 3/7/25 5:33 PM, Jakub Jelinek wrote:

On Fri, Mar 07, 2025 at 11:49:37AM +0000, Richard Sandiford wrote:

+    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"?

I think target device clauses is fine, sure, it applies to all of
target{, data, enter data, exit data, update} but I wouldn't read target as
target construct only.

In any case, none of the SVE types make sense in the
is_device_ptr/use_device_addr/use_device_ptr/has_device_addr clauses
unless we'd have offloading from aarch64 host to aarch64 offloading device
with the same properties, because those clauses assume something is on the
offloading device already or similar.

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.

--- 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;
+               }

Please avoid {} around single statement body.

Otherwise the middle-end changes look ok to me.

Thanks Richard and Jakub for the reviews. I have now posted v4 series here:

https://gcc.gnu.org/pipermail/gcc-patches/2025-March/678086.html

Is this OK? Also, should I wait for Stage 1 if OK?

Thanks,
Tejas.

+             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

        Jakub