Fix Intel MIC 'mkoffload' for OpenMP 'requires' (was: [Patch] OpenMP: Move omp requires checks to libgomp)

Hi!

On 2022-06-08T05:56:02+0200, Tobias Burnus <Tobias_Burnus@mentor.com> wrote:
> This is based on Chung-Lin's patch at https://gcc.gnu.org/pipermail/gcc-patches/2021-January/563393.html

> PS: I have not fully tested the intelmic version.

As part of my standard testing, I'm reporting that it got completely
broken.  ;'-) Your commit 683f11843974f0bdf42f79cdcbb0c2b43c7b81b0
"OpenMP: Move omp requires checks to libgomp" states:
"When the device lto1 runs, it extracts the data for mkoffload. The
latter than passes the value on to GOMP_offload_register_ver."
That's not implemented for Intel MIC 'mkoffload', so we always run into
'gcc/lto-cgraph.cc:input_offload_tables':

    +#ifdef ACCEL_COMPILER
    +  char *omp_requires_file = getenv ("GCC_OFFLOAD_OMP_REQUIRES_FILE");
    +  if (omp_requires_file == NULL || omp_requires_file[0] == '\0')
    +    fatal_error (input_location, "GCC_OFFLOAD_OMP_REQUIRES_FILE unset");

..., and all offloading compilation fail with that 'fatal_error'.  OK to
push the attached "Fix Intel MIC 'mkoffload' for OpenMP 'requires'"?
(Currently testing.)


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,

On 06.07.22 13:04, Thomas Schwinge wrote:
> On 2022-06-08T05:56:02+0200, Tobias Burnus <Tobias_Burnus@mentor.com> wrote:
>> PS: I have not fully tested the intelmic version.
> As part of my standard testing, I'm reporting that it got completely
> broken.  ;'-)

Interesting. Because intelmic-mkoffload.cc calls GOMP_offload_register
and not GOMP_offload_register_ver - and that call path should be unchanged.

However, I missed that I had an assert that GCC_OFFLOAD_OMP_REQUIRES_FILE is
set. - Thus, an alternative is to change that into an 'if'.

But I concur that updating intelmic-mkoffload.cc is nicer! Thanks!


Regarding:
> -! { dg-do link { target { offload_target_nvptx || offload_target_amdgcn } } }
> +! { dg-do link { target offloading_enabled } }
This patch looks wrong. We are not interested whether there is an offloading device
available or not - but whether the offloading compiler is running.

Those are completely independent. Obviously, offloading can be configured but not
being present. (That's the usual case for testing distro builds but also can
occur elsewhere.)
And also the reverse if possible - usually because of -foffload=... but when GCC is
configured with --enable-offload-defaulted, also other combinations are possible.


I think the proper check would be write and use an 'offload_target_any',
i.e. OFFLOAD_TARGET_NAMES= being present and nonempty.

Cf. check_effective_target_offload_target_nvptx / ..._amdgcn and
libgomp_check_effective_target_offload_target
in libgomp/testsuite/lib/libgomp.exp

Possible patch (untested):

# Return 1 if compiling for some offload target(s)
proc check_effective_target_offload_target_any { } {
     return [libgomp_check_effective_target_offload_target ""]
}

At least if I understand the following correctly, "" should work:
         return [string match "*:$target_name*:*" ":$gcc_offload_targets:"]


Thanks for taking care of the patch fallout!

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-06T13:29:14+0200, Tobias Burnus <tobias@codesourcery.com> wrote:
> On 06.07.22 13:04, Thomas Schwinge wrote:
>> On 2022-06-08T05:56:02+0200, Tobias Burnus <Tobias_Burnus@mentor.com> wrote:
>>> PS: I have not fully tested the intelmic version.
>> As part of my standard testing, I'm reporting that it got completely
>> broken.  ;'-)
>
> Interesting. Because intelmic-mkoffload.cc calls GOMP_offload_register
> and not GOMP_offload_register_ver - and that call path should be unchanged.

True indeed for that code path...

> However, I missed that I had an assert that GCC_OFFLOAD_OMP_REQUIRES_FILE is
> set.

..., but not for that one.

> Thus, an alternative is to change that into an 'if'.
> But I concur that updating intelmic-mkoffload.cc is nicer! Thanks!

