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

[Tobias Burnus <tobias@codesourcery.com>]

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