First, I want to note that:
* [PATCH 1/4] libgomp/gcn: parallelize initializing threads of a team
has been approved by Andrew (with request to expand a comment)
* [PATCH 3/4] libgomp/plugin-gcn: remove unneeded heap allocation in
run_kernel
has been approved by Andrew
* [PATCH 4/4] libgomp/oacc-mem: add missing assert to goacc_enter_datum
is trivial and has been approved by Thomas.
Hence, only his patch remains to be reviewed.
On May 5, 2026, Arsen Arsenović wrote:
In my examination of BabelStream results on AMD GCN, I've found that,
for each BabelStream kernel execution, we spend significant time in
allocating and initializing memory in gomp_map_vars (~55µs, whereas the
actual BabelStream code executes in ~746µs, meaning we increase the time
BabelStream measures by 7% just on that).
This overhead matters - not only for BabelStream but also for some
real world code that uses many tiny kernels rather than one
kernel and then doing lengthier work.
Upon further examination, I've found that the only reason gomp_map_vars
decides to allocate and map any memory in the first place is because it
is constructing the table of pointers to variables on the target, which
I've taken to calling the "target variable table". Given that the GCN
plugin already must perform some memory allocation before starting up a
kernel, namely to allocate kernel arguments, it would be beneficial if
we could merge this allocation with the kernel arguments allocation.
In addition, since the kernel arguments live in host memory, populating
them can be performed using string functions, without any need to call
for expensive host2dev copies.
I note that Nvptx (at least as currently implemented) does not
profit from this - and effectively will do the same as previously.
However, the obfuscation due to the 'session' handling is
still okayish enough - and for GCN there is a clear benefit!
This patch introduces an opaque type for "offload sessions". This type
is defined by each plugin and allows it to store data related to a
single offload job. The sessions are allocated and managed by libgomp,
and initialized and utilized by the plugin. Their lifetime starts with
a call to GOMP_OFFLOAD_session_start, and ends with
GOMP_OFFLOAD_{openacc_{async_,}exec,{async_,}run}.
The patch then uses this framework to make management of the target
variable table more flexible: the plugin may elect to implement
GOMP_OFFLOAD_session_allocate_target_var_table, which allows the plugin
to attempt to allocate the target variable table in host memory.
If it fails, or if the plugin does not provide this function, libgomp
will perform this allocation as it does today - in target memory - and
tell the session about it using
GOMP_OFFLOAD_session_set_target_var_table.
Namely, the code either asks the runtime to allocate the memory,
which works for GCN and is cached (as kernel args in general)
or as for 'target {enter,exit,} data' or for Nvptx also for target,
the allocation is handled by libgomp - and the mentioned function
is then used to add it to the session opaque state structure.
* * *
As a general remark, I find the following hard to read, when
glancing at the code (appears twice):
struct gomp_offload_session *session = (gomp_offload_session_new
(devicep, alloca));
I think it is easier to read if written as either
struct gomp_offload_session *session
= gomp_offload_session_new (devicep, alloca);
or
struct gomp_offload_session *session = gomp_offload_session_new (devicep,
alloca);
?
The problem is that due to the ( and the alignment of
the indentation, it takes a while to realize that this is
a function (ok: macro) call and to disentangle what's the
function name and what are the arguments. It doesn't look
to different from code like:
struct mystruct_t *var = {abc, def, ghm};
Same issue:
+ host_tvt = (devicep->session.alloc_tvt_func
+ (session, gomp_get_tvt_size (mapnum)));
how about:
host_tvt = devicep->session.alloc_tvt_func (session,
gomp_get_tvt_size
(mapnum));
(That's 78 characters.)
This might be the only two cases; it felt as if there were more,
but possibly that's it.
* * *
--- a/libgomp/libgomp-plugin.h
+++ b/libgomp/libgomp-plugin.h
+/* Check that the 'struct gomp_offload_struct' declaration is acceptable, and
+ implement GOMP_OFFLOAD_session_size. */
I wonder whether adding - "Call this in the plugin
after defining the struct." makes this clearer?
+#define GOMP_OFFLOAD_session_boilerplate() \
...
* * *
+/* Attempt to allocate a target variable table in host memory for SESSION.
+ This table must be of at least table_size bytes and aligned to
+ __BIGGEST_ALIGNMENT__.
+
+ This function will be called at most once per SESSION.
+
+ If this function returns NULL, or if libgomp never calls it,
+ GOMP_OFFLOAD_session_set_target_var_table will be called instead, with
+ memory allocated by libgomp for the purpose.
+
+ If this function is omitted, libgomp will behave as if it always returns
+ NULL. */
+extern void **GOMP_OFFLOAD_session_allocate_target_var_table
I gather that this is mostly written from the point of a plugin
writer. Still, it is also read when trying to understand the code
in general and as libgomp/*.c programmer. Still, I find the last
three paragraphs rather confusing. How about something like that:
This function is optional and might not be implemented. If not
implemented or when it returns NULL, the memory allocation shall
be done by the caller followed by a call to
GOMP_OFFLOAD_session_allocate_target_var_table.
Or something like that?
* * *
+/* Set TABLE, a device pointer, as the pointer to the target variable table.
+ It may be NULL, in which case there's no target variable table.
+
+ Called iff GOMP_OFFLOAD_session_allocate_target_var_table did not succeed
+ or was not called. */
Either this function or GOMP_OFFLOAD_session_allocate_target_var_table
(returning a non-NULL pointer) must be called before using the
target variable table.
Or something like that?
+extern void GOMP_OFFLOAD_session_set_target_var_table
...
* * *
+ struct {
+ /* Cached below as 'size'. */
+ __typeof (GOMP_OFFLOAD_session_size) *size_func;
...
+ /* Size of a single gomp_offload_session object, as specified by
+ GOMP_OFFLOAD_session_size. */
+ size_t size;
+ } session;
"specified by ..." sounds odd - "as returned by ..." ?
Is there actually a need to add the function pointer to the struct?
+ DLSYM2 (session.size, session_size);
+ device->session.size = device->session.size_func ();
It feels as if one could just do:
__typeof (GOMP_OFFLOAD_session_size) *size_func
= dlsym (plugin, GOMP_OFFLOAD_session_size);
device->session_size = size_func ();
which seems to be possibly clearer and smaller?
However, also the current version is fine.
* * *
+/* Allocate an offload session using for gomp_device_descr DEV using ALLOC, and
+ initialize it. Provided as a macro, so that 'alloca' can be used as
+ ALLOC. */
+#define gomp_offload_session_new(devicep, alloc) \
This somehow reads odd. Maybe:
Allocate an offload session for the gomp_device_descr DEVICEP
using ALLOC and initialize it.
?
* * *
--- a/libgomp/plugin/plugin-gcn.c
+++ b/libgomp/plugin/plugin-gcn.c
+allocate_session_kernargs (struct gomp_offload_session *session,
+ size_t table_size)
+{
+ GCN_DEBUG ("Session %p asked for allocation of kernargs+%zu...\n", session,
table_size);
First, this line is too long.
Second, I wonder whether it is okay to use %zu or not.
For the current implementation, it probably is - but if
we ever want to move to Windows, it probably isn't. At
least for MinGW, GCC warns when using %zu - even though it
is new since C99, i.e. a long time.
Cf. ->
https://www.eevblog.com/forum/programming/why-does-gccs-format-function-attribute-no-longer-think-zu-is-valid/
I am torn between replacing it by something
different and leaving it as is, given that we
target Linux and C99 is old and somewhat common.
(And depending what is linked on Windows for printf,
it actually works - besides: if ever compiling for
MinGW, GCC seemingly will warn.)
* * *
+GOMP_OFFLOAD_session_allocate_target_var_table (struct gomp_offload_session
*session,
+ size_t table_size)
+{
+ GCN_DEBUG ("Session %p asked to allocate\n", session);
+ /* libgomp wants us to handle the TVT. */
+ assert (!session->target_var_table);
+
+ if (secure_getenv ("GCN_INHIBIT_KERNARGS_TVT_MERGE"))
+ /* ... but the user does not. Used for testing. */
+ return NULL;
I am a bit unsure whether it makes sense that this is called
every time. On one hand, getenv has some overhead - and it
will be called once per kernel launch. On the other hand,
there is so much action happening and, hopefully, the time
spend here is neglibile compared with all other overhead.
Still, one option would be to cache the value.
* * *
+ /* Target variable table in host memory. If we're doing target offloading,
+ we'll let the plugin attempt to allocate it.. */
s/.././
Otherwise LGTM.
Thanks for the patch!
Tobias
