Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c' behave consistently, regardless of checking level
Commit Message
Hi Richard!
On 2022-05-03T12:53:50+0200, Richard Biener <richard.guenther@gmail.com> wrote:
> On Tue, May 3, 2022 at 10:16 AM Thomas Schwinge <thomas@codesourcery.com> wrote:
>> On 2022-05-03T09:17:52+0200, Richard Biener <richard.guenther@gmail.com> wrote:
>> > On Mon, May 2, 2022 at 4:01 PM Thomas Schwinge <thomas@codesourcery.com> wrote:
>> >> On 2022-05-01T11:02:29+0100, Iain Sandoe via Gcc <gcc@gcc.gnu.org> wrote:
>> >> >> On 29 Apr 2022, at 15:34, Jakub Jelinek via Gcc <gcc@gcc.gnu.org> wrote:
>> >> >>
>> >> >> The first release candidate for GCC 12.1 is available from
>> >> >>
>> >> >> https://gcc.gnu.org/pub/gcc/snapshots/12.1.0-RC-20220429/
>> >> >> ftp://gcc.gnu.org/pub/gcc/snapshots/12.1.0-RC-20220429/
>> >> >>
>> >> >> and shortly its mirrors. It has been generated from git commit
>> >> >> r12-8321-g621650f64fb667.
>> >>
>> >> > [...] new fails (presumably because checking is off):
>> >>
>> >> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c -std=c++98 (internal compiler error)
>> >> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c -std=c++98 (test for excess errors)
>> >> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c -std=c++14 (internal compiler error)
>> >> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c -std=c++14 (test for excess errors)
>> >> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c -std=c++17 (internal compiler error)
>> >> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c -std=c++17 (test for excess errors)
>> >> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c -std=c++20 (internal compiler error)
>> >> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c -std=c++20 (test for excess errors)
>> >>
>> >> Confirmed, and sorry. I had taken care to add explicit '-fchecking'
>> >> next to 'dg-ice', but that's in fact not the problem/cure here.
>> >> OK to push to the relevant branches the attached
>> >> "Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c' behave
>> >> consistently, regardless of checking level"?
>> >
>> > No,
>> >
>> > +++ b/gcc/omp-oacc-kernels-decompose.cc
>> > @@ -239,7 +239,13 @@ visit_loops_in_gang_single_region
>> > (gimple_stmt_iterator *gsi_p,
>> > case GIMPLE_OMP_FOR:
>> > /*TODO Given the current 'adjust_region_code' algorithm, this is
>> > actually... */
>> > +#if 0
>> > gcc_unreachable ();
>> > +#else
>> > + /* ..., but due to bugs (PR100400), we may actually come here.
>> > + Reliably catch this, regardless of checking level. */
>> > + abort ();
>> > +#endif
>> >
>> > this doesn't look correct. If you want a reliable diagnostic here please use
>> > sorry ("...") or call internal_error () manually (the IL verifiers do this).
>>
>> Hmm, I feel I'm going in circles... ;-)
>>
>> Here, 'sorry' isn't appropriate, because this is a plain bug, and not
>> "a language feature which is required by the relevant specification but
>> not implemented by GCC" ('gcc/diagnostic.cc').
>>
>> I first had this as 'internal_error', but then saw the following source
>> code comment, 'gcc/diagnostic.cc':
>>
>> /* An internal consistency check has failed. We make no attempt to
>> continue. Note that unless there is debugging value to be had from
>> a more specific message, or some other good reason, you should use
>> abort () instead of calling this function directly. */
>> void
>> internal_error (const char *gmsgid, ...)
>> {
>>
>> Here, there's no "debugging value to be had from a more specific
>> message", and I couldn't think of "some other good reason", so decided to
>> "use abort () instead of calling this function directly".
>
> I think that is misguided.
So that I know which one to fix/reconsider: does your "that" refer to the
'gcc/diagnostic.cc:internal_error' source code comment cited above, or my
interpretation of it?
>> But, if that's what we want, I'm happy to again switch to
>> 'internal_error', and then we should update this source code comment,
>> too?
> So maybe do
>
> internal_error ("PRnnnn");
>
> then?
Sure, but note that this just changes from:
[...]
during GIMPLE pass: omp_oacc_kernels_decompose
dump file: kernels-decompose-pr100400-1-3.c.008t.omp_oacc_kernels_decompose
source-gcc/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-3.c: In function ‘void foo()’:
source-gcc/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-3.c:42:7: internal compiler error: in visit_loops_in_gang_single_region, at omp-oacc-kernels-decompose.cc:247
42 | ;
| ^
[backtrace]
... to:
[...]
during GIMPLE pass: omp_oacc_kernels_decompose
dump file: kernels-decompose-pr100400-1-3.c.008t.omp_oacc_kernels_decompose
source-gcc/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-3.c: In function ‘void foo()’:
source-gcc/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-3.c:42:7: internal compiler error: PR100400
42 | ;
| ^
[backtrace]
..., and it wasn't clear to me that it's an actual improvement to
diagnose "internal compiler error: PR100400" instead of
"internal compiler error: in visit_loops_in_gang_single_region, at
omp-oacc-kernels-decompose.cc:247" (with that location pointing to
PR100400).
That's 'gcc/system.h':
#define abort() fancy_abort (__FILE__, __LINE__, __FUNCTION__)
..., plus 'gcc/diagnostic.cc':
/* Report an internal compiler error in a friendly manner. This is
the function that gets called upon use of abort() in the source
code generally, thanks to a special macro. */
void
fancy_abort (const char *file, int line, const char *function)
{
[...]
internal_error ("in %s, at %s:%d", function, trim_filename (file), line);
}
Anyway, fine by me, if you like that better? ;-P
OK then to push to the relevant branches the attached
"Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c' behave
consistently, regardless of checking level"?
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
Comments
On Tue, May 3, 2022 at 2:29 PM Thomas Schwinge <thomas@codesourcery.com> wrote:
>
> Hi Richard!
>
> On 2022-05-03T12:53:50+0200, Richard Biener <richard.guenther@gmail.com> wrote:
> > On Tue, May 3, 2022 at 10:16 AM Thomas Schwinge <thomas@codesourcery.com> wrote:
> >> On 2022-05-03T09:17:52+0200, Richard Biener <richard.guenther@gmail.com> wrote:
> >> > On Mon, May 2, 2022 at 4:01 PM Thomas Schwinge <thomas@codesourcery.com> wrote:
> >> >> On 2022-05-01T11:02:29+0100, Iain Sandoe via Gcc <gcc@gcc.gnu.org> wrote:
> >> >> >> On 29 Apr 2022, at 15:34, Jakub Jelinek via Gcc <gcc@gcc.gnu.org> wrote:
> >> >> >>
> >> >> >> The first release candidate for GCC 12.1 is available from
> >> >> >>
> >> >> >> https://gcc.gnu.org/pub/gcc/snapshots/12.1.0-RC-20220429/
> >> >> >> ftp://gcc.gnu.org/pub/gcc/snapshots/12.1.0-RC-20220429/
> >> >> >>
> >> >> >> and shortly its mirrors. It has been generated from git commit
> >> >> >> r12-8321-g621650f64fb667.
> >> >>
> >> >> > [...] new fails (presumably because checking is off):
> >> >>
> >> >> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c -std=c++98 (internal compiler error)
> >> >> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c -std=c++98 (test for excess errors)
> >> >> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c -std=c++14 (internal compiler error)
> >> >> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c -std=c++14 (test for excess errors)
> >> >> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c -std=c++17 (internal compiler error)
> >> >> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c -std=c++17 (test for excess errors)
> >> >> > XPASS: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c -std=c++20 (internal compiler error)
> >> >> > FAIL: c-c++-common/goacc/kernels-decompose-pr100400-1-2.c -std=c++20 (test for excess errors)
> >> >>
> >> >> Confirmed, and sorry. I had taken care to add explicit '-fchecking'
> >> >> next to 'dg-ice', but that's in fact not the problem/cure here.
> >> >> OK to push to the relevant branches the attached
> >> >> "Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c' behave
> >> >> consistently, regardless of checking level"?
> >> >
> >> > No,
> >> >
> >> > +++ b/gcc/omp-oacc-kernels-decompose.cc
> >> > @@ -239,7 +239,13 @@ visit_loops_in_gang_single_region
> >> > (gimple_stmt_iterator *gsi_p,
> >> > case GIMPLE_OMP_FOR:
> >> > /*TODO Given the current 'adjust_region_code' algorithm, this is
> >> > actually... */
> >> > +#if 0
> >> > gcc_unreachable ();ember all reasons, but parts of them simply
was: new and potentia
> >> > +#else
> >> > + /* ..., but due to bugs (PR100400), we may actually come here.
> >> > + Reliably catch this, regardless of checking level. */
> >> > + abort ();
> >> > +#endif
> >> >
> >> > this doesn't look correct. If you want a reliable diagnostic here please use
> >> > sorry ("...") or call internal_error () manually (the IL verifiers do this).
> >>
> >> Hmm, I feel I'm going in circles... ;-)
> >>
> >> Here, 'sorry' isn't appropriate, because this is a plain bug, and not
> >> "a language feature which is required by the relevant specification but
> >> not implemented by GCC" ('gcc/diagnostic.cc').
> >>
> >> I first had this as 'internal_error', but then saw the following source
> >> code comment, 'gcc/diagnostic.cc':
> >>
> >> /* An internal consistency check has failed. We make no attempt to
> >> continue. Note that unless there is debugging value to be had from
> >> a more specific message, or some other good reason, you should use
> >> abort () instead of calling this function directly. */
> >> void
> >> internal_error (const char *gmsgid, ...)
> >> {
> >>
> >> Here, there's no "debugging value to be had from a more specific
> >> message", and I couldn't think of "some other good reason", so decided to
> >> "use abort () instead of calling this function directly".
> >
> > I think that is misguided.
>
> So that I know which one to fix/reconsider: does your "that" refer to the
> 'gcc/diagnostic.cc:internal_error' source code comment cited above, or my
> interpretation of it?
The comment to "use abort ()".
> >> But, if that's what we want, I'm happy to again switch to
> >> 'internal_error', and then we should update this source code comment,
> >> too?
>
> > So maybe do
> >
> > internal_error ("PRnnnn");
> >
> > then?
>
> Sure, but note that this just changes from:
>
> [...]
> during GIMPLE pass: omp_oacc_kernels_decompose
> dump file: kernels-decompose-pr100400-1-3.c.008t.omp_oacc_kernels_decompose
> source-gcc/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-3.c: In function ‘void foo()’:
> source-gcc/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-3.c:42:7: internal compiler error: in visit_loops_in_gang_single_region, at omp-oacc-kernels-decompose.cc:247
> 42 | ;
> | ^
> [backtrace]
>
> ... to:
>
> [...]
> during GIMPLE pass: omp_oacc_kernels_decompose
> dump file: kernels-decompose-pr100400-1-3.c.008t.omp_oacc_kernels_decompose
> source-gcc/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-3.c: In function ‘void foo()’:
> source-gcc/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-3.c:42:7: internal compiler error: PR100400
> 42 | ;
> | ^
> [backtrace]
>
> ..., and it wasn't clear to me that it's an actual improvement to
> diagnose "internal compiler error: PR100400" instead of
> "internal compiler error: in visit_loops_in_gang_single_region, at
> omp-oacc-kernels-decompose.cc:247" (with that location pointing to
> PR100400).
Well, it should be an improvement for consistency? Ah, of course you still
get 'confused by earlier errors' then and a testsuite FAIL.
> That's 'gcc/system.h':
>
> #define abort() fancy_abort (__FILE__, __LINE__, __FUNCTION__)
>
> ..., plus 'gcc/diagnostic.cc':
>
> /* Report an internal compiler error in a friendly manner. This is
> the function that gets called upon use of abort() in the source
> code generally, thanks to a special macro. */
>
> void
> fancy_abort (const char *file, int line, const char *function)
> {
> [...]
> internal_error ("in %s, at %s:%d", function, trim_filename (file), line);
> }
>
> Anyway, fine by me, if you like that better? ;-P
>
> OK then to push to the relevant branches the attached
> "Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c' behave
> consistently, regardless of checking level"?
OK for trunk and the GCC 12 branch after the 12.1 release.
Richard.
>
> 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
From ff9e80d1aa257ec53637b14641c2b3027dbf7675 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Mon, 2 May 2022 15:15:26 +0200
Subject: [PATCH] Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c'
behave consistently, regardless of checking level
Fix-up for commit c14ea6a72fb1ae66e3d32ac8329558497c6e4403
"Catch 'GIMPLE_DEBUG' misbehavior in OpenACC 'kernels' decomposition
[PR100400, PR103836, PR104061]".
For C++ compilation of 'c-c++-common/goacc/kernels-decompose-pr100400-1-2.c',
we first emit a 'sorry' diagnostic, and then a 'gcc_unreachable' (or
'internal_error', see below) diagnostic, but for example, for
'--enable-checking=release' (thus, '!CHECKING_P'), the second one may actually
be turned into a 'confused by earlier errors, bailing out' diagnostic. (See
'gcc/diagnostic.cc:diagnostic_report_diagnostic': "When not checking, ICEs are
converted to fatal errors when an error has already occurred.") Thus, make
'c-c++-common/goacc/kernels-decompose-pr100400-1-2.c' behave consistently via
'-Wfatal-errors', and thus only matching the 'sorry' diagnostic.
For example, for '--enable-checking=no' (thus, '!ENABLE_ASSERT_CHECKING'), a
call to 'gcc_unreachable' cannot be assumed emit an 'internal_error'-like
diagnostic, so explicitly call 'internal_error' in
'gcc/omp-oacc-kernels-decompose.cc:visit_loops_in_gang_single_region', in the
'GIMPLE_OMP_FOR' case, to avoid regressing
'c-c++-common/goacc/kernels-decompose-pr100400-1-3.c', and
'c-c++-common/goacc/kernels-decompose-pr100400-1-4.c'.
PR middle-end/100400
gcc/
* omp-oacc-kernels-decompose.cc
(visit_loops_in_gang_single_region) <GIMPLE_OMP_FOR>: Explicitly
call 'internal_error'.
gcc/testsuite/
* c-c++-common/goacc/kernels-decompose-pr100400-1-2.c: Specify
'-Wfatal-errors'.
---
gcc/omp-oacc-kernels-decompose.cc | 6 ++++++
.../goacc/kernels-decompose-pr100400-1-2.c | 12 +++++-------
2 files changed, 11 insertions(+), 7 deletions(-)
@@ -239,7 +239,13 @@ visit_loops_in_gang_single_region (gimple_stmt_iterator *gsi_p,
case GIMPLE_OMP_FOR:
/*TODO Given the current 'adjust_region_code' algorithm, this is
actually... */
+#if 0
gcc_unreachable ();
+#else
+ /* ..., but due to bugs (PR100400), we may actually come here.
+ Reliably catch this, regardless of checking level. */
+ internal_error ("PR100400");
+#endif
{
tree clauses = gimple_omp_for_clauses (stmt);
@@ -1,8 +1,8 @@
/* { dg-additional-options "--param openacc-kernels=decompose" } */
-/* { dg-additional-options "-fchecking" }
- { dg-ice TODO { c++ } }
- { dg-prune-output "during GIMPLE pass: omp_oacc_kernels_decompose" } */
+/* Ensure consistent diagnostics, regardless of checking level:
+ { dg-additional-options -Wfatal-errors }
+ { dg-message {terminated due to -Wfatal-errors} TODO { target *-*-* } 0 } */
/* { dg-additional-options "-g" } */
/* { dg-additional-options "-O1" } so that we may get some 'GIMPLE_DEBUG's. */
@@ -19,18 +19,16 @@ foo (void)
/* { dg-bogus {sorry, unimplemented: 'gimple_debug' not yet supported} TODO { xfail *-*-* } .+1 } */
#pragma acc kernels /* { dg-line l_compute1 } */
/* { dg-note {OpenACC 'kernels' decomposition: variable 'p' in 'copy' clause requested to be made addressable} {} { target *-*-* } l_compute1 }
- { dg-note {variable 'p' made addressable} {} { target *-*-* xfail c++ } l_compute1 } */
+ { dg-note {variable 'p' made addressable} {} { xfail *-*-* } l_compute1 } */
/* { dg-note {variable 'c' declared in block is candidate for adjusting OpenACC privatization level} {} { xfail *-*-* } l_compute1 } */
/* { dg-note {variable 'c\.0' declared in block isn't candidate for adjusting OpenACC privatization level: not addressable} {} { xfail *-*-* } l_compute1 } */
{
- /* { dg-bogus {note: beginning 'gang-single' part in OpenACC 'kernels' region} {w/ debug} { xfail c++ } .-1 }
- { dg-bogus {note: beginning 'gang-single' part in OpenACC 'kernels' region} {w/ debug} { xfail c } .+1 } */
int c;
/* { dg-note {beginning 'gang-single' part in OpenACC 'kernels' region} {} { xfail *-*-* } .+1 } */
p = &c;
- /* { dg-note {parallelized loop nest in OpenACC 'kernels' region} {} { xfail c++ } .+1 } */
+ /* { dg-note {parallelized loop nest in OpenACC 'kernels' region} {} { xfail *-*-* } .+1 } */
#pragma acc loop independent /* { dg-line l_loop_c1 } */
/* { dg-note {variable 'c\.0' in 'private' clause isn't candidate for adjusting OpenACC privatization level: not addressable} {} { xfail *-*-* } l_loop_c1 } */
/* { dg-note {variable 'c' in 'private' clause is candidate for adjusting OpenACC privatization level} {} { xfail *-*-* } l_loop_c1 }
--
2.25.1