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

[Andrew Stubbs <ams@codesourcery.com>]

On 19/05/2022 17:00, Jakub Jelinek wrote:
> 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

I interpreted that more like "omp_init_allocator/... is not required to 
work", as in the set-up steps provided by 
dynamic_allocators/uses_allocators won't be available. Since we don't 
have any such on/off mode I don't believe we need to worry about this 
(and adding extra logic for this is make-work which will not improve the 
user-experience).

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

Again, does a conforming implementation reject this, or is it merely not 
required to accept it?

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

We are only implementing 5.0 at this time. If 5.2 is less work and the 
only way to achieve correctness then maybe that's the way to go, but in 
general, one step at a time, please.

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

Thank you.