On 2022/5/7 12:40 AM, Tobias Burnus wrote:
> 
> 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?

Done.

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

Okay, added some to testcases.

>> +/* 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.

I supported both syntaxes, that's why I designated it as "5.2".

> 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.)

Well, the intention here is to search for "omp_allocator_handle_kind" and "omp_memspace_handle_kind",
and use their value to check if the kinds are the same as declared allocator handles and memspace constant.
Not to generally search for "omp_...".

However the sym->attr.dimension test seems useful, added in new v2 patch.

> 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:
...
> subroutine foo
>    use m
>    use omp_lib, only: omp_alloctrait
...
>    !$omp target uses_allocators(my_allocator(traits_array) allocate(my_allocator:A) firstprivate(A)
>       ...
>    !$omp end target
> end

If someone wants to use OpenMP allocators, but intentionally only imports insufficient standard symbols from omp_lib,
then he/she is on their own :)

The specification really makes this quite clear: omp_allocator_handle_kind, omp_alloctrait, omp_memspace_handle_kind are
all part of the same package.

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

As above. I mean, what is so hard with including "use omp_lib" where you need it? :D

> * * *
> 
> 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.

I think it could be worth reminding that limitation, instead of a generic error.

>> +      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.)

I've changed it to "(deprecated) traits array list syntax", is that better?

>> +      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.

Okay.

> [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")

Thanks, didn't know it was this convenient :)

I've attached v2 of the patch. Currently in testing.

Thanks,
Chung-Lin