[v2] PR92090: Fix testcase failures by r276469
diff mbox series

Message ID ad3c665d-1850-3bbd-2a35-d04ee88dc0cc@linux.ibm.com
State New
Headers show
Series
  • [v2] PR92090: Fix testcase failures by r276469
Related show

Commit Message

Xiong Hu Luo Nov. 4, 2019, 3:29 a.m. UTC
-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.
---
 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(-)

Comments

Jeff Law Nov. 4, 2019, 4:12 p.m. UTC | #1
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
Segher Boessenkool Nov. 4, 2019, 10:39 p.m. UTC | #2
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
Joseph Myers Nov. 4, 2019, 10:57 p.m. UTC | #3
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.
Xiong Hu Luo Nov. 5, 2019, 3:43 a.m. UTC | #4
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.  */
Kewen.Lin Nov. 5, 2019, 5:54 a.m. UTC | #5
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.
>
Joseph Myers Nov. 5, 2019, 6:20 p.m. UTC | #6
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).
Xiong Hu Luo Nov. 6, 2019, 5:12 a.m. UTC | #7
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

Patch
diff mbox series

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.  */