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

[Jakub Jelinek <jakub@redhat.com>]

On Tue, May 10, 2022 at 07:29:23PM +0800, Chung-Lin Tang wrote:
> I've attached v2 of the patch. Currently in testing.

Just a general rant, the non-requires dynamic_allocators support
seems to be a total mess in the standard.  Probably something
that should be discussed on omp-lang.

Allocators can appear in various places, with requires dynamic_allocators
it is all clear, the only allocators required to be constant expressions
(aka predefined allocators) are on allocate for non-automatic variables
(my understanding is that omp_null_allocator isn't valid for those but I
could be wrong), but there are no other requirements imposed (except of
course referencing a destroyed or not yet initialized allocator is UB),
in particular omp_alloc etc. can be passed omp_null_allocator, or a
variable, and similarly for allocate clause etc.

Without requires dynamic_allocators, there are various extra restrictions
imposed:
1) omp_init_allocator/omp_destroy_allocator may not be called (except for
   implicit calls to it from uses_allocators) in a target region
2) omp_alloc etc. can't be called with omp_null_allocator and the argument
   has to be a constant expression for a predefined memory allocator
   (that is also mentioned on uses_allocators, though that doesn't have to
   be visible in the source because it could be lexically included in
   a target construct's body)
3) for allocate directive on static vars the above applies plus it has
   to be mentioned in uses_allocators
4) for allocate clause e.g. when privatizing stuff or allocate directive
   for automatic vars no such restrictions exist

Now, that means that e.g. the user provided uses_allocators without
requires dynamic_allocators are only useful for the case 4), it is unclear
if that was really intended.

With uses_allocators in particular, it is unclear if
uses_allocators(omp_null_allocator) is allowed or not, IMHO it shouldn't,
but I really don't see a restriction disallowing it.

Then there is the issue that 5.0/5.1 said for C/C++ that traits-array
should be
"an identifier of const omp_alloctrait_t * type."
which is wrong for multiple reasons, because identifiers don't have type,
expressions or variables do, but more importantly because from the pointer
to const omp_alloctrait_t it is impossible to find out how many elements
the traits have.  5.2 fixed that to say that it must be an array
(so we thankfully know the size), so we certainly should consider that
change like a defect report against 5.0/5.1 too and require even in the
old syntax an array.  Note, I'm afraid we need to support even VLAs,
not just constant size arrays.

There is also in the spec that when allocator in uses_allocators is
a variable, it is treated as a private variable that can't be explicitly
privatized, but nothing said about the traits array, so is say:
void foo () {
omp_allocator_handle_t h;
omp_alloctrait_t t[3] = { ... };
#pragma omp target uses_allocators(h(t)) firstprivate(t)
{
}
ok or not?  We need to firstprivatize t so that we can call
h = omp_init_allocator (omp_default_mem_space, 3, t); in the target region
and it is kind of difficult to privatize the same var multiple times.

And yet another issue, in omp_alloctrait_t one can point to other allocators
(with { omp_atk_fallback, some_alloc }).  If some_alloc is a predefined
allocator, fine, I don't see big deal with that, especially if that
predefined allocator is also mentioned in uses_allocators clause (before or
after).  But if it is a user allocator, there is no restriction on that, and
no way how to map that, say that there should be some specific ordering
of uses_allocators induced omp_init_allocator calls and that we should
somehow replace the host value with privatized target replacement.

More on the actual patch later.

	Jakub