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.
