On 12/12/2023 10:05, Tobias Burnus wrote:
Hi Andrew,
On 11.12.23 18:04, Andrew Stubbs wrote:
This creates a new predefined allocator as a shortcut for using pinned
memory with OpenMP. The name uses the OpenMP extension space and is
intended to be consistent with other OpenMP implementations currently in
development.
Discussed this with Jakub - and 9 does not permit to have a contiguous
range of numbers if OpenMP ever extends this,
Thus, maybe start the ompx_ with 100.
These numbers are not defined in any standard, are they? We can use
whatever enumeration we choose.
I'm happy to change them, but the *_mem_alloc numbers are used as an
index into a constant table to map them to the corresponding
*_mem_space, so do we really want to make it a sparse table?
We were also pondering whether it should be ompx_gnu_pinned_mem_alloc or
ompx_pinned_mem_alloc.
It's a long time ago now, and I'm struggling to remember, but I think
those names were agreed with some other parties (can't remember who
though, and I may be thinking of the ompx_unified_shared_mem_alloc that
is still to come).
The only other compiler supporting this flag seems to be IBM; their
compiler uses ompx_pinned_mem_alloc with the same meaning:
https://www.ibm.com/support/pages/system/files/inline-files/OMP5_User_Reference.pdf
(page 5)
As the obvious meaning is what both compilers have, I am fine without
the "gnu" infix, which Jakub accepted.
Good.
* * *
And you have not updated the compiler itself to support more this new
allocator. Cf.
https://github.com/gcc-mirror/gcc/blob/master/gcc/testsuite/c-c++-common/gomp/allocate-9.c#L23-L28
That file gives an overview what needs to be changed:
* The check functions mentioned there (seemingly for two ranges now)
* Update the OMP_ALLOCATOR env var parser in env.c
* That linked testcase (and possibly some some more) should be updated,
also to ensure that the new allocator is accepted + to check for new
unsupported values (99, 101 ?)
If we now leave gaps, the const_assert in libgomp/allocator.c probably
needs to be updated as well.
* * *
Glancing through the patches, for test cases, I think you should
'abort()' in CHECK_SIZE if it fails (rlimit issue or not supported
system). Or do you think that the results are still could make sense
when continuing and possibly failing later?
Those were not meant to be part of the test, really, but rather to
demystify failures for future maintainers.
Tobias
Thanks for the review.
Andrew
