thanks for proof reading and the suggestions! – Do have comments to the
attached patch?
* * *
Crossref: For further optimizations, see also
https://gcc.gnu.org/PR101581 — [OpenMP] omp_target_memcpy – support
inter-device memcpy
https://gcc.gnu.org/PR110813 — [OpenMP] omp_target_memcpy_rect (+
strided 'target update'): Improve GCN performance and contiguous
subranges
and just added based on Thomas' comment:
https://gcc.gnu.org/PR107424 — [OpenMP] Check whether device locking is
really needed for bare memcopy to/from devices (omp_target_memcpy...)
* * *
On 27.07.23 23:00, Thomas Schwinge wrote:
+++ b/include/cuda/cuda.h
I note that you're not actually using everything you're adding here.
(..., but I understand you're simply adding everying that relates to
these 'cuMemcpy[...]' routines -- OK as far as I'm concerned.)
Yes. That was on purpose to make it easier to pick something when needed
– especially as we might want to use some of those later on.
For symmetry, I now also added cuMemcpyPeer + ...Async, which also
remain unused. (But could be used as part of the PRs linked above.)
+ const void *dstHost;
That last one isn't 'const'. ;-)
Fixed - three times.
A 'cuda.h' that I looked at calls that last one 'reserved0', with
comment
"Must be NULL".
Seems to be unused in real world code and in the documentation. But
let's use this name as it might be exposed in the wild.
--- a/libgomp/libgomp-plugin.h
+++ b/libgomp/libgomp-plugin.h
+extern int GOMP_OFFLOAD_memcpy2d (int, int, size_t, size_t,
+ void*, size_t, size_t, size_t,
+ const void*, size_t, size_t, size_t);
+extern int GOMP_OFFLOAD_memcpy3d (int, int, size_t, size_t, size_t,
void *,
+ size_t, size_t, size_t, size_t, size_t,
+ const void *, size_t, size_t,
size_t, size_t,
+ size_t);
Oh, wow. ;-)
Maybe this is not the best ABI. We can consider to modify it before the
GCC 14 release. (And in principle also afterwards, given that libgomp
and its plugins should™ be compiled and installed alongside.)
I think once we know how to implement GCN, we will see whether it was
done smartly or whether other arguments should be used or whether the
two functions should be combined.
[Regarding the reserve0/reserve1 values for cuMemcpy3D and whether it
should be NULL or not; quoting the usage in plugin-nvptx.c:]
I note that this doesn't adhere to the two "Must be NULL" remarks from
above -- but I'm confused, because, for example, on
<https://docs.nvidia.com/cuda/cuda-driver-api/group__CUDA__MEM.html#group__CUDA__MEM_1g4b5238975579f002c0199a3800ca44df
"cuMemcpy3D", there also are no such remarks. (... in contast to:
"srcLOD and dstLOD members of the CUDA_MEMCPY3D structure must be set
to 0",
which you do set accordingly.)
Maybe just 'memset' the whole thing to '0', for good measure?
OK - but then we can remove the LOD init.
+ else if (src_devicep != NULL
+ && (dst_devicep == NULL
+ || (dst_devicep->capabilities
+ & GOMP_OFFLOAD_CAP_SHARED_MEM)))
Are these 'GOMP_OFFLOAD_CAP_SHARED_MEM' actually reachable, given that
'omp_target_memcpy_check' (via 'omp_target_memcpy_rect_check') clears
out
the device to 'NULL' for 'GOMP_OFFLOAD_CAP_SHARED_MEM'?
I have now undone this change – I did not dig deep enough into the
function calls.
+ else if (dst_devicep == NULL && src_devicep == NULL)
+ {
+ memcpy ((char *) dst + dst_off, (const char *) src + src_off,
+ length);
+ ret = 1;
+ }
else if (src_devicep == dst_devicep)
ret = src_devicep->dev2dev_func (src_devicep->target_id,
(char *) dst + dst_off,
(const char *) src + src_off,
length);
..., but also left the intra-device case here -- which should now be
dead
code here?
Why? Unless I missed something, the old, the current, and the proposed
(= old) code do still run this code.
I have not added an assert to confirm, but in any case, it is tested for
in my recently added testcase - thus, we could add a 'printf' to confirm.
+ else if (*tmp_size < length)
+ {
+ *tmp_size = length;
+ *tmp = realloc (*tmp, length);
+ if (*tmp == NULL)
+ return ENOMEM;
If 'realloc' returns 'NULL', we should 'free' the original '*tmp'?
Do we really need here the property here that if the re-allocation can't
be done in-place, 'realloc' copies the original content to the new? In
other words, should we just unconditionally 'free' and re-'malloc' here,
instead of 'realloc'?
I have now done so – but I am not really sure what's faster on average.
If it can be enlarged, 'realloc' is faster, if it cannot free+malloc is
better.
I haven't looked whether the re-use of 'tmp' for multiple calls to this
is then actually useful, or whether we should just always 'malloc', use,
'free' the buffer here?
Well, it can run in a hot loop – assume a C-array array[1024][1024][2]
and copying array[:1024,:1024,0:1] (using OpenMP syntax) – i.e. 1048576
times every other element. And therefore I would like to avoid repeated
malloc/free in such a case. (But in general, interdevice copying should
be very rare.)
Actually, I think the realloc case is unreachable: for rectangular
copies, as implied both by 'target update' with strided access and by
'omp_target_memcpy_rect', the size should be constant.
(For later...) Is that what
<https://docs.nvidia.com/cuda/cuda-driver-api/group__CUDA__MEM.html#group__CUDA__MEM_1ge1f5c7771544fee150ada8853c7cbf4a
"cuMemcpyPeer" would be used for?
I concur that we could/should use cuMemcpyPeer and cuMemcpy3DPeer for
omp_target and ..._rect. I added a comment to the respective PRs (linked
at the top).
+ lock_src = (src_devicep
+ && (!dst_devicep
+ || src_devicep == dst_devicep
+ || !(src_devicep->capabilities
+ & GOMP_OFFLOAD_CAP_SHARED_MEM)));
Similar doubt than above re "'GOMP_OFFLOAD_CAP_SHARED_MEM' actually
reachable"?
Updated in the attach patch + filed a PR about the need of the lock (see
link at the top).
+ DLSYM (memcpy2d);
+ DLSYM (memcpy3d);
With 'DLSYM' used here, won't that fail if these symbols don't actually
exist (like for 'libgomp/plugin/plugin-gcn.c')?
Hmm, that should be: DLSYM_OPT, but somehow testing on GCN did not show
any fails. (Neither here nor in the testsuite; odd.)
I intent to commit the attached follow-up patch very soon.
[It has been tested without offloading support compiled in, with AMD GCN
offloading (single GPU, no memcpy[23]d available), and on a 2-GPU system
with nvptx offloading.]
Tobias
-----------------
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
