On 10/02/2023 14:21, Thomas Schwinge wrote:
Is the correct fix the following (conceptually like
'linux_memspace_alloc' cited above), or is there something that I fail to
understand?
static void *
linux_memspace_calloc (omp_memspace_handle_t memspace, size_t size, int
pin)
{
if (memspace == ompx_unified_shared_mem_space)
{
void *ret = gomp_usm_alloc (size, GOMP_DEVICE_ICV);
memset (ret, 0, size);
return ret;
}
- else if (memspace == ompx_unified_shared_mem_space
- || pin)
+ else if (pin)
return linux_memspace_alloc (memspace, size, pin);
else
return calloc (1, size);
Yes, I think that is what was intended (and what actually happens). You
can have your memory both unified and pinned (well, maybe it's possible,
but there's no one Cuda API for that), so the USM takes precedence.
The following ones then again are conceptually like
'linux_memspace_alloc' cited above:
@@ -77,9 +86,9 @@ static void
linux_memspace_free (omp_memspace_handle_t memspace, void *addr, size_t size,
int pin)
{
- (void)memspace;
-
- if (pin)
+ if (memspace == ompx_unified_shared_mem_space)
+ gomp_usm_free (addr, GOMP_DEVICE_ICV);
+ else if (pin)
munmap (addr, size);
else
free (addr);
@@ -89,7 +98,9 @@ static void *
linux_memspace_realloc (omp_memspace_handle_t memspace, void *addr,
size_t oldsize, size_t size, int oldpin, int pin)
{
- if (oldpin && pin)
+ if (memspace == ompx_unified_shared_mem_space)
+ goto manual_realloc;
+ else if (oldpin && pin)
{
void *newaddr = mremap (addr, oldsize, size, MREMAP_MAYMOVE);
if (newaddr == MAP_FAILED)
@@ -98,18 +109,19 @@ linux_memspace_realloc (omp_memspace_handle_t memspace,
void *addr,
[...]
Yes.
..., and similar those here:
--- a/libgomp/config/nvptx/allocator.c
+++ b/libgomp/config/nvptx/allocator.c
@@ -125,6 +125,8 @@ nvptx_memspace_alloc (omp_memspace_handle_t memspace,
size_t size)
__atomic_store_n (&__nvptx_lowlat_heap_root, root.raw,
MEMMODEL_RELEASE);
return result;
}
+ else if (memspace == ompx_host_mem_space)
+ return NULL;
else
return malloc (size);
}
@@ -145,6 +147,8 @@ nvptx_memspace_calloc (omp_memspace_handle_t memspace,
size_t size)
return result;
}
+ else if (memspace == ompx_host_mem_space)
+ return NULL;
else
return calloc (1, size);
}
@@ -354,6 +358,8 @@ nvptx_memspace_realloc (omp_memspace_handle_t memspace,
void *addr,
}
return result;
}
+ else if (memspace == ompx_host_mem_space)
+ return NULL;
else
return realloc (addr, size);
}
(I'd have added an explicit no-op (or, 'abort'?) to
'nvptx_memspace_free', but that's maybe just me...) ;-\
Why? The host memspace is just the regular heap, which can be a thing on
any device. It's an extension though so we can define it either way.
--- a/libgomp/libgomp.h
+++ b/libgomp/libgomp.h
+extern void * gomp_usm_alloc (size_t size, int device_num);
+extern void gomp_usm_free (void *device_ptr, int device_num);
+extern bool gomp_is_usm_ptr (void *ptr);
'gomp_is_usm_ptr' isn't defined/used anywhere; I'll remove it.
I think I started that and then decided against. Thanks.
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -3740,6 +3807,9 @@ gomp_load_plugin_for_device (struct gomp_device_descr
*device,
DLSYM (unload_image);
DLSYM (alloc);
DLSYM (free);
+ DLSYM_OPT (usm_alloc, usm_alloc);
+ DLSYM_OPT (usm_free, usm_free);
+ DLSYM_OPT (is_usm_ptr, is_usm_ptr);
DLSYM (dev2host);
DLSYM (host2dev);
As a sanity check, shouldn't we check that either none or all three of
those are defined, like in the 'if (cuda && cuda != 4) { [error] }' check
a bit further down?
This is only going to happen when somebody writes a new plugin, and then
they'll discover very quickly that there are issues. I've wasted more
time writing this sentence than it's worth already. :)
Note that these remarks likewise apply to the current upstream
submission:
<https://inbox.sourceware.org/gcc-patches/ef374d055251b2bc65b97d7e54a0a72d811b869d.1657188329.git....@codesourcery.com>>
"openmp, nvptx: ompx_unified_shared_mem_alloc".
I have new patches to heap on top of this set already on OG12, and more
planned, plus these ones you're working on; the whole patchset is going
to have to get a rebase, squash, and tidy "soonish".
Andrew
