Tobias Burnus <[email protected]> writes: > However, the obfuscation due to the 'session' handling is > still okayish enough - and for GCN there is a clear benefit!
Hopefully, the session stuff is broad enough to be arbitrarily extended
in the future also. Its lifetime is meant to match that of one target
region, and ergo, anything that must be communicated between libgomp and
its plugins for a single target region could (and, in future, should) be
stored in sessions.
Or, at least, that's what I was going for with this.
>> 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);
This seems OK to me also - it just seemed to me that the former is more
prevalent in GCC (or, at least, in GNU code more broadly). Will change.
>> --- 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?
Yes, that's a good idea. Will add.
>> +#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?
The original description leaves open the possibility of the function
being implemented but libgomp nonetheless opting not to call it.
I specifically wanted to provide the guarantee of exactly
'session_allocate_target_var_table' or 'session_set_target_var_table'
being called successfully, without allowing plugin authors to rely on
which one it will be.
Obviously, this implies that session_allocate_target_var_table failing
forces a call to session_set_target_var_table, but this formualtion does
not guarantee or imply that session_allocate_target_var_table will be
attempted if present.
The reason for this is that it is valid for libgomp to not want any
target variable table, but in that case it still must call
session_set_target_var_table with a NULL pointer.
> * * *
>> +/* 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?
Perhaps "before dispatching the offload session kernel" or such, but
yes, that seems fine (it is otherwise unclear what "using" here means -
it is already used to store pointers by libgomp before the session is
finished, either after session_allocate_target_var_table if the plugin
allocates, or before session_set_target_var_table if libgomp does).
>> +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 ..." ?
Seems OK to me.
> 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.
Yes, that can be removed, good point. I only added the cache later
while writing the patch and it didn't occur to me to remove the
redundant function.
> * * *
>> +/* 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.
... yes, there's clearly a typo there.
I'll replace the former sentence with your version.
> ?
>
> * * *
>
>> --- 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.)
Apparently it's only standard since C23 - I was under the impression
it's older.
%ull could work - I expect even that is more than any value that can
reasonably appear here and, since it's for a mere debug print, it isn't
too important.
Hopefully we can lose this nonsense with the C++ conversion eventually.
>
>> +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.
Yes, caching this in init_environment_variables seems fair. I'll do
that.
Here's a range diff (i.e. a diff-of-diffs) of the (untested) updated
patch series, including the comment Andrew asked for on the parallel
init patch:
~/gcc/gcc 130 $ git --no-pager range-diff ed3abd237854...HEAD
1: 9462a78715ed ! 1: 38cacbb6b203 libgomp/gcn: parallelize initializing
threads of a team
@@ Commit message
be able to read from, and only initializes each remaining thread in the
team with a few pointers.
- No functional changes intended in this commit.
+ No functional changes intended in this commit. It may seem like there
+ is a functional change, as gomp_prep_our_thread no longer sets
+ icv.nthreads_var, whereas the old code did, but the value that was
being
+ set by old code was always equal to the value already present in the
+ ICV, because both are initialized from parent tasks ICV (or global ICV
+ if that's missing) and, hence, the write was always redundant.
libgomp/ChangeLog:
2: 3f95756e73fb ! 2: 33ac16dc133b libgomp: let plugins handle allocating the
target variable table
@@ libgomp/libgomp-plugin.h: extern int GOMP_OFFLOAD_memcpy3d (int, int,
size_t, si
+[[gnu::const]] extern size_t GOMP_OFFLOAD_session_size (void);
+
+/* Check that the 'struct gomp_offload_struct' declaration is acceptable,
and
-+ implement GOMP_OFFLOAD_session_size. */
++ implement GOMP_OFFLOAD_session_size. Call this in the plugin after
defining
++ the aforementioned struct. */
+#define GOMP_OFFLOAD_session_boilerplate() \
+ GOMP_OFFLOAD_check_session_struct (); \
+ [[gnu::const]] size_t \
@@ libgomp/libgomp-plugin.h: extern int GOMP_OFFLOAD_memcpy3d (int, int,
size_t, si
+/* 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. */
++ Before dispatching the offload kernel associated with this session,
exactly
++ a successful call to GOMP_OFFLOAD_session_allocate_target_var_table or
a
++ call to this function must happen, but not both. */
+extern void GOMP_OFFLOAD_session_set_target_var_table
+ (struct gomp_offload_session *session, void **table);
+
@@ libgomp/libgomp.h: struct gomp_device_descr
__typeof (GOMP_OFFLOAD_memcpy3d) *memcpy3d_func;
__typeof (GOMP_OFFLOAD_memset) *memset_func;
+ struct {
-+ /* Cached below as 'size'. */
-+ __typeof (GOMP_OFFLOAD_session_size) *size_func;
+ __typeof (GOMP_OFFLOAD_session_start) *start_func;
+ __typeof (GOMP_OFFLOAD_session_allocate_target_var_table)
*alloc_tvt_func;
+ __typeof (GOMP_OFFLOAD_session_set_target_var_table) *set_tvt_func;
+
-+ /* Size of a single gomp_offload_session object, as specified by
++ /* Size of a single gomp_offload_session object, as returned by
+ GOMP_OFFLOAD_session_size. */
+ size_t size;
+ } session;
@@ libgomp/libgomp.h: struct gomp_device_descr
acc_dispatch_t openacc;
};
-+/* 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
++/* Allocate an offload session for the gomp_device_descr DEVICEP using
ALLOC,
++ and initialize it. Provided as a macro, so that 'alloca' can be used
as
+ ALLOC. */
+#define gomp_offload_session_new(devicep, alloc) \
+ ({ \
@@ libgomp/oacc-host.c
+};
+_Static_assert (_Alignof (struct host_offload_session) <
__BIGGEST_ALIGNMENT__,
+ "gomp_offload_session requires too high alignment");
-+
-+static size_t
-+host_session_size (void)
-+{ return sizeof (struct host_offload_session); }
static struct gomp_device_descr host_dispatch;
@@ libgomp/oacc-host.c: static struct gomp_device_descr host_dispatch =
.run_func = host_run,
+ .session = {
-+ .size_func = host_session_size,
+ .start_func = host_session_start,
+ .set_tvt_func = host_session_set_target_var_table,
+ .size = sizeof (struct host_offload_session),
@@ libgomp/plugin/plugin-gcn.c: struct kernargs {
struct GOMP_kernel_launch_attributes kla;
};
+@@ libgomp/plugin/plugin-gcn.c: static int lowlat_size = -1;
+
+ static bool debug;
+
++/* Flag to decide whether to prevent merging the kernel arguments with the
++ target variable table, i.e. whether to always fail
++ GOMP_OFFLOAD_session_allocate_target_var_table. Set from the
++ GCN_INHIBIT_KERNARGS_TVT_MERGE env var. */
++
++static bool inhibit_kernargs_tvt_merge;
++
+ /* Flag to decide if the runtime should suppress a possible fallback to
host
+ execution. */
+
@@ libgomp/plugin/plugin-gcn.c: dump_executable_symbols (hsa_executable_t
executable)
/* Dump kernel DISPATCH data structure and indent it by INDENT spaces. */
@@ libgomp/plugin/plugin-gcn.c: dump_executable_symbols (hsa_executable_t
executabl
fprintf (stderr, "%*sthis: %p\n", indent, "", dispatch);
fprintf (stderr, "%*squeue: %p\n", indent, "", dispatch->queue);
fprintf (stderr, "%*skernarg_address: %p\n", indent, "", kernargs);
+@@ libgomp/plugin/plugin-gcn.c: init_environment_variables (void)
+ const char *lowlat = secure_getenv ("GOMP_GCN_LOWLAT_POOL");
+ if (lowlat)
+ lowlat_size = atoi (lowlat);
++
++ inhibit_kernargs_tvt_merge
++ = (bool) secure_getenv ("GCN_INHIBIT_KERNARGS_TVT_MERGE");
+ }
+
+ /* Return malloc'd string with name of SYMBOL. */
@@ libgomp/plugin/plugin-gcn.c: max_isa_vgprs (int isa)
/* }}} */
@@ libgomp/plugin/plugin-gcn.c: max_isa_vgprs (int isa)
+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);
++ GCN_DEBUG ("Session %p asked for allocation of kernargs+%llu...\n",
++ session, (unsigned long long) table_size);
+ struct agent_info *agent = session->agent;
+ assert (!session->kernarg_cache_node);
+
@@ libgomp/plugin/plugin-gcn.c: max_isa_vgprs (int isa)
+ /* libgomp wants us to handle the TVT. */
+ assert (!session->target_var_table);
+
-+ if (secure_getenv ("GCN_INHIBIT_KERNARGS_TVT_MERGE"))
++ if (inhibit_kernargs_tvt_merge)
+ /* ... but the user does not. Used for testing. */
+ return NULL;
+
@@ libgomp/target.c: GOMP_target (int device, void (*fn) (void *), const
void *unus
|| !(fn_addr = gomp_get_target_fn_addr (devicep, fn)))
return gomp_target_fallback (fn, hostaddrs, devicep, NULL);
-+ struct gomp_offload_session *session = (gomp_offload_session_new
-+ (devicep, alloca));
++ struct gomp_offload_session *session
++ = gomp_offload_session_new (devicep, alloca);
+
htab_t refcount_set = htab_create (mapnum);
struct target_mem_desc *tgt_vars
@@ libgomp/target.c: GOMP_target_ext (int device, void (*fn) (void *),
size_t mapnu
struct target_mem_desc *tgt_vars;
htab_t refcount_set = NULL;
-+ struct gomp_offload_session *session = (gomp_offload_session_new
-+ (devicep, alloca));
++ struct gomp_offload_session *session
++ = gomp_offload_session_new (devicep, alloca);
+
if (devicep->capabilities & GOMP_OFFLOAD_CAP_SHARED_MEM)
{
@@ libgomp/target.c: gomp_load_plugin_for_device (struct gomp_device_descr
*device,
+ device->session.size = 0;
if (device->capabilities & GOMP_OFFLOAD_CAP_OPENMP_400)
{
-+ DLSYM2 (session.size, session_size);
-+ device->session.size = device->session.size_func ();
++ __typeof (GOMP_OFFLOAD_session_size) *size_func
++ = dlsym (plugin_handle, "GOMP_OFFLOAD_session_size");
++ device->session.size = size_func ();
+ DLSYM2 (session.start, session_start);
+ DLSYM_OPT (session.alloc_tvt, session_allocate_target_var_table);
+ DLSYM2 (session.set_tvt, session_set_target_var_table);
3: 845319e68d1f = 3: ee8ead4d9759 libgomp/plugin-gcn: remove unneeded heap
allocation in run_kernel
4: ed3abd237854 = 4: 7e06df7286eb libgomp/oacc-mem: add missing assert to
goacc_enter_datum
--
Arsen Arsenović
signature.asc
Description: PGP signature
