Message ID | 87tu7u4j4r.fsf@euler.schwinge.homeip.net |
---|---|
State | New |
Headers | show |
Series | Restore 'GOMP_offload_unregister_ver' functionality (was: [Patch][v5] OpenMP: Move omp requires checks to libgomp) | expand |
Hi Thomas, hello all, On 06.07.22 12:42, Thomas Schwinge wrote: > On 2022-07-01T15:06:05+0200, Tobias Burnus <tobias@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
Hi Tobias! On 2022-07-06T15:59:59+0200, Tobias Burnus <tobias@codesourcery.com> wrote: > On 06.07.22 12:42, Thomas Schwinge wrote: >> --- a/libgomp/target.c >> +++ b/libgomp/target.c >> /* This function should be called from every offload image while unloading. >> 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. I'd be fine to change this into a 'gomp_error', but I don't think it's necessary. Maybe that wasn't obvious (and I should add a source code comment), but my point here is that this situation really should never arise (hence, if it does: internal error, thus 'assert'). Or, in other words, such a check should've been present in the original implementation already -- and would then have flagged your patch as being incomplete in that function. Thinking about it again, shouldn't we also add a corresponding sanity-check ('assert') to 'GOMP_offload_register_ver', such that the newly registered offload image must not already be present in 'offload_images'? (Isn't that understanding also supported by the 'break' in 'if (offload_images[i].target_data == target_data)' in 'GOMP_offload_unregister_ver', as cited above: that no duplicates are expected?) That's at least my understanding of the situation; happy to hear if I'm wrong. (It's a pity that we're totally devoid of test cases for dynamic registration/unregistration of offload images...) Anyway: it's totally fine to address (or not, if so desired) this sanity-check aspect independently of the other changes, so I've backed that out, and then pushed to master branch commit 3f05e03d6cfdf723ca0556318b6a9aa37be438e7 "Restore 'GOMP_offload_unregister_ver' functionality", see attached. Grüße Thomas ----------------- 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
On Wed, Jul 06, 2022 at 03:59:59PM +0200, Tobias Burnus wrote: > > @@ -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.) I think we don't need any asserts here, nor gomp_error/gomp_fatal. The GOMP_* APIs aren't meant for direct user uses, they are to be only called by compiler generated code or support object files, and in the case of GOMP_offload_{,un}register_ver the crt file makes sure it will be registered and unregistered in pairs. Jakub
From 9a49a3e1e4d3def7b48beccdde6fa9f218719244 Mon Sep 17 00:00:00 2001 From: Thomas Schwinge <thomas@codesourcery.com> Date: Tue, 5 Jul 2022 18:23:15 +0200 Subject: [PATCH] Restore 'GOMP_offload_unregister_ver' functionality The recent commit 683f11843974f0bdf42f79cdcbb0c2b43c7b81b0 "OpenMP: Move omp requires checks to libgomp" changed the 'GOMP_offload_register_ver' interface but didn't change 'GOMP_offload_unregister_ver' accordingly, so we're no longer actually unregistering. 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'. --- gcc/config/gcn/mkoffload.cc | 8 ++++---- gcc/config/nvptx/mkoffload.cc | 8 ++++---- libgomp/target.c | 33 ++++++++++++++++++++++++++------- 3 files changed, 34 insertions(+), 15 deletions(-) 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); } -- 2.35.1