Re: [Patch] libgomp: Handle OpenMP's reverse offloads

[Jakub Jelinek <jakub@redhat.com>]

On Tue, Dec 06, 2022 at 08:45:07AM +0100, Tobias Burnus wrote:
> 32bit vs. 64bit: libgomp itself is compiled with both -m32 and -m64; however,
> nvptx and gcn requires -m64 on the device side and assume that the device
> pointers are representable on the host (i.e. all are 64bit). The new code
> tries to be in principle compatible with uint32_t pointers and uses uint64_t
> to represent it consistently. – The code should be mostly fine, except that
> one called function requires an array of void* and size_t. Instead of handling
> that case, I added some code to permit optimizing away the function content
> without offloading - and a run-time assert if it should ever happen that this
> function gets called on a 32bit host from the target side.

I think we just shouldn't support libgomp plugins for 32-bit libgomp, only
host fallback.  If you want offloading, use 64-bit host...

> libgomp: Handle OpenMP's reverse offloads
> 
> This commit enabled reverse offload for nvptx such that gomp_target_rev
> actually gets called.  And it fills the latter function to do all of
> the following: finding the host function to the device func ptr and
> copying the arguments to the host, processing the mapping/firstprivate,
> calling the host function, copying back the data and freeing as needed.
> 
> The data handling is made easier by assuming that all host variables
> either existed before (and are in the mapping) or that those are
> devices variables not yet available on the host. Thus, the reverse
> mapping can do without refcounts etc. Note that the spec disallows
> inside a target region device-affecting constructs other than target
> plus ancestor device-modifier and it also limits the clauses permitted
> on this construct.
> 
> For the function addresses, an additional splay tree is used; for
> the lookup of mapped variables, the existing splay-tree is used.
> Unfortunately, its data structure requires a full walk of the tree;
> Additionally, the just mapped variables are recorded in a separate
> data structure an extra lookup. While the lookup is slow, assuming
> that only few variables get mapped in each reverse offload construct
> and that reverse offload is the exception and not performance critical,
> this seems to be acceptable.
> 
> libgomp/ChangeLog:
> 
> 	* libgomp.h (struct target_mem_desc): Predeclare; move
> 	below after 'reverse_splay_tree_node' and add rev_array
> 	member.
> 	(struct reverse_splay_tree_key_s, reverse_splay_compare): New.
> 	(reverse_splay_tree_node, reverse_splay_tree,
> 	reverse_splay_tree_key): New typedef.
> 	(struct gomp_device_descr): Add mem_map_rev member.
> 	* oacc-host.c (host_dispatch): NULL init .mem_map_rev.
> 	* plugin/plugin-nvptx.c (GOMP_OFFLOAD_get_num_devices): Claim
> 	support for GOMP_REQUIRES_REVERSE_OFFLOAD.
> 	* splay-tree.h (splay_tree_callback_stop): New typedef; like
> 	splay_tree_callback but returning int not void.
> 	(splay_tree_foreach_lazy): Define; like splay_tree_foreach but
> 	taking splay_tree_callback_stop as argument.
> 	* splay-tree.c (splay_tree_foreach_internal_lazy,
> 	splay_tree_foreach_lazy): New; but early exit if callback returns
> 	nonzero.
> 	* target.c: Instatiate splay_tree_c with splay_tree_prefix 'reverse'.
> 	(gomp_map_lookup_rev): New.
> 	(gomp_load_image_to_device): Handle reverse-offload function
> 	lookup table.
> 	(gomp_unload_image_from_device): Free devicep->mem_map_rev.
> 	(struct gomp_splay_tree_rev_lookup_data, gomp_splay_tree_rev_lookup,
> 	gomp_map_rev_lookup, struct cpy_data, gomp_map_cdata_lookup_int,
> 	gomp_map_cdata_lookup): New auxiliary structs and functions for
> 	gomp_target_rev.
> 	(gomp_target_rev): Implement reverse offloading and its mapping.
> 	(gomp_target_init): Init current_device.mem_map_rev.root.
> 	* testsuite/libgomp.fortran/reverse-offload-2.f90: New test.
> 	* testsuite/libgomp.fortran/reverse-offload-3.f90: New test.
> 	* testsuite/libgomp.fortran/reverse-offload-4.f90: New test.
> 	* testsuite/libgomp.fortran/reverse-offload-5.f90: New test.
> 	* testsuite/libgomp.fortran/reverse-offload-5a.f90: New test without
> 	mapping of on-device allocated variables.

> +  /* Likeverse for the reverse lookup device->host for reverse offload. */

Likewise

> +  reverse_splay_tree_node rev_array;

Do we need reverse_splay_tree* stuff in libgomp.h?
As splay_tree_node is just a pointer, perhaps just
struct reverse_splay_tree_node_s;
early and
  struct reverse_splay_tree_node_s *rev_array;
in libgomp.h and include the extra splay-tree.h only in target.c?
Unless one needs it anywhere else...

Otherwise LGTM.

	Jakub