From: Thomas Schwinge <thomas@codesourcery.com>
To: Tobias Burnus <tobias@codesourcery.com>, <gcc-patches@gcc.gnu.org>
Cc: Jakub Jelinek <jakub@redhat.com>
Subject: Re: Restore 'GOMP_offload_unregister_ver' functionality (was: [Patch][v5] OpenMP: Move omp requires checks to libgomp)
Date: Wed, 6 Jul 2022 23:08:03 +0200 [thread overview]
Message-ID: <87y1x6rlu4.fsf@dirichlet.schwinge.homeip.net> (raw)
In-Reply-To: <1f1aea0a-dbe8-7b95-6bee-9d157faa320e@codesourcery.com>
[-- Attachment #1: Type: text/plain, Size: 2979 bytes --]
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
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Restore-GOMP_offload_unregister_ver-functionality.patch --]
[-- Type: text/x-diff, Size: 6404 bytes --]
From 3f05e03d6cfdf723ca0556318b6a9aa37be438e7 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'.
---
gcc/config/gcn/mkoffload.cc | 8 ++++----
gcc/config/nvptx/mkoffload.cc | 8 ++++----
libgomp/target.c | 30 +++++++++++++++++++++++-------
3 files changed, 31 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..c66c61b0621 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. */
--
2.35.1
next prev parent reply other threads:[~2022-07-06 21:08 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-13 15:07 [PATCH, OpenMP 5.0] More implementation of the requires directive Chung-Lin Tang
2021-01-13 15:27 ` Jakub Jelinek
2021-03-25 11:18 ` Thomas Schwinge
2022-03-29 13:42 ` Andrew Stubbs
2022-06-08 3:56 ` [Patch] OpenMP: Move omp requires checks to libgomp Tobias Burnus
2022-06-09 11:40 ` Jakub Jelinek
2022-06-09 12:46 ` Tobias Burnus
2022-06-09 14:19 ` Jakub Jelinek
2022-06-29 14:33 ` [Patch][v4] " Tobias Burnus
2022-06-29 17:02 ` Jakub Jelinek
2022-06-29 18:10 ` Tobias Burnus
2022-06-29 20:18 ` Jakub Jelinek
2022-07-01 13:06 ` [Patch][v5] " Tobias Burnus
2022-07-01 14:34 ` Jakub Jelinek
2022-07-01 16:31 ` Tobias Burnus
2022-07-01 16:55 ` Jakub Jelinek
2022-07-01 21:08 ` Tobias Burnus
2022-07-04 8:31 ` Jakub Jelinek
2022-07-07 13:26 ` Fix one issue in OpenMP 'requires' directive diagnostics (was: [Patch][v5] OpenMP: Move omp requires checks to libgomp) Thomas Schwinge
2022-07-07 13:56 ` Tobias Burnus
2022-07-08 6:59 ` Thomas Schwinge
2022-07-06 10:42 ` Restore 'GOMP_offload_unregister_ver' functionality " Thomas Schwinge
2022-07-06 13:59 ` Tobias Burnus
2022-07-06 21:08 ` Thomas Schwinge [this message]
2022-08-17 11:45 ` Jakub Jelinek
2023-09-15 9:41 ` [Patch][v5] OpenMP: Move omp requires checks to libgomp Thomas Schwinge
2022-07-07 8:37 ` Adjust 'libgomp.c-c++-common/requires-3.c' (was: [Patch][v4] OpenMP: Move omp requires checks to libgomp) Thomas Schwinge
2022-07-07 9:02 ` Tobias Burnus
2022-07-07 8:42 ` Enhance 'libgomp.c-c++-common/requires-4.c', 'libgomp.c-c++-common/requires-5.c' testing " Thomas Schwinge
2022-07-07 9:36 ` Tobias Burnus
2022-07-07 10:42 ` Thomas Schwinge
2022-07-06 10:30 ` Define 'OMP_REQUIRES_[...]', 'GOMP_REQUIRES_[...]' in a single place (was: [Patch] " Thomas Schwinge
2022-07-06 13:40 ` Tobias Burnus
2022-07-06 11:04 ` Fix Intel MIC 'mkoffload' for OpenMP 'requires' " Thomas Schwinge
2022-07-06 11:29 ` Tobias Burnus
2022-07-06 12:38 ` Thomas Schwinge
2022-07-06 13:30 ` Tobias Burnus
2022-07-07 10:46 ` Thomas Schwinge
2022-07-06 14:19 ` Tobias Burnus
2024-03-07 12:38 ` nvptx: 'cuDeviceGetCount' failure is fatal " Thomas Schwinge
2024-03-07 14:28 ` nvptx: 'cuDeviceGetCount' failure is fatal Tobias Burnus
2024-03-08 15:58 ` Thomas Schwinge
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87y1x6rlu4.fsf@dirichlet.schwinge.homeip.net \
--to=thomas@codesourcery.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=tobias@codesourcery.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).