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
