Hi Thomas, hello all,
On 06.07.22 12:42, Thomas Schwinge wrote:
On 2022-07-01T15:06:05+0200, Tobias Burnus <tob...@codesourcery.com>
wrote:
Attached is the updated patch. Main changes: [...]
This is now a great implementation of cross-component communication
(host/offloading compilers, runtime), thanks! I'm sure this will be
usable (or at least instructing) for further purposes, too.
I also see potential use for other tools.
- Uses GOMP_register_var to pass the mask to libgomp
Like 'GOMP_offload_register_ver', also 'GOMP_offload_unregister_ver'
needs to be adjusted correspondingly.
Ups! Thanks for catching it and the patch.
OK to push the attached
Fixing the data handling in GOMP_offload_unregister_ver,
i.e. the 'target_data = &((void **) data)[1];' bit,
is obvious and a good bug fix.
Regarding the renaming of (..._)'target_data' to (..._)'data',
I do not have a real opinion as I also regard the requires part
as being part of the target(-related) data. Thus, I don't think
it harms but I also do not think that there is a large benefit.
Regarding the assert ... (continued below)
gcc/
* config/gcn/mkoffload.cc (process_obj): Clarify 'target_data' ->
'[...]_data'.
* config/nvptx/mkoffload.cc (process): Likewise.
libgomp/
* target.c (GOMP_offload_register_ver): Clarify 'target_data' ->
'data'.
(GOMP_offload_unregister_ver): Likewise. Fix up 'target_data',
and add 'assert'.
...
diff --git a/gcc/config/gcn/mkoffload.cc b/gcc/config/gcn/mkoffload.cc
index b8b3fecfcb4..d2464332275 100644
--- a/gcc/config/gcn/mkoffload.cc
+++ b/gcc/config/gcn/mkoffload.cc
@@ -692,13 +692,13 @@ process_obj (FILE *in, FILE *cfile, uint32_t omp_requires)
len);
fprintf (cfile,
- "static const struct gcn_image_desc {\n"
+ "static const struct gcn_data {\n"
" uintptr_t omp_requires_mask;\n"
" const struct gcn_image *gcn_image;\n"
" unsigned kernel_count;\n"
" const struct hsa_kernel_description *kernel_infos;\n"
" unsigned global_variable_count;\n"
- "} target_data = {\n"
+ "} gcn_data = {\n"
" %d,\n"
" &gcn_image,\n"
" sizeof (gcn_kernels) / sizeof (gcn_kernels[0]),\n"
@@ -723,7 +723,7 @@ process_obj (FILE *in, FILE *cfile, uint32_t omp_requires)
fprintf (cfile, "static __attribute__((constructor)) void init (void)\n"
"{\n"
" GOMP_offload_register_ver (%#x, __OFFLOAD_TABLE__,"
- " %d/*GCN*/, &target_data);\n"
+ " %d/*GCN*/, &gcn_data);\n"
"};\n",
GOMP_VERSION_PACK (GOMP_VERSION, GOMP_VERSION_GCN),
GOMP_DEVICE_GCN);
@@ -731,7 +731,7 @@ process_obj (FILE *in, FILE *cfile, uint32_t omp_requires)
fprintf (cfile, "static __attribute__((destructor)) void fini (void)\n"
"{\n"
" GOMP_offload_unregister_ver (%#x, __OFFLOAD_TABLE__,"
- " %d/*GCN*/, &target_data);\n"
+ " %d/*GCN*/, &gcn_data);\n"
"};\n",
GOMP_VERSION_PACK (GOMP_VERSION, GOMP_VERSION_GCN),
GOMP_DEVICE_GCN);
diff --git a/gcc/config/nvptx/mkoffload.cc b/gcc/config/nvptx/mkoffload.cc
index d8c81eb0547..0fa5f4423bf 100644
--- a/gcc/config/nvptx/mkoffload.cc
+++ b/gcc/config/nvptx/mkoffload.cc
@@ -310,7 +310,7 @@ process (FILE *in, FILE *out, uint32_t omp_requires)
fprintf (out, "\n};\n\n");
fprintf (out,
- "static const struct nvptx_tdata {\n"
+ "static const struct nvptx_data {\n"
" uintptr_t omp_requires_mask;\n"
" const struct ptx_obj *ptx_objs;\n"
" unsigned ptx_num;\n"
@@ -318,7 +318,7 @@ process (FILE *in, FILE *out, uint32_t omp_requires)
" unsigned var_num;\n"
" const struct nvptx_fn *fn_names;\n"
" unsigned fn_num;\n"
- "} target_data = {\n"
+ "} nvptx_data = {\n"
" %d, ptx_objs, sizeof (ptx_objs) / sizeof (ptx_objs[0]),\n"
" var_mappings,"
" sizeof (var_mappings) / sizeof (var_mappings[0]),\n"
@@ -344,7 +344,7 @@ process (FILE *in, FILE *out, uint32_t omp_requires)
fprintf (out, "static __attribute__((constructor)) void init (void)\n"
"{\n"
" GOMP_offload_register_ver (%#x, __OFFLOAD_TABLE__,"
- " %d/*NVIDIA_PTX*/, &target_data);\n"
+ " %d/*NVIDIA_PTX*/, &nvptx_data);\n"
"};\n",
GOMP_VERSION_PACK (GOMP_VERSION, GOMP_VERSION_NVIDIA_PTX),
GOMP_DEVICE_NVIDIA_PTX);
@@ -352,7 +352,7 @@ process (FILE *in, FILE *out, uint32_t omp_requires)
fprintf (out, "static __attribute__((destructor)) void fini (void)\n"
"{\n"
" GOMP_offload_unregister_ver (%#x, __OFFLOAD_TABLE__,"
- " %d/*NVIDIA_PTX*/, &target_data);\n"
+ " %d/*NVIDIA_PTX*/, &nvptx_data);\n"
"};\n",
GOMP_VERSION_PACK (GOMP_VERSION, GOMP_VERSION_NVIDIA_PTX),
GOMP_DEVICE_NVIDIA_PTX);
diff --git a/libgomp/target.c b/libgomp/target.c
index 4dac81862d7..288b748b9e8 100644
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -2334,23 +2334,29 @@ gomp_requires_to_name (char *buf, size_t size, int
requires_mask)
/* This function should be called from every offload image while loading.
It gets the descriptor of the host func and var tables HOST_TABLE, TYPE of
- the target, and TARGET_DATA needed by target plugin. */
+ the target, and DATA. */
void
GOMP_offload_register_ver (unsigned version, const void *host_table,
- int target_type, const void *target_data)
+ int target_type, const void *data)
{
int i;
- int omp_req = 0;
if (GOMP_VERSION_LIB (version) > GOMP_VERSION)
gomp_fatal ("Library too old for offload (version %u < %u)",
GOMP_VERSION, GOMP_VERSION_LIB (version));
+ int omp_req;
+ const void *target_data;
if (GOMP_VERSION_LIB (version) > 1)
{
- omp_req = (int) (size_t) ((void **) target_data)[0];
- target_data = &((void **) target_data)[1];
+ omp_req = (int) (size_t) ((void **) data)[0];
+ target_data = &((void **) data)[1];
+ }
+ else
+ {
+ omp_req = 0;
+ target_data = data;
}
gomp_mutex_lock (®ister_lock);
@@ -2413,14 +2419,24 @@ GOMP_offload_register (const void *host_table, int
target_type,
/* This function should be called from every offload image while unloading.
It gets the descriptor of the host func and var tables HOST_TABLE, TYPE of
- the target, and TARGET_DATA needed by target plugin. */
+ the target, and DATA. */
void
GOMP_offload_unregister_ver (unsigned version, const void *host_table,
- int target_type, const void *target_data)
+ int target_type, const void *data)
{
int i;
+ if (GOMP_VERSION_LIB (version) > GOMP_VERSION)
+ gomp_fatal ("Library too old for offload (version %u < %u)",
+ GOMP_VERSION, GOMP_VERSION_LIB (version));
+
+ const void *target_data;
+ if (GOMP_VERSION_LIB (version) > 1)
+ target_data = &((void **) data)[1];
+ else
+ target_data = data;
+
gomp_mutex_lock (®ister_lock);
/* Unload image from all initialized devices. */
@@ -2436,12 +2452,15 @@ GOMP_offload_unregister_ver (unsigned version, const
void *host_table,
}
/* Remove image from array of pending images. */
+ bool found = false;
for (i = 0; i < num_offload_images; i++)
if (offload_images[i].target_data == target_data)
{
offload_images[i] = offload_images[--num_offload_images];
+ found = true;
break;
}
+ assert (found);
gomp_mutex_unlock (®ister_lock);
}
... I don't like that libgomp crashes without any helpful message in that case.
In my opinion:
* Either we assume that it is unlikely to occur - ignore it.
(Matches the current implementation: do nothing.)
* Or we want to have some diagnostic in case it occurs. But in that case,
it should be some explicit diagnostic printed by gomp_error or gomp_fatal.
IMHO, gomp_error is better than gomp_fatal as libgomp then continues cleaning
up after this error, which IMHO makes more sense that just aborting.
To conclude, I think the '&((void **) data)[1]' shall go in - it is an obvious
bugfix! While the 'data' change is IMHO fine (but not doing it is also fine.
And, for the assert change, I think it shouldn't be done - either keep it as is
or use a gomp_error instead.
Thanks,
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
