| Message ID | ad3c665d-1850-3bbd-2a35-d04ee88dc0cc@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] PR92090: Fix testcase failures by r276469 | expand |
On 11/3/19 8:29 PM, luoxhu wrote: > -finline-functions is enabled by default for O2 since r276469, update the > test cases with -fno-inline-functions. > > v2: disable inlining for the failed cases. Add two more failed cases > not listed in BZ. Tested on P8LE, P8BE and P9LE. > > > gcc/testsuite/ChangeLog: > > 2019-10-30 Xiong Hu Luo <luoxhu@linux.ibm.com> > > PR92090 > * g++.dg/tree-ssa/pr61034.C: Add -fno-inline-functions --param > max-inline-insns-single-O2=200. > * gcc.dg/atomic/c11-atomic-exec-5.c: Likewise. > * gcc.target/powerpc/pr72804.c: Likewie. > * gcc.target/powerpc/pr79439-1.c: Add -fno-inline-functions. > * gcc.target/powerpc/vsx-builtin-7.c: Likewise. OK jeff
Hi! On Mon, Nov 04, 2019 at 11:29:56AM +0800, luoxhu wrote: > -finline-functions is enabled by default for O2 since r276469, update the > test cases with -fno-inline-functions. Jeff already okay'ed it; just some changelog nits: > 2019-10-30 Xiong Hu Luo <luoxhu@linux.ibm.com> > > PR92090 PR other/92090 > * gcc.target/powerpc/pr72804.c: Likewie. "Likewise." Thanks! Segher
On Mon, 4 Nov 2019, luoxhu wrote: > -finline-functions is enabled by default for O2 since r276469, update the > test cases with -fno-inline-functions. > > v2: disable inlining for the failed cases. Add two more failed cases > not listed in BZ. Tested on P8LE, P8BE and P9LE. If inlining (or other interprocedural analysis) invalidates a test's intent (e.g. all the code gets optimized away), our normal approach is to use noinline etc. function attributes to prevent that inlining. If you're adding such options to work around an ICE, which certainly appears to be the case in the architecture-independent testcases here, you should (a) have comments in the tests saying explicitly that the options are there temporarily to work around the ICE in a bug whose number is given in the comment, and (b) a remark in the open regression bug for the ICE saying that those options have been added as a temporary workaround and that a patch fixing the ICE should remove them again. The commit message also needs to make very clear that the commit is *not* a fix for that bug and so it must *not* be closed as fixed until there is an actual fix for the ICE. So I don't think this patch is OK without having such comments in the tests to explain the issue and a carefully written commit message warning that the patch is a workaround, not a fix and the bug in question must not be closed simply because of the commit mentioning it.
Hi, On 2019/11/5 06:57, Joseph Myers wrote: > On Mon, 4 Nov 2019, luoxhu wrote: > >> -finline-functions is enabled by default for O2 since r276469, update the >> test cases with -fno-inline-functions. >> >> v2: disable inlining for the failed cases. Add two more failed cases >> not listed in BZ. Tested on P8LE, P8BE and P9LE. > > If inlining (or other interprocedural analysis) invalidates a test's > intent (e.g. all the code gets optimized away), our normal approach is to > use noinline etc. function attributes to prevent that inlining. > > If you're adding such options to work around an ICE, which certainly > appears to be the case in the architecture-independent testcases here, you > should (a) have comments in the tests saying explicitly that the options > are there temporarily to work around the ICE in a bug whose number is > given in the comment, and (b) a remark in the open regression bug for the > ICE saying that those options have been added as a temporary workaround > and that a patch fixing the ICE should remove them again. The commit > message also needs to make very clear that the commit is *not* a fix for > that bug and so it must *not* be closed as fixed until there is an actual > fix for the ICE. > > So I don't think this patch is OK without having such comments in the > tests to explain the issue and a carefully written commit message warning > that the patch is a workaround, not a fix and the bug in question must not > be closed simply because of the commit mentioning it. > Thanks. Updated the comments and message to mark the unsolved ICE issue as below, will commit it soon: -finline-functions is enabled by default for O2 since r276469, update the test cases with -fno-inline-functions. The options "-fno-inline-funtions --param max-inline-insns-single-O2=200" for c11-atomic-exec-5.c is a temporary work around to the ICE of LRA on BE systems in PR92090. This commit is NOT a fix for the ICE bug and so it must NOT be closed. v2: Disable inlining for the failed cases. Add two more failed cases not listed in BZ. Tested on P8LE, P8BE and P9LE. v3: Update commit message and comments for atomic ICE. gcc/testsuite/ChangeLog: 2019-10-30 Xiong Hu Luo <luoxhu@linux.ibm.com> PR92090 * g++.dg/tree-ssa/pr61034.C: Add -fno-inline-functions --param max-inline-insns-single-O2=200. * gcc.dg/atomic/c11-atomic-exec-5.c: Likewise. * gcc.target/powerpc/pr72804.c: Likewise. * gcc.target/powerpc/pr79439-1.c: Add -fno-inline-functions. * gcc.target/powerpc/vsx-builtin-7.c: Likewise. --- gcc/testsuite/g++.dg/tree-ssa/pr61034.C | 2 +- gcc/testsuite/gcc.dg/atomic/c11-atomic-exec-5.c | 2 +- gcc/testsuite/gcc.target/powerpc/pr72804.c | 2 +- gcc/testsuite/gcc.target/powerpc/pr79439-1.c | 2 +- gcc/testsuite/gcc.target/powerpc/vsx-builtin-7.c | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/gcc/testsuite/g++.dg/tree-ssa/pr61034.C b/gcc/testsuite/g++.dg/tree-ssa/pr61034.C index 2e3dfecacb4..338de1ebb13 100644 --- a/gcc/testsuite/g++.dg/tree-ssa/pr61034.C +++ b/gcc/testsuite/g++.dg/tree-ssa/pr61034.C @@ -1,5 +1,5 @@ // { dg-do compile } -// { dg-options "-O2 -fdump-tree-fre3 -fdump-tree-optimized -fdelete-null-pointer-checks --param early-inlining-insns-O2=14" } +// { dg-options "-O2 -fdump-tree-fre3 -fdump-tree-optimized -fdelete-null-pointer-checks --param early-inlining-insns-O2=14 -fno-inline-functions --param max-inline-insns-single-O2=200" } #define assume(x) if(!(x))__builtin_unreachable() diff --git a/gcc/testsuite/gcc.dg/atomic/c11-atomic-exec-5.c b/gcc/testsuite/gcc.dg/atomic/c11-atomic-exec-5.c index 692c64ad207..a3a5a05da30 100644 --- a/gcc/testsuite/gcc.dg/atomic/c11-atomic-exec-5.c +++ b/gcc/testsuite/gcc.dg/atomic/c11-atomic-exec-5.c @@ -3,7 +3,7 @@ iterations of the compare-and-exchange loop are needed, exceptions get properly cleared). */ /* { dg-do run } */ -/* { dg-options "-std=c11 -pedantic-errors -pthread -U_POSIX_C_SOURCE -D_POSIX_C_SOURCE=200809L" } */ +/* { dg-options "-std=c11 -pedantic-errors -pthread -U_POSIX_C_SOURCE -D_POSIX_C_SOURCE=200809L -fno-inline-functions --param max-inline-insns-single-O2=200" } */ /* Disable inline here as "-Os" will hit ICE in LRA on Power8 BE, see PR92090. */ /* { dg-add-options ieee } */ /* { dg-additional-options "-mfp-trap-mode=sui" { target alpha*-*-* } } */ /* { dg-additional-options "-D_XOPEN_SOURCE=600" { target *-*-solaris2* } } */ diff --git a/gcc/testsuite/gcc.target/powerpc/pr72804.c b/gcc/testsuite/gcc.target/powerpc/pr72804.c index b83b6350d75..0fc3df1d89b 100644 --- a/gcc/testsuite/gcc.target/powerpc/pr72804.c +++ b/gcc/testsuite/gcc.target/powerpc/pr72804.c @@ -1,6 +1,6 @@ /* { dg-do compile { target { lp64 } } } */ /* { dg-skip-if "" { powerpc*-*-darwin* } } */ -/* { dg-require-effective-target powerpc_vsx_ok } */ +/* { dg-require-effective-target powerpc_vsx_ok -fno-inline-functions --param max-inline-insns-single-O2=200 } */ /* { dg-options "-O2 -mvsx" } */ __int128_t diff --git a/gcc/testsuite/gcc.target/powerpc/pr79439-1.c b/gcc/testsuite/gcc.target/powerpc/pr79439-1.c index 5732a236c8e..539c96f571e 100644 --- a/gcc/testsuite/gcc.target/powerpc/pr79439-1.c +++ b/gcc/testsuite/gcc.target/powerpc/pr79439-1.c @@ -1,5 +1,5 @@ /* { dg-do compile { target { powerpc*-*-linux* && lp64 } } } */ -/* { dg-options "-O2 -fpic -fno-reorder-blocks" } */ +/* { dg-options "-O2 -fpic -fno-reorder-blocks -fno-inline-functions" } */ /* On the Linux 64-bit ABIs, we eliminate NOP in the 'rec' call even if -fpic is used. The recursive call should call the local alias. The diff --git a/gcc/testsuite/gcc.target/powerpc/vsx-builtin-7.c b/gcc/testsuite/gcc.target/powerpc/vsx-builtin-7.c index 5d31309f272..0780b01ffab 100644 --- a/gcc/testsuite/gcc.target/powerpc/vsx-builtin-7.c +++ b/gcc/testsuite/gcc.target/powerpc/vsx-builtin-7.c @@ -1,7 +1,7 @@ /* { dg-do compile { target { powerpc*-*-* } } } */ /* { dg-skip-if "" { powerpc*-*-darwin* } } */ /* { dg-require-effective-target powerpc_vsx_ok } */ -/* { dg-options "-O2 -mdejagnu-cpu=power7" } */ +/* { dg-options "-O2 -mdejagnu-cpu=power7 -fno-inline-functions" } */ /* Test simple extract/insert/slat operations. Make sure all types are supported with various options. */
on 2019/11/5 上午6:57, Joseph Myers wrote: > On Mon, 4 Nov 2019, luoxhu wrote: > >> -finline-functions is enabled by default for O2 since r276469, update the >> test cases with -fno-inline-functions. >> >> v2: disable inlining for the failed cases. Add two more failed cases >> not listed in BZ. Tested on P8LE, P8BE and P9LE. > > If inlining (or other interprocedural analysis) invalidates a test's > intent (e.g. all the code gets optimized away), our normal approach is to > use noinline etc. function attributes to prevent that inlining. > > If you're adding such options to work around an ICE, which certainly > appears to be the case in the architecture-independent testcases here, you > should (a) have comments in the tests saying explicitly that the options > are there temporarily to work around the ICE in a bug whose number is > given in the comment, and (b) a remark in the open regression bug for the > ICE saying that those options have been added as a temporary workaround > and that a patch fixing the ICE should remove them again. The commit > message also needs to make very clear that the commit is *not* a fix for > that bug and so it must *not* be closed as fixed until there is an actual > fix for the ICE. > Hi Joseph, Very good point! Since gcc doesn't pursue 100% testsuite pass rate, I noticed there are a few failures exposed/caused by some PRs all the time. Could we just leave the test case there without any pre workaround till the PR get fixed? It seems more like what we did often. Or the good thing with pre workaround here is to make this case sensitive again for being used for other testing? BR, Kewen > So I don't think this patch is OK without having such comments in the > tests to explain the issue and a carefully written commit message warning > that the patch is a workaround, not a fix and the bug in question must not > be closed simply because of the commit mentioning it. >
On Tue, 5 Nov 2019, Kewen.Lin wrote: > Very good point! Since gcc doesn't pursue 100% testsuite pass rate, I noticed > there are a few failures exposed/caused by some PRs all the time. Could we > just leave the test case there without any pre workaround till the PR get fixed? Yes, leaving the test as-is would be typical in GCC (though there is also an argument for XFAILing and filing bugs to try to keep the baseline testsuite state clean and instead use only Bugzilla to track known regressions).
On 2019/11/6 02:20, Joseph Myers wrote: > On Tue, 5 Nov 2019, Kewen.Lin wrote: > >> Very good point! Since gcc doesn't pursue 100% testsuite pass rate, I noticed >> there are a few failures exposed/caused by some PRs all the time. Could we >> just leave the test case there without any pre workaround till the PR get fixed? > > Yes, leaving the test as-is would be typical in GCC (though there is also > an argument for XFAILing and filing bugs to try to keep the baseline > testsuite state clean and instead use only Bugzilla to track known > regressions). > Committed to r277872 to fix part of the failures except the ICE case atomics-exec-5.c. Peter Bergner has a stand alone test to reproduce it. It may be fixed it soon. Xiong Hu Thanks
diff --git a/gcc/testsuite/g++.dg/tree-ssa/pr61034.C b/gcc/testsuite/g++.dg/tree-ssa/pr61034.C index 2e3dfecacb4..338de1ebb13 100644 --- a/gcc/testsuite/g++.dg/tree-ssa/pr61034.C +++ b/gcc/testsuite/g++.dg/tree-ssa/pr61034.C @@ -1,5 +1,5 @@ // { dg-do compile } -// { dg-options "-O2 -fdump-tree-fre3 -fdump-tree-optimized -fdelete-null-pointer-checks --param early-inlining-insns-O2=14" } +// { dg-options "-O2 -fdump-tree-fre3 -fdump-tree-optimized -fdelete-null-pointer-checks --param early-inlining-insns-O2=14 -fno-inline-functions --param max-inline-insns-single-O2=200" } #define assume(x) if(!(x))__builtin_unreachable() diff --git a/gcc/testsuite/gcc.dg/atomic/c11-atomic-exec-5.c b/gcc/testsuite/gcc.dg/atomic/c11-atomic-exec-5.c index 692c64ad207..0d70a769fdd 100644 --- a/gcc/testsuite/gcc.dg/atomic/c11-atomic-exec-5.c +++ b/gcc/testsuite/gcc.dg/atomic/c11-atomic-exec-5.c @@ -3,7 +3,7 @@ iterations of the compare-and-exchange loop are needed, exceptions get properly cleared). */ /* { dg-do run } */ -/* { dg-options "-std=c11 -pedantic-errors -pthread -U_POSIX_C_SOURCE -D_POSIX_C_SOURCE=200809L" } */ +/* { dg-options "-std=c11 -pedantic-errors -pthread -U_POSIX_C_SOURCE -D_POSIX_C_SOURCE=200809L -fno-inline-functions --param max-inline-insns-single-O2=200" } */ /* { dg-add-options ieee } */ /* { dg-additional-options "-mfp-trap-mode=sui" { target alpha*-*-* } } */ /* { dg-additional-options "-D_XOPEN_SOURCE=600" { target *-*-solaris2* } } */ diff --git a/gcc/testsuite/gcc.target/powerpc/pr72804.c b/gcc/testsuite/gcc.target/powerpc/pr72804.c index b83b6350d75..0fc3df1d89b 100644 --- a/gcc/testsuite/gcc.target/powerpc/pr72804.c +++ b/gcc/testsuite/gcc.target/powerpc/pr72804.c @@ -1,6 +1,6 @@ /* { dg-do compile { target { lp64 } } } */ /* { dg-skip-if "" { powerpc*-*-darwin* } } */ -/* { dg-require-effective-target powerpc_vsx_ok } */ +/* { dg-require-effective-target powerpc_vsx_ok -fno-inline-functions --param max-inline-insns-single-O2=200 } */ /* { dg-options "-O2 -mvsx" } */ __int128_t diff --git a/gcc/testsuite/gcc.target/powerpc/pr79439-1.c b/gcc/testsuite/gcc.target/powerpc/pr79439-1.c index 5732a236c8e..539c96f571e 100644 --- a/gcc/testsuite/gcc.target/powerpc/pr79439-1.c +++ b/gcc/testsuite/gcc.target/powerpc/pr79439-1.c @@ -1,5 +1,5 @@ /* { dg-do compile { target { powerpc*-*-linux* && lp64 } } } */ -/* { dg-options "-O2 -fpic -fno-reorder-blocks" } */ +/* { dg-options "-O2 -fpic -fno-reorder-blocks -fno-inline-functions" } */ /* On the Linux 64-bit ABIs, we eliminate NOP in the 'rec' call even if -fpic is used. The recursive call should call the local alias. The diff --git a/gcc/testsuite/gcc.target/powerpc/vsx-builtin-7.c b/gcc/testsuite/gcc.target/powerpc/vsx-builtin-7.c index 5d31309f272..0780b01ffab 100644 --- a/gcc/testsuite/gcc.target/powerpc/vsx-builtin-7.c +++ b/gcc/testsuite/gcc.target/powerpc/vsx-builtin-7.c @@ -1,7 +1,7 @@ /* { dg-do compile { target { powerpc*-*-* } } } */ /* { dg-skip-if "" { powerpc*-*-darwin* } } */ /* { dg-require-effective-target powerpc_vsx_ok } */ -/* { dg-options "-O2 -mdejagnu-cpu=power7" } */ +/* { dg-options "-O2 -mdejagnu-cpu=power7 -fno-inline-functions" } */ /* Test simple extract/insert/slat operations. Make sure all types are supported with various options. */