Re: [Patch] OpenMP: C/C++ parse 'omp allocate'

[Jakub Jelinek <jakub@redhat.com>]

On Mon, Nov 23, 2020 at 03:50:33PM +0100, Tobias Burnus wrote:
> Given that (at least for C/C++) there is some initial support for
> OpenMP 5.0's allocators, it is likely that users will try it.

Sadly at least the current implementation doesn't offer much benefits;
I meant to add e.g. HBM support through dlopening of the memkind library,
but I haven't found a box with hw I could test it on.
Something that could be done is the memory pinning, we can use mlock for
that at least on Linux, the question is how to handle small allocations.
Larger allocations (say 2 or more pages) we could just round to number of
pages with page alignment and mlock that part and munlock on freeing.
Also, we should do something smarter for the NVPTX and AMDGCN offloading
targets for the allocators, perhaps handle omp_alloc etc. with some constant
allocator arguments directly through PTX etc. directives.

> Also the release notes state: "the allocator routines of OpenMP 5.0,
> including initial|allocate|  clause support in C/C++."
> 
> The latter does not include the omp allocate directive, still,
> it can be expected that users will try:
> 
>   #pragma omp allocate(...)
> 
> And that will fail at runtime. I think that's undesirable,
> even if - like any unknown directive - -Wunknown-pragmas
> (-Wall) warns about it.
> 
> Thoughts? OK?
> 
> Tobias
> 
> PS: I have not tried to implement restrictions or additions
> like 'allocate(a[5])', which is currently rejected. I also

I think allocate(a[5]) is not valid, allocate can't mention parts of other
variables (array elements, array sections, structure members).

> did not check whether there are differences between the clause
> ([partially] implemented) and the directive (this patch).

I guess your patch is ok, but I should fine time to implement at least
the rest of the restrictions; in particular e.g.:

A declarative allocate directive must appear in the same scope as the declarations of each of
its list items and must follow all such declarations.

Check if the current scope is the scope that contains all the vars.

Stick the allocator as an artificial attribute to the decls rather than
throwing it away.

I think we should implement also the 5.1 restriction:
A declared variable may appear as a list item in at most one declarative allocate directive in a
given compilation unit.
because having multiple allocate directives for the same variable is just
insane and unspecified what it would do.

While the patch tests for C that the allocator has the right type, for C++
(for obvious reasons) it isn't checked, so we need the checking there later
from the attributes or so, at least if it is dependent.

For static storage vars, we need to verify the allocator is a constant
expression, and most likely otherwise just ignore, unless we want to use say
PTX etc. directives to allocate stuff in special kinds of memory.

For automatic variables, we likely need to handle it during gimplification,
that is the last time we can reasonably add the destructors easily
(GOMP_free) such that it would be destructed on C++ exceptions, goto out of
scope etc.

> OpenMP: C/C++ parse 'omp allocate'
> 
> gcc/c-family/ChangeLog:
> 
> 	* c-pragma.c (omp_pragmas): Add 'allocate'.
> 	* c-pragma.h (enum pragma_kind): Add PRAGMA_OMP_ALLOCATE.
> 
> gcc/c/ChangeLog:
> 
> 	* c-parser.c (c_parser_omp_allocate): New.
> 	(c_parser_omp_construct): Call it.
> 
> gcc/cp/ChangeLog:
> 
> 	* parser.c (cp_parser_omp_allocate): New.
> 	(cp_parser_omp_construct, cp_parser_pragma): Call it.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* c-c++-common/gomp/allocate-5.c: New test.

Ok, thanks.

	Jakub