Re: [PATCH, OpenMP] Implement uses_allocators clause for target regions

[Tobias Burnus <tobias@codesourcery.com>]

Hi Chung-Lin,

thanks for the patch – and some comments from my side.

On 06.05.22 15:20, Chung-Lin Tang wrote:
> For user defined allocator handles, this allows target regions to assign
> memory space and traits to allocators, and automatically calls
> omp_init/destroy_allocator() in the beginning/end of the target region.

Can please also handle the new clause in Fortran's dump-parse-tree.cc?

I did see some split handling in C, but not in Fortran; do you also need
to up update gfc_split_omp_clauses in Fortran's trans-openmp.cc?

Actually, glancing at the testcases, no combined construct (like
"omp target parallel") is used, I think that would be useful because of ↑.

> +/* OpenMP 5.2:
> +   uses_allocators ( allocator-list )
That's not completely true: uses_allocators is OpenMP 5.1.
However, 5.1 only supports (for non-predefined allocators):
    uses_allocators( allocator(traits) )
while OpenMP 5.2 added modifiers:
    uses_allocatrors( traits(...), memspace(...) : allocator )
and deprecated the 5.1 'allocator(traits)'. (Scheduled for removal in OMP 6.0)

The advantage of 5.2 syntax is that a memory space can be defined.

BTW: This makes uses_allocators the first OpenMP 5.2 feature which
will make it into GCC :-)


gcc/fortran/openmp.cc:
> +  if (gfc_get_symbol ("omp_allocator_handle_kind", NULL, &sym)
> +      || !sym->value
> +      || sym->value->expr_type != EXPR_CONSTANT
> +      || sym->value->ts.type != BT_INTEGER)
> +    {
> +      gfc_error ("OpenMP %<omp_allocator_handle_kind%> constant not found by "
> +              "%<uses_allocators%> clause at %C");
> +      goto error;
> +    }
> +  allocator_handle_kind = sym;
I think you rather want to use
   gfc_find_symbol ("omp_...", NULL, true, &sym)
   || sym == NULL
where true is for parent_flag to search also the parent namespace.
(The function returns 1 if the symbol is ambiguous, 0 otherwise -
including 0 + sym == NULL when the symbol could not be found.)

   || sym->attr.flavor != FL_PARAMETER
   || sym->ts.type != BT_INTEGER
   || sym->attr.dimension

Looks cleaner than to access sym->value. The attr.dimension is just
to makes sure the user did not smuggle an array into this.
(Invalid as omp_... is a reserved namespace but users will still do
this and some are good in finding ICE as hobby.)

  * * *

However, I fear that will fail for the following two examples (both untested):

   use omp_lib, my_kind = omp_allocator_handle_kind
   integer(my_kind) :: my_allocator

as this gives 'my_kind' in the symtree->name (while symtree->n.sym->name is "omp_...").
Hence, by searching the symtree for 'omp_...' the symbol will not be found.


It will likely also fail for the following more realistic example:

module m
   use omp_lib
   implicit none
   private
   integer(omp_allocator_handle_kind), public :: my_allocator
   type(omp_alloctrait), public, parameter :: my_traits(*) = [...]
end module

subroutine foo
   use m
   use omp_lib, only: omp_alloctrait
   implicit none
   ! currently, same scope is required - makes sense for C and 'const' but
   ! not for Fortran's parameters; restriction might be lifted/clarified in
   ! next OpenMP version:
   type(omp_alloctrait), parameter :: traits_array(*) = my_traits
   integer :: A(200)
   A = 0
   !$omp target uses_allocators(my_allocator(traits_array) allocate(my_allocator:A) firstprivate(A)
      ...
   !$omp end target
end

In this case, omp_allocator_handle_kind is not in the namespace of 'foo'
but the code should be still valid. Thus, an alternative would be to hard-code
the value - as done for the depobj. As we have:

         integer, parameter :: omp_allocator_handle_kind = c_intptr_t
         integer, parameter :: omp_memspace_handle_kind = c_intptr_t

that would be
    sym->ts.type == BT_CHARACTER
    sym->ts.kind == gfc_index_integer_kind
for the allocator variable and the the memspace kind.

However, I grant that either example is not very typical. The second one is more
natural – such a code will very likely be written in the real world. But not
with uses_allocators but rather with "!$omp requires dynamic_allocators" and
omp_init_allocator().

Thoughts?

* * *

gcc/fortran/openmp.cc
> +      if (++i > 2)
> +     {
> +       gfc_error ("Only two modifiers are allowed on %<uses_allocators%> "
> +                  "clause at %C");
> +       goto error;
> +     }
> +

Is this really needed? There is a check for multiple traits and multiple memspace
Thus, 'trait(),memspace(),trait()' is already handled and
'trait(),something' give a break and will lead to an error as in that case
a ':' and not ',something' is expected.

> +      if (gfc_match_char ('(') == MATCH_YES)
> +     {
> +       if (memspace_seen || traits_seen)
> +         {
> +           gfc_error ("Modifiers cannot be used with legacy "
> +                      "array syntax at %C");
I wouldn't uses the term 'array synax' to denote
   uses_allocators(allocator (alloc_array) )
How about:
   error: "Using both modifiers and allocator variable with traits argument"

(And I think 'deprecated' is better than 'legacy', if we really want to use it.)


> +       if (traits_sym->ts.type != BT_DERIVED
> +           || strcmp (traits_sym->ts.u.derived->name,
> +                      "omp_alloctrait") != 0
> +           || traits_sym->attr.flavor != FL_PARAMETER
> +           || traits_sym->as->rank != 1
> +           || traits_sym->value == NULL
> +           || !gfc_is_constant_expr (traits_sym->value))

I think the gfc_is_constant_expr is unreachable as you already
have checked FL_PARAMETER. Thus, you can remove the last two
lines.

[Regarding the traits_sym->ts.u.derived->name, I am not sure whether that
won't fail with
   use omp_lib, trait_t => omp_alloctrait
but I have not checked. It likely does work correctly.]

> +           /* Check if identifier is of 'omp_..._mem_space' format.  */
> +           || (pos = strstr (memspace_sym->name, "omp_")) == NULL
> +           || pos != memspace_sym->name
> +           || (pos = strstr (memspace_sym->name, "_mem_space")) == NULL
> +           || *(pos + strlen ("_mem_space")) != '\0')

I wonder whether that's not more readable written as:
    || !startswith (memspace_sym->name, "omp_")
    || !endswith (memspace_sym->name, "_mem_space")


Tobias

-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955