Hi!
On 2023-02-10T15:31:47+0000, Andrew Stubbs <a...@codesourcery.com> wrote:
> On 10/02/2023 14:21, Thomas Schwinge wrote:
>> Is the correct fix the following [...]
>
> Yes, [...]
>>> --- 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.
My point was: for nvptx libgomp, all 'ompx_host_mem_space' allocator
functions (cited above) 'return NULL', and it's a cheap check to verify
that in 'nvptx_memspace_free'.
>>> --- 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.
These three combined, I've pushed to devel/omp/gcc-12 branch
commit 23f52e49368d7b26a1b1a72d6bb903d31666e961
"Miscellaneous clean-up re OpenMP 'ompx_unified_shared_mem_space',
'ompx_host_mem_space'",
see attached.
>>> --- 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. :)
Eh. ;-) OK, outvoted.
Grüße
Thomas
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht
München, HRB 106955
>From 23f52e49368d7b26a1b1a72d6bb903d31666e961 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <tho...@codesourcery.com>
Date: Tue, 14 Feb 2023 17:10:57 +0100
Subject: [PATCH] Miscellaneous clean-up re OpenMP
'ompx_unified_shared_mem_space', 'ompx_host_mem_space'
Clean-up for og12 commit 84914e197d91a67b3d27db0e4c69a433462983a5
"openmp, nvptx: ompx_unified_shared_mem_alloc". No functional change.
libgomp/
* config/linux/allocator.c (linux_memspace_calloc): Elide
(innocuous) duplicate 'if' condition.
* config/nvptx/allocator.c (nvptx_memspace_free): Explicitly
handle 'memspace == ompx_host_mem_space'.
* libgomp.h (gomp_is_usm_ptr): Remove.
---
libgomp/ChangeLog.omp | 6 ++++++
libgomp/config/linux/allocator.c | 3 +--
libgomp/config/nvptx/allocator.c | 4 ++++
libgomp/libgomp.h | 1 -
4 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/libgomp/ChangeLog.omp b/libgomp/ChangeLog.omp
index b667c72b8ca..1c4b1833c0b 100644
--- a/libgomp/ChangeLog.omp
+++ b/libgomp/ChangeLog.omp
@@ -1,5 +1,11 @@
2023-02-16 Thomas Schwinge <tho...@codesourcery.com>
+ * config/linux/allocator.c (linux_memspace_calloc): Elide
+ (innocuous) duplicate 'if' condition.
+ * config/nvptx/allocator.c (nvptx_memspace_free): Explicitly
+ handle 'memspace == ompx_host_mem_space'.
+ * libgomp.h (gomp_is_usm_ptr): Remove.
+
* basic-allocator.c (BASIC_ALLOC_YIELD): instead of '#deine',
'#define' it.
diff --git a/libgomp/config/linux/allocator.c b/libgomp/config/linux/allocator.c
index 07af3a2821a..8a9171c36df 100644
--- a/libgomp/config/linux/allocator.c
+++ b/libgomp/config/linux/allocator.c
@@ -95,8 +95,7 @@ linux_memspace_calloc (omp_memspace_handle_t memspace, size_t size, int pin)
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);
diff --git a/libgomp/config/nvptx/allocator.c b/libgomp/config/nvptx/allocator.c
index 7c2a7463bf7..cbf86b8a2ec 100644
--- a/libgomp/config/nvptx/allocator.c
+++ b/libgomp/config/nvptx/allocator.c
@@ -42,6 +42,7 @@
chunks. */
#include "libgomp.h"
+#include <assert.h>
#include <stdlib.h>
#define BASIC_ALLOC_PREFIX __nvptx_lowlat
@@ -93,6 +94,9 @@ nvptx_memspace_free (omp_memspace_handle_t memspace, void *addr, size_t size)
__nvptx_lowlat_free (shared_pool, addr, size);
}
+ else if (memspace == ompx_host_mem_space)
+ /* Just verify what all allocator functions return. */
+ assert (addr == NULL);
else
free (addr);
}
diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
index d1e45cc584e..c001b468252 100644
--- a/libgomp/libgomp.h
+++ b/libgomp/libgomp.h
@@ -1133,7 +1133,6 @@ extern void gomp_target_rev (uint64_t, uint64_t, uint64_t, uint64_t, uint64_t,
void *);
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);
/* Splay tree definitions. */
typedef struct splay_tree_node_s *splay_tree_node;
--
2.25.1
