| Message ID | 87czgwkp8t.fsf@euler.schwinge.homeip.net |
|---|---|
| State | New |
| Headers | show |
| Series | Make 'c-c++-common/goacc/kernels-decompose-pr100400-1-*.c' behave consistently, regardless of checking level (was: GCC 12.1 Release Candidate available from gcc.gnu.org) | expand |
On Mon, May 2, 2022 at 4:01 PM Thomas Schwinge <thomas@codesourcery.com> wrote: > > Hi! > > 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). That said, having a testcase that checks for an ICE as a TODO is maybe not the very best thing to have. We have bugzilla for unfixed bugs. 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
Hi Richard! 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". 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? > That said, having a testcase that checks for an ICE as a TODO is maybe not > the very best thing to have. We have bugzilla for unfixed bugs. As per the past 'dg-ice' discussions, there's certainly value seen for having such test cases (and it already did help me during development, elsewhere). I do agree/acknowledge that it's a bit more difficult to make those behave consistently across GCC configurations. 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 Tue, May 3, 2022 at 10:16 AM Thomas Schwinge <thomas@codesourcery.com> wrote: > > Hi Richard! > > 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. > 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? > > > > That said, having a testcase that checks for an ICE as a TODO is maybe not > > the very best thing to have. We have bugzilla for unfixed bugs. > > As per the past 'dg-ice' discussions, there's certainly value seen for > having such test cases (and it already did help me during development, > elsewhere). > > I do agree/acknowledge that it's a bit more difficult to make those > behave consistently across GCC configurations. So maybe do internal_error ("PRnnnn"); then? > > 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 7827d018a9e0109b8d271ceb824f2c718bae508e 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 'abort', 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 'abort'-like diagnostic, so explicitly call 'abort' 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>: Reliably 'abort'. 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(-) diff --git a/gcc/omp-oacc-kernels-decompose.cc b/gcc/omp-oacc-kernels-decompose.cc index 4386787ba3c..9f5c1ecb255 100644 --- a/gcc/omp-oacc-kernels-decompose.cc +++ 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 { tree clauses = gimple_omp_for_clauses (stmt); diff --git a/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-2.c b/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-2.c index a643f109bf1..8b65e07c623 100644 --- a/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-2.c +++ b/gcc/testsuite/c-c++-common/goacc/kernels-decompose-pr100400-1-2.c @@ -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.35.1