ACK.


> Regarding:
>> -! { dg-do link { target { offload_target_nvptx || offload_target_amdgcn } } }
>> +! { dg-do link { target offloading_enabled } }
> This patch looks wrong. We are not interested whether there is an offloading device
> available or not - but whether the offloading compiler is running.
>
> Those are completely independent. Obviously, offloading can be configured but not
> being present. (That's the usual case for testing distro builds but also can
> occur elsewhere.)
> And also the reverse if possible - usually because of -foffload=... but when GCC is
> configured with --enable-offload-defaulted, also other combinations are possible.
>
>
> I think the proper check would be write and use an 'offload_target_any',
> i.e. OFFLOAD_TARGET_NAMES= being present and nonempty.
>
> Cf. check_effective_target_offload_target_nvptx / ..._amdgcn and
> libgomp_check_effective_target_offload_target
> in libgomp/testsuite/lib/libgomp.exp
>
> Possible patch (untested):
>
> # Return 1 if compiling for some offload target(s)
> proc check_effective_target_offload_target_any { } {
>      return [libgomp_check_effective_target_offload_target ""]
> }
>
> At least if I understand the following correctly, "" should work:
>          return [string match "*:$target_name*:*" ":$gcc_offload_targets:"]

:-) Haha, that's actually *exactly* what I had implemented first!  But
then I realized that 'target offloading_enabled' is doing exactly that:
check that offloading compilation is configured -- not that "there is an
offloading device available or not" as you seem to understand?  Or am I
confused there?

I do however agree that (generally) replacing 'target offloading_enabled'
with a new 'target offload_target_any' would seem appropriate (as a
separate patch), because that would also do the right thing when running
libgomp testing with non-default '-foffload=[...]', including
'-foffload=disable'.

For checking "offloading device available" we'd use
'check_effective_target_offload_device[...]'.


Grüße
 Thomas


> Thanks for taking care of the patch fallout!
>
> 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 Thomas,

On 06.07.22 14:38, Thomas Schwinge wrote:
> :-) Haha, that's actually *exactly* what I had implemented first!  But
> then I realized that 'target offloading_enabled' is doing exactly that:
> check that offloading compilation is configured -- not that "there is an
> offloading device available or not" as you seem to understand?  Or am I
> confused there?

I think as you mentioned below – there is a difference. And that difference,
I explicitly maked use of:
  - libgomp.c-c++-common/requires-{1,5,7}.c test the device lto1 compiler,
    which requires that it is actually available. Thus, I used:
    /* { dg-do link { target { offload_target_nvptx || offload_target_amdgcn } } } */
while
  - libgomp.c-c++-common/requires-2.c checks that ENABLE_OFFLOADING == true, i.e.
    the host lto1 compiler is configured to create the offload tables and, thus,
    can diagnose the omp-requires mismatch. Hence, the testcase has:
    /* { dg-do link { target offloading_enabled } } */
    /* { dg-additional-options "-foffload=disable -flto" } */

Granted, as the other files do not use -foffload=..., it should not
make a difference - but, still, replacing it unconditionally
with 'target offloading_enabled' feels wrong.

I was about to write again about --enable-offload-defaulted and
having no offloading compilers installed. But that comes too late.
gcc.cc (the driver) will set
   OFFLOAD_TARGET_NAMES=
to the configured offload-target compilers (after -foffload= filtering),
but the is-available check is only done in lto-wrapper.cc, which comes
too late. But I admit it is unlikely that someone configures + builds
offload compilers and, then, for testing have no offload compilers
available.

> I do however agree that (generally) replacing 'target offloading_enabled'
> with a new 'target offload_target_any' would seem appropriate (as a
> separate patch), because that would also do the right thing when running
> libgomp testing with non-default '-foffload=[...]', including
> '-foffload=disable'.

I concur – but I would prefer to have that lib/libgomp.exp patch committed
before the testsuite-part of this patch is committed. Albeit, I do not feel
very strong about this.

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 Thomas, hello all,

On 06.07.22 13:04, Thomas Schwinge wrote:
> On 2022-06-08T05:56:02+0200, Tobias Burnus <Tobias_Burnus@mentor.com>
> wrote:
>> PS: I have not fully tested the intelmic version.
...
> Subject: [PATCH] Fix Intel MIC 'mkoffload' for OpenMP 'requires'
...
> This also means finally switching Intel MIC 'mkoffload' to
> 'GOMP_offload_register_ver', 'GOMP_offload_unregister_ver',
> making 'GOMP_offload_register', 'GOMP_offload_unregister'
> legacy entry points.
...
>       gcc/
>       * config/i386/intelmic-mkoffload.cc (generate_host_descr_file)
>       (prepare_target_image, main): Handle OpenMP 'requires'.
>       (generate_host_descr_file): Switch to 'GOMP_offload_register_ver',
>       'GOMP_offload_unregister_ver'.
>       libgomp/
>       * target.c (GOMP_offload_register, GOMP_offload_unregister):
>       Denote as legacy entry points.
>       * testsuite/libgomp.c-c++-common/requires-1.c: Enable for all
>       'target offloading_enabled'.
>       * testsuite/libgomp.c-c++-common/requires-5.c: Likewise.
>       * testsuite/libgomp.c-c++-common/requires-7.c: Likewise.
>       * testsuite/libgomp.fortran/requires-1.f90: Likewise.
...
> diff --git a/gcc/config/i386/intelmic-mkoffload.cc b/gcc/config/i386/intelmic-mkoffload.cc
> index c683d6f473e..596f6f107b8 100644
> --- a/gcc/config/i386/intelmic-mkoffload.cc
> +++ b/gcc/config/i386/intelmic-mkoffload.cc
> @@ -370,7 +370,7 @@ generate_target_offloadend_file (const char *target_compiler)
>
>   /* Generates object file with the host side descriptor.  */
>   static const char *
> -generate_host_descr_file (const char *host_compiler)
> +generate_host_descr_file (const char *host_compiler, uint32_t omp_requires)
>   {
>     char *dump_filename = concat (dumppfx, "_host_descr.c", NULL);
>     const char *src_filename = save_temps
> @@ -386,39 +386,50 @@ generate_host_descr_file (const char *host_compiler)
>     if (!src_file)
>       fatal_error (input_location, "cannot open '%s'", src_filename);
>
> +  fprintf (src_file, "#include <stdint.h>\n\n");
> +
>     fprintf (src_file,
>          "extern const void *const __OFFLOAD_TABLE__;\n"
>          "extern const void *const __offload_image_intelmic_start;\n"
>          "extern const void *const __offload_image_intelmic_end;\n\n"
>
> -        "static const void *const __offload_target_data[] = {\n"
> +        "static const struct intelmic_data {\n"
> +        "  uintptr_t omp_requires_mask;\n"
> +        "  const void *const image_start;\n"
> +        "  const void *const image_end;\n"
> +        "} intelmic_data = {\n"
> +        "  %d,\n"
>          "  &__offload_image_intelmic_start, &__offload_image_intelmic_end\n"
> -        "};\n\n");
> +        "};\n\n", omp_requires);
>
>     fprintf (src_file,
>          "#ifdef __cplusplus\n"
>          "extern \"C\"\n"
>          "#endif\n"
> -        "void GOMP_offload_register (const void *, int, const void *);\n"
> +        "void GOMP_offload_register_ver (unsigned, const void *, int, const void *);\n"
This line now ends in column 90, I think you want to wrap it.
>          "#ifdef __cplusplus\n"
>          "extern \"C\"\n"
>          "#endif\n"
> -        "void GOMP_offload_unregister (const void *, int, const void *);\n\n"
> +        "void GOMP_offload_unregister_ver (unsigned, const void *, int, const void *);\n\n"
Likewise - before col 80, now col 94.
>
>          "__attribute__((constructor))\n"
>          "static void\n"
>          "init (void)\n"
>          "{\n"
> -        "  GOMP_offload_register (&__OFFLOAD_TABLE__, %d, __offload_target_data);\n"
> -        "}\n\n", GOMP_DEVICE_INTEL_MIC);
> +        "  GOMP_offload_register_ver (%#x, &__OFFLOAD_TABLE__, %d, &intelmic_data);\n"
Likewise - albeit before already col 87, now col 89.
> +        "}\n\n",
> +        GOMP_VERSION_PACK (GOMP_VERSION, GOMP_VERSION_INTEL_MIC),
> +        GOMP_DEVICE_INTEL_MIC);
>
>     fprintf (src_file,
>          "__attribute__((destructor))\n"
>          "static void\n"
>          "fini (void)\n"
>          "{\n"
> -        "  GOMP_offload_unregister (&__OFFLOAD_TABLE__, %d, __offload_target_data);\n"
> -        "}\n", GOMP_DEVICE_INTEL_MIC);
>
> +        "}\n",
> +        GOMP_VERSION_PACK (GOMP_VERSION, GOMP_VERSION_INTEL_MIC),
> +        GOMP_DEVICE_INTEL_MIC);
>
>     fclose (src_file);
>
> @@ -462,7 +473,7 @@ generate_host_descr_file (const char *host_compiler)
>   }
>
>   static const char *
> -prepare_target_image (const char *target_compiler, int argc, char **argv)
> +prepare_target_image (const char *target_compiler, int argc, char **argv, uint32_t *omp_requires)
Likewise now too long.
>   {
>     const char *target_descr_filename
>       = generate_target_descr_file (target_compiler);
> @@ -509,8 +520,26 @@ prepare_target_image (const char *target_compiler, int argc, char **argv)
>     obstack_ptr_grow (&argv_obstack, "");
>     obstack_ptr_grow (&argv_obstack, "-o");
>     obstack_ptr_grow (&argv_obstack, target_so_filename);
> +
> +  char *omp_requires_file;
> +  if (save_temps)
> +    omp_requires_file = concat (dumppfx, ".mkoffload.omp_requires", NULL);
> +  else
> +    omp_requires_file = make_temp_file (".mkoffload.omp_requires");
> +  xputenv (concat ("GCC_OFFLOAD_OMP_REQUIRES_FILE=", omp_requires_file, NULL));
> +
>     compile_for_target (&argv_obstack);
>
> +  unsetenv("GCC_OFFLOAD_OMP_REQUIRES_FILE");
> +  FILE *in = fopen (omp_requires_file, "rb");
> +  if (!in)
> +    fatal_error (input_location, "cannot open omp_requires file %qs",
> +              omp_requires_file);
> +  if (fread (omp_requires, sizeof (*omp_requires), 1, in) != 1)
> +    fatal_error (input_location, "cannot read omp_requires file %qs",
> +              omp_requires_file);
> +  fclose (in);
> +
>     /* Run objcopy.  */
>     char *rename_section_opt
>       = XALLOCAVEC (char, sizeof (".data=") + strlen (image_section_name));
> @@ -643,10 +672,13 @@ main (int argc, char **argv)
>     if (!dumppfx)
>       dumppfx = out_obj_filename;
>
> +  uint32_t omp_requires;
> +
>     const char *target_so_filename
> -    = prepare_target_image (target_compiler, argc, argv);
> +    = prepare_target_image (target_compiler, argc, argv, &omp_requires);
>
> -  const char *host_descr_filename = generate_host_descr_file (host_compiler);
> +  const char *host_descr_filename
> +    = generate_host_descr_file (host_compiler, omp_requires);
>
>     /* Perform partial linking for the target image and host side descriptor.
>        As a result we'll get a finalized object file with all offload data.  */
> diff --git a/libgomp/target.c b/libgomp/target.c
> index 288b748b9e8..18c5f6e27db 100644
> --- a/libgomp/target.c
> +++ b/libgomp/target.c
> @@ -2410,6 +2410,8 @@ GOMP_offload_register_ver (unsigned version, const void *host_table,
>     gomp_mutex_unlock (&register_lock);
>   }
>
> +/* Legacy entry point.  */
> +
>   void
>   GOMP_offload_register (const void *host_table, int target_type,
>                      const void *target_data)
> @@ -2465,6 +2467,8 @@ GOMP_offload_unregister_ver (unsigned version, const void *host_table,
>     gomp_mutex_unlock (&register_lock);
>   }
>
> +/* Legacy entry point.  */
> +
>   void
>   GOMP_offload_unregister (const void *host_table, int target_type,
>                        const void *target_data)

Except for the too-long lines, it LGTM up to here.

However, regarding the following, I prefer to have something like 'offload_target_any'
(or similar), instead of (ab)using 'offloading_enabled', as discussed elsewhere in this thread.

While it does not make currently any difference, I could imagine to do the same
for OpenMP as done in libgomp.oacc-*/*exp. Namely: Compile/run all 'omp target'
testcases in addition with -foffload=disable.

As we don't do this, yet, I think it is fine to apply the patch for now. However, if
you don't want to fix it as follow up (please do), can you at least open a PR about it?

> diff --git a/libgomp/testsuite/libgomp.c-c++-common/requires-1.c b/libgomp/testsuite/libgomp.c-c++-common/requires-1.c
> index fedf9779769..8eaac54e187 100644
> --- a/libgomp/testsuite/libgomp.c-c++-common/requires-1.c
> +++ b/libgomp/testsuite/libgomp.c-c++-common/requires-1.c
> @@ -1,4 +1,4 @@
> -/* { dg-do link { target { offload_target_nvptx || offload_target_amdgcn } } } */
> +/* { dg-do link { target offloading_enabled } } */
>   /* { dg-additional-sources requires-1-aux.c } */
>
>   /* Check diagnostic by device-compiler's lto1.
> diff --git a/libgomp/testsuite/libgomp.c-c++-common/requires-5.c b/libgomp/testsuite/libgomp.c-c++-common/requires-5.c
> index c1e5540cfc5..5aa04a5a604 100644
> --- a/libgomp/testsuite/libgomp.c-c++-common/requires-5.c
> +++ b/libgomp/testsuite/libgomp.c-c++-common/requires-5.c
> @@ -1,4 +1,4 @@
> -/* { dg-do run { target { offload_target_nvptx || offload_target_amdgcn } } } */
> +/* { dg-do run { target offloading_enabled } } */
>   /* { dg-additional-sources requires-5-aux.c } */
>
>   #pragma omp requires unified_shared_memory, unified_address, reverse_offload
> diff --git a/libgomp/testsuite/libgomp.c-c++-common/requires-7.c b/libgomp/testsuite/libgomp.c-c++-common/requires-7.c
> index c94a4c10846..31c6f73c6da 100644
> --- a/libgomp/testsuite/libgomp.c-c++-common/requires-7.c
> +++ b/libgomp/testsuite/libgomp.c-c++-common/requires-7.c
> @@ -1,4 +1,4 @@
> -/* { dg-do link { target { offload_target_nvptx || offload_target_amdgcn } } } */
> +/* { dg-do link { target offloading_enabled } } */
>   /* { dg-additional-sources requires-7-aux.c } */
>
>   /* Check diagnostic by device-compiler's lto1.
> diff --git a/libgomp/testsuite/libgomp.fortran/requires-1.f90 b/libgomp/testsuite/libgomp.fortran/requires-1.f90
> index 33741af15f1..1020ebb4277 100644
> --- a/libgomp/testsuite/libgomp.fortran/requires-1.f90
> +++ b/libgomp/testsuite/libgomp.fortran/requires-1.f90
> @@ -1,4 +1,4 @@
> -! { dg-do link { target { offload_target_nvptx || offload_target_amdgcn } } }
> +! { dg-do link { target offloading_enabled } }
>   ! { dg-additional-sources requires-1-aux.f90 }
>
>   ! Check diagnostic by device-compiler's lto1.

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:30:57+0200, Tobias Burnus <tobias@codesourcery.com> wrote:
> On 06.07.22 14:38, Thomas Schwinge wrote:
>> :-) Haha, that's actually *exactly* what I had implemented first!  But
>> then I realized that 'target offloading_enabled' is doing exactly that:
>> check that offloading compilation is configured -- not that "there is an
>> offloading device available or not" as you seem to understand?  Or am I
>> confused there?
>
> I think as you mentioned below – there is a difference.

Eh, thanks for un-confusing me on that aspect!  There's a reason after
all that 'offloading_enabled' lives in 'gcc/testsuite/lib/'...

> And that difference,
> I explicitly maked use of: [...]

> Granted, as the other files do not use -foffload=..., it should not
> make a difference - but, still, replacing it unconditionally
> with 'target offloading_enabled' feels wrong.

ACK!

I've pushed to master branch
commit 9ef714539cb7cc1cd746312fd5dcc987bf167471
"Fix Intel MIC 'mkoffload' for OpenMP 'requires'", 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