Fix one issue in OpenMP 'requires' directive diagnostics (was: [Patch][v5] OpenMP: Move omp requires checks to libgomp)

  Hi!

On 2022-07-01T23:08:16+0200, Tobias Burnus <tobias@codesourcery.com> wrote:
> Updated version attached – I hope I got everything right, but I start to
> get tired, I am not 100% sure.

..., and so the obligatory copy'n'past-o ;-) crept in:

> --- a/gcc/lto-cgraph.cc
> +++ b/gcc/lto-cgraph.cc

> @@ -1773,6 +1804,10 @@ input_offload_tables (bool do_force_output)
>    struct lto_file_decl_data **file_data_vec = lto_get_file_decl_data ();
>    struct lto_file_decl_data *file_data;
>    unsigned int j = 0;
> +  const char *requires_fn = NULL;
> +  tree requires_decl = NULL_TREE;
> +
> +  omp_requires_mask = (omp_requires) 0;
>
>    while ((file_data = file_data_vec[j++]))
>      {
> @@ -1784,6 +1819,7 @@ input_offload_tables (bool do_force_output)
>        if (!ib)
>       continue;
>
> +      tree tmp_decl = NULL_TREE;
>        enum LTO_symtab_tags tag
>       = streamer_read_enum (ib, LTO_symtab_tags, LTO_symtab_last_tag);
>        while (tag)
> @@ -1799,6 +1835,7 @@ input_offload_tables (bool do_force_output)
>                LTO mode.  */
>             if (do_force_output)
>               cgraph_node::get (fn_decl)->mark_force_output ();
> +           tmp_decl = fn_decl;
>           }
>         else if (tag == LTO_symtab_variable)
>           {
> @@ -1810,6 +1847,72 @@ input_offload_tables (bool do_force_output)
>                may be no refs to var_decl in offload LTO mode.  */
>             if (do_force_output)
>               varpool_node::get (var_decl)->force_output = 1;
> +           tmp_decl = var_decl;
> +         }
> +       else if (tag == LTO_symtab_edge)
> +         {
> +           static bool error_emitted = false;
> +           HOST_WIDE_INT val = streamer_read_hwi (ib);
> +
> +           if (omp_requires_mask == 0)
> +             {
> +               omp_requires_mask = (omp_requires) val;
> +               requires_decl = tmp_decl;
> +               requires_fn = file_data->file_name;
> +             }
> +           else if (omp_requires_mask != val && !error_emitted)
> +             {
> +               const char *fn1 = requires_fn;
> +               if (requires_decl != NULL_TREE)
> +                 {
> +                   while (DECL_CONTEXT (requires_decl) != NULL_TREE
> +                          && TREE_CODE (requires_decl) != TRANSLATION_UNIT_DECL)
> +                     requires_decl = DECL_CONTEXT (requires_decl);
> +                   if (requires_decl != NULL_TREE)
> +                     fn1 = IDENTIFIER_POINTER (DECL_NAME (requires_decl));
> +                 }
> +
> +               const char *fn2 = file_data->file_name;
> +               if (tmp_decl != NULL_TREE)
> +                 {
> +                   while (DECL_CONTEXT (tmp_decl) != NULL_TREE
> +                          && TREE_CODE (tmp_decl) != TRANSLATION_UNIT_DECL)
> +                     tmp_decl = DECL_CONTEXT (tmp_decl);
> +                   if (tmp_decl != NULL_TREE)
> +                     fn2 = IDENTIFIER_POINTER (DECL_NAME (requires_decl));
> +                 }

... here: tmp_decl' not 'requires_decl'.  OK to push the attached
"Fix one issue in OpenMP 'requires' directive diagnostics"?

I'd even push that one "as obvious", but thought I'd ask whether you
maybe have a quick idea about the XFAILs that I'm adding?  (I'm otherwise
not planning on resolving that issue at this time.)

> +
> +               char buf1[sizeof ("unified_address, unified_shared_memory, "
> +                                 "reverse_offload")];
> +               char buf2[sizeof ("unified_address, unified_shared_memory, "
> +                                 "reverse_offload")];
> +               omp_requires_to_name (buf2, sizeof (buf2),
> +                                     val != OMP_REQUIRES_TARGET_USED
> +                                     ? val
> +                                     : (HOST_WIDE_INT) omp_requires_mask);
> +               if (val != OMP_REQUIRES_TARGET_USED
> +                   && omp_requires_mask != OMP_REQUIRES_TARGET_USED)
> +                 {
> +                   omp_requires_to_name (buf1, sizeof (buf1),
> +                                         omp_requires_mask);
> +                   error ("OpenMP %<requires%> directive with non-identical "
> +                          "clauses in multiple compilation units: %qs vs. "
> +                          "%qs", buf1, buf2);
> +                   inform (UNKNOWN_LOCATION, "%qs has %qs", fn1, buf1);
> +                   inform (UNKNOWN_LOCATION, "%qs has %qs", fn2, buf2);
> +                 }
> +               else
> +                 {
> +                   error ("OpenMP %<requires%> directive with %qs specified "
> +                          "only in some compilation units", buf2);
> +                   inform (UNKNOWN_LOCATION, "%qs has %qs",
> +                           val != OMP_REQUIRES_TARGET_USED ? fn2 : fn1,
> +                           buf2);
> +                   inform (UNKNOWN_LOCATION, "but %qs has not",
> +                           val != OMP_REQUIRES_TARGET_USED ? fn1 : fn2);
> +                 }
> +               error_emitted = true;
> +             }
>           }
>         else
>           fatal_error (input_location,


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 07.07.22 15:26, Thomas Schwinge wrote:
> On 2022-07-01T23:08:16+0200, Tobias Burnus <tobias@codesourcery.com>
> wrote:
>> Updated version attached – I hope I got everything right, but I start to
>> get tired, I am not 100% sure.
> ..., and so the obligatory copy'n'past-o;-)  crept in:
...
>> +                  if (tmp_decl != NULL_TREE)
>> +                    fn2 = IDENTIFIER_POINTER (DECL_NAME (requires_decl));
>> +                }
> ... here: tmp_decl' not 'requires_decl'.  OK to push the attached
> "Fix one issue in OpenMP 'requires' directive diagnostics"?
Good that you spotted it and thanks for testing + fixing it!
> I'd even push that one "as obvious", but thought I'd ask whether you
> maybe have a quick idea about the XFAILs that I'm adding?  (I'm otherwise
> not planning on resolving that issue at this time.)

(This question relates to what's printed if there is no TRANSLATION_UNIT_DECL.)

  * * *

Pre-remark - the code does:

* If there is any offload_func or offload_var DECL in the TU, it uses
   that one for diagnostic.
   This is always the case if there is a 'declare target' or 'omp target'
   but not when there is only 'omp target data'.
   → In real-world code it likely has a proper name.

* Otherwise, it takes the file name of file_data->file_name.

With -save-temps, that's based on the input files, which
gives a useful output.

When using
   gcc -c *.c
   gcc *.o
the file name is <inputfile>.o - which is also quite useful.

However, when doing
   gcc *.c
which combines compiling and linking in one step, the filename
is /tmp/cc*.o which is not that helpful.

There is no real way to avoid this, unless we explicitly store the
filename or some location_t for the 'requires'. But the used
LTO writer does not support it directly. It can be fixed, but
requires some re-organization and increased intermittent .o file
size. (Cf. https://gcc.gnu.org/pipermail/gcc-patches/2022-June/597496.html
for what it needed and why it does not work.)

However, in the real world, there should be usually a proper message as
(a) It is unlikely to have code which only does 'omp target ... data'
     transfer - and no 'omp target'
and
(b) for larger code, separating compilation and linking is common
     while for smaller code, 'requires' mismatches is less likely and
     also easier to find the file causing the issue.

Still, like always, having a nice diagnostic would not harm :-)

  * * *

> Subject: [PATCH] Fix one issue in OpenMP 'requires' directive diagnostics
>
> Fix-up for recent commit 683f11843974f0bdf42f79cdcbb0c2b43c7b81b0
> "OpenMP: Move omp requires checks to libgomp".
>
>       gcc/
>       * lto-cgraph.cc (input_offload_tables) <LTO_symtab_edge>: Correct
>       'fn2' computation.
>       libgomp/
>       * testsuite/libgomp.c-c++-common/requires-1.c: Add 'dg-note's.
>       * testsuite/libgomp.c-c++-common/requires-2.c: Likewise.
>       * testsuite/libgomp.c-c++-common/requires-3.c: Likewise.
>       * testsuite/libgomp.c-c++-common/requires-7.c: Likewise.
>       * testsuite/libgomp.fortran/requires-1.f90: Likewise.

Regarding the patch, it adds 'dg-note' for the location data - and fixes the wrong-decl-use bug.

Those LGTM - Thanks!

Regarding the xfail part:

> --- a/libgomp/testsuite/libgomp.c-c++-common/requires-7.c
...
>   -/* { dg-error "OpenMP 'requires' directive with non-identical clauses in multiple compilation units: 'unified_shared_memory' vs. 'unified_address'" "" { target *-*-* } 0 }  */
> +/* { dg-error "OpenMP 'requires' directive with non-identical clauses in multiple compilation units: 'unified_shared_memory' vs. 'unified_address'" "" { target *-*-* } 0 }
> +     { dg-note {requires-7\.c' has 'unified_shared_memory'} {} { target *-*-* } 0 }
> +     TODO There is some issue that we're not seeing the source file name here (but a temporary '*.o' instead):
> +     { dg-note {requires-7-aux\.c' has 'unified_address'} {} { xfail *-*-* } 0 }
> +     ..., so verify that at least the rest of the diagnostic is correct:
> +     { dg-note {' has 'unified_address'} {} { target *-*-* } 0 } */

The requires-7-aux.c file uses, on purpose, only 'omp target enter data'
to trigger the .o name in 'inform' as no decl is written to
offload_func/offload_vars for that TU.

As the testsuite compiles+links the two requires-7*.c files in one step
and is invoked without -save-temps, the used object file names will be
/tmp/cc*.o.

* * *

Regarding the xfail: I think it is fine to have this xfail, but as it is
clear why inform points to /tmp/cc*.o, you could reword the TODO to
state why it goes wrong.

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!

On 2022-07-07T15:56:28+0200, Tobias Burnus <tobias@codesourcery.com> wrote:
> On 07.07.22 15:26, Thomas Schwinge wrote:
>> On 2022-07-01T23:08:16+0200, Tobias Burnus <tobias@codesourcery.com>
>> wrote:
>>> Updated version attached – I hope I got everything right, but I start to
>>> get tired, I am not 100% sure.
>> ..., and so the obligatory copy'n'past-o;-)  crept in:
> ...
>>> +                 if (tmp_decl != NULL_TREE)
>>> +                   fn2 = IDENTIFIER_POINTER (DECL_NAME (requires_decl));
>>> +               }
>> ... here: tmp_decl' not 'requires_decl'.  OK to push the attached
>> "Fix one issue in OpenMP 'requires' directive diagnostics"?
> Good that you spotted it and thanks for testing + fixing it!
>> I'd even push that one "as obvious", but thought I'd ask whether you
>> maybe have a quick idea about the XFAILs that I'm adding?  (I'm otherwise
>> not planning on resolving that issue at this time.)
>
> (This question relates to what's printed if there is no TRANSLATION_UNIT_DECL.)
> [...]

Thanks for the explanation, makes sense.

> Regarding the xfail: I think it is fine to have this xfail, but as it is
> clear why inform points to /tmp/cc*.o, you could reword the TODO to
> state why it goes wrong.

Done:

    TODO We're currently not streaming location information for the OpenMP
    directives used in 'requires-7-aux.c', so we're not seeing the source file
    name here (but a temporary '*.o' instead; for details, see
    <https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598011.html>):
    { dg-note {requires-7-aux\.c' has 'unified_address'} {} { xfail *-*-* } 0 }
    ..., but we may still verify that the rest of the diagnostic is correct:
    { dg-note {' has 'unified_address'} {} { target *-*-* } 0 }

With just that changed, I've pushed to master branch
commit faa0c328ee65f0d6d65d6e20181d26e336071919
"Fix one issue in OpenMP 'requires' directive diagnostics", 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