Restore 'GOMP_offload_unregister_ver' functionality (was: [Patch][v5] OpenMP: Move omp requires checks to libgomp)

  Hi!

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.

> - 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.  OK to push the attached
"Restore 'GOMP_offload_unregister_ver' functionality"?  (Currently
testing.)

> (and no longer a weak variable)

... which actually removed my "contribution" (hack!) to this patch.  ;-)


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
  

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 (&register_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 (&register_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 (&register_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 (&register_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 (&register_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