On 08.12.23 15:09, Thomas Schwinge wrote:
On 22/11/2023 17:07, Tobias Burnus wrote:
Let's start with the patch itself:
--- a/libgomp/target.c
+++ b/libgomp/target.c
...
+static struct gomp_device_descr *
+get_device_for_page_locked (void)
+{
+ gomp_debug (0, "%s\n",
+ __FUNCTION__);
+
+ struct gomp_device_descr *device;
+#ifdef HAVE_SYNC_BUILTINS
+ device
+ = __atomic_load_n (&device_for_page_locked, MEMMODEL_RELAXED);
+ if (device == (void *) -1)
+ {
+ gomp_debug (0, " init\n");
+
+ gomp_init_targets_once ();
+
+ device = NULL;
+ for (int i = 0; i < num_devices; ++i)
Given that this function just sets a single variable based on whether the
page_locked_host_alloc_func function pointer exists, wouldn't it be much
simpler to just do all this handling in gomp_target_init ?
@Thomas, care to comment on this?
From what I remember, we cannot assume that 'gomp_target_init' has
already been done when we get here; therefore 'gomp_init_targets_once' is
being called here. We may get to 'get_device_for_page_locked' via
host-side OpenMP, in code that doesn't contain any OpenMP 'target'
offloading things. Therefore, this was (a) necessary to make that work,
and (b) did seem to be a useful abstraction to me.
I am not questioning the "gomp_init_targets_once ();" but I am wounding
whether only 'gomp_init_targets_once()' should remain without the
locking + loading dance - and then just set that single variable inside
gomp_target_init.
If you reach here w/o target set up, the "gomp_init_targets_once ();"
would ensure it gets initialized with all the other code inside
gomp_target_init.
And if gomp_target_init() was called before, gomp_init_targets_once()
will just return without doing anything and your are also fine.
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
