Re: [PATCH 3/5] openmp, nvptx: ompx_unified_shared_mem_alloc

[Andrew Stubbs <ams@codesourcery.com>]

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.ams@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