| Message ID | 20201028104835.GY15956@bubble.grove.modra.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RS6000] float128-type-2.c unsupported | expand |
On Wed, Oct 28, 2020 at 6:48 AM Alan Modra <amodra@gmail.com> wrote: > > From e7ce33cef478a826a2fe4e110b43b49586ef2438 Mon Sep 17 00:00:00 2001 > From: Alan Modra <amodra@gmail.com> > Date: Wed, 28 Oct 2020 15:57:57 +1030 > Subject: > > I noticed this test is unsupported on power10 when looking through > test logs. There seems no reason why that should be the case, ie. > likely the target test was meant to be powerpc64*-*-linux*. And that > simplifies down further. > > Regression tested powerpc64le-linux. OK? > > * gcc.target/powerpc/float128-type-1.c: Simplify target test. > * gcc.target/powerpc/float128-type-2.c: Likewise. Unfortunately, no. The GCC testsuite has probes for float128, ppc_float128, ppc_ieee128. The testcases should test for the appropriate feature, not for Linux nor for LP64. Thanks, David
On Wed, Oct 28, 2020 at 09:18:35PM +1030, Alan Modra wrote: > >From e7ce33cef478a826a2fe4e110b43b49586ef2438 Mon Sep 17 00:00:00 2001 > From: Alan Modra <amodra@gmail.com> > Date: Wed, 28 Oct 2020 15:57:57 +1030 > Subject: > > I noticed this test is unsupported on power10 when looking through > test logs. There seems no reason why that should be the case, ie. > likely the target test was meant to be powerpc64*-*-linux*. And that > simplifies down further. The target name does not tell you if you are doing a -m32 or a -m64 build; both powerpc-linux and powerpc64-linux can build both 32-bit and 64-bit just fine (and hopefully identically). Having target powerpc64* is basically always wrong. > diff --git a/gcc/testsuite/gcc.target/powerpc/float128-type-1.c b/gcc/testsuite/gcc.target/powerpc/float128-type-1.c > index 13152ac7c26..53f9e357535 100644 > --- a/gcc/testsuite/gcc.target/powerpc/float128-type-1.c > +++ b/gcc/testsuite/gcc.target/powerpc/float128-type-1.c > @@ -1,4 +1,4 @@ > -/* { dg-do compile { target { powerpc64*-*-linux* && lp64 } } } */ > +/* { dg-do compile { target { *-*-linux* && lp64 } } } */ > /* { dg-require-effective-target powerpc_p8vector_ok } */ > /* { dg-options "-mdejagnu-cpu=power8 -O2 -mno-float128" } */ > > diff --git a/gcc/testsuite/gcc.target/powerpc/float128-type-2.c b/gcc/testsuite/gcc.target/powerpc/float128-type-2.c > index 5644281c3d4..02dbad1fa4f 100644 > --- a/gcc/testsuite/gcc.target/powerpc/float128-type-2.c > +++ b/gcc/testsuite/gcc.target/powerpc/float128-type-2.c > @@ -1,4 +1,4 @@ > -/* { dg-do compile { target { powerpc64-*-linux* && lp64 } } } */ > +/* { dg-do compile { target { *-*-linux* && lp64 } } } */ > /* { dg-require-effective-target powerpc_p9vector_ok } */ > /* { dg-options "-mdejagnu-cpu=power9 -O2 -mno-float128" } */ Your patch is fine though, modulo what David said. If there is some selector you can use (or you can make one) that is much preferred. But since this patch is strictly an improvement already, it is okay for trunk (if the 2nd works on powerpc64le-linux of course ;-) ) Thanks! (Improving it to test on exactly the right targets would be nice :-) ) Segher
On Wed, Oct 28, 2020 at 01:44:54PM -0500, Segher Boessenkool wrote: > On Wed, Oct 28, 2020 at 09:18:35PM +1030, Alan Modra wrote: > > >From e7ce33cef478a826a2fe4e110b43b49586ef2438 Mon Sep 17 00:00:00 2001 > > From: Alan Modra <amodra@gmail.com> > > Date: Wed, 28 Oct 2020 15:57:57 +1030 > > Subject: > > > > I noticed this test is unsupported on power10 when looking through > > test logs. There seems no reason why that should be the case, ie. > > likely the target test was meant to be powerpc64*-*-linux*. And that > > simplifies down further. > > The target name does not tell you if you are doing a -m32 or a -m64 > build; both powerpc-linux and powerpc64-linux can build both 32-bit and > 64-bit just fine (and hopefully identically). Having target powerpc64* > is basically always wrong. Yes. Even le/be selection should really be done with { target le } for example rather than { target powerpc*le-*-* }. One day we might want to test compilers with multi-endian support. > Your patch is fine though, modulo what David said. If there is some > selector you can use (or you can make one) that is much preferred. But > since this patch is strictly an improvement already, it is okay for > trunk (if the 2nd works on powerpc64le-linux of course ;-) ) Thanks! > > (Improving it to test on exactly the right targets would be nice :-) ) Thank you, I committed it "as is". An incremental improvement is better than no improvement.
On Wed, Oct 28, 2020 at 7:28 PM Alan Modra <amodra@gmail.com> wrote: > > On Wed, Oct 28, 2020 at 01:44:54PM -0500, Segher Boessenkool wrote: > > On Wed, Oct 28, 2020 at 09:18:35PM +1030, Alan Modra wrote: > > > >From e7ce33cef478a826a2fe4e110b43b49586ef2438 Mon Sep 17 00:00:00 2001 > > > From: Alan Modra <amodra@gmail.com> > > > Date: Wed, 28 Oct 2020 15:57:57 +1030 > > > Subject: > > > > > > I noticed this test is unsupported on power10 when looking through > > > test logs. There seems no reason why that should be the case, ie. > > > likely the target test was meant to be powerpc64*-*-linux*. And that > > > simplifies down further. > > > > The target name does not tell you if you are doing a -m32 or a -m64 > > build; both powerpc-linux and powerpc64-linux can build both 32-bit and > > 64-bit just fine (and hopefully identically). Having target powerpc64* > > is basically always wrong. > > Yes. Even le/be selection should really be done with { target le } > for example rather than { target powerpc*le-*-* }. One day we might > want to test compilers with multi-endian support. > > > Your patch is fine though, modulo what David said. If there is some > > selector you can use (or you can make one) that is much preferred. But > > since this patch is strictly an improvement already, it is okay for > > trunk (if the 2nd works on powerpc64le-linux of course ;-) ) Thanks! > > > > (Improving it to test on exactly the right targets would be nice :-) ) > > Thank you, I committed it "as is". An incremental improvement is > better than no improvement. Alan, It is disrespectful for you to ignore the review of a maintainer and your colleague. You may not pick and choose amongst maintainers. And Segher should not be so disrespectful as to contradict his colleague and co-maintainer. I replied no to your patch and requested a different solution -- one that does not require significant effort. Please fix this testcase the way that I requested. Thank You, David
On Wed, Oct 28, 2020 at 11:35:07PM -0400, David Edelsohn wrote: > Alan, > > It is disrespectful for you to ignore the review of a maintainer and > your colleague. You may not pick and choose amongst maintainers. And > Segher should not be so disrespectful as to contradict his colleague > and co-maintainer. I'm sorry you see this as a matter of respect. I didn't see it that way at all. Segher disagreed with your review, and gave sufficient technical reason for me to commit the patch. > I replied no to your patch and requested a different solution -- one > that does not require significant effort. Please fix this testcase > the way that I requested. I am not going to do as you demand. Those tests are clearly commented as "VSX Linux 64-bit" tests. Ignoring that comment and changing them to make them run for AIX and Darwin plainly requires that I test those changes, which often results in significant effort building cross-compilers. If you wish them to run for AIX targets then that is your job as an AIX maintainer. Don't expect me to do the work. Incidentally, this is the result of your recent commit 122f0db2793 on powerpc64-linux biarch. +FAIL: gcc.target/powerpc/signbit-1.c scan-assembler-not stxvd2x +FAIL: gcc.target/powerpc/signbit-1.c scan-assembler-times mfvsrd 3 +FAIL: gcc.target/powerpc/signbit-1.c scan-assembler-times srdi 3 +FAIL: gcc.target/powerpc/signbit-2.c scan-assembler-times ld 1 +FAIL: gcc.target/powerpc/signbit-2.c scan-assembler-times srdi 1 I'm not complaining about that, or demanding that you fix those fails; In fact I'm in the middle of fixing them. But they do quite perfectly illustrate that tweaking the testsuite is never simple.
Fixes
FAIL: gcc.target/powerpc/signbit-1.c scan-assembler-not stxvd2x
FAIL: gcc.target/powerpc/signbit-1.c scan-assembler-times mfvsrd 3
FAIL: gcc.target/powerpc/signbit-1.c scan-assembler-times srdi 3
FAIL: gcc.target/powerpc/signbit-2.c scan-assembler-times ld 1
FAIL: gcc.target/powerpc/signbit-2.c scan-assembler-times srdi 1
on powerpc-linux (or powerpc64-linux biarch -m32).
signbit-1.c is quite obviously a 64-bit only testcase given the
scan-assembler directives, and the purpose of the testcase to verify
the 64-bit only UNSPEC_SIGNBIT patterns. It could be made to pass for
-m32 by adding -mpowerpc64, but that option that isn't very effective
when bi-arch testing and results in errors on rs6000-aix. And it is
pointless to match -m32 stores to the stack followed by loads, which
is what we do at the moment.
signbit-2.c on the other hand has more reasonable 32-bit output.
Regression tested powerpc64-linux biarch.
* gcc.target/powerpc/signbit-1.c: Reinstate lp64 condition.
* gcc.target/powerpc/signbit-2.c: Match 32-bit output too.
diff --git a/gcc/testsuite/gcc.target/powerpc/signbit-1.c b/gcc/testsuite/gcc.target/powerpc/signbit-1.c
index eb4f53e397d..1642bf46d7a 100644
--- a/gcc/testsuite/gcc.target/powerpc/signbit-1.c
+++ b/gcc/testsuite/gcc.target/powerpc/signbit-1.c
@@ -1,4 +1,5 @@
/* { dg-do compile } */
+/* { dg-require-effective-target lp64 } */
/* { dg-require-effective-target ppc_float128_sw } */
/* { dg-require-effective-target powerpc_p8vector_ok } */
/* { dg-options "-mdejagnu-cpu=power8 -O2 -mfloat128" } */
diff --git a/gcc/testsuite/gcc.target/powerpc/signbit-2.c b/gcc/testsuite/gcc.target/powerpc/signbit-2.c
index ff6af963dda..1b792916eba 100644
--- a/gcc/testsuite/gcc.target/powerpc/signbit-2.c
+++ b/gcc/testsuite/gcc.target/powerpc/signbit-2.c
@@ -13,5 +13,7 @@ int do_signbit_kf (__float128 *a) { return __builtin_signbit (*a); }
/* { dg-final { scan-assembler-not "lxvw4x" } } */
/* { dg-final { scan-assembler-not "lxsd" } } */
/* { dg-final { scan-assembler-not "lxsdx" } } */
-/* { dg-final { scan-assembler-times "ld" 1 } } */
-/* { dg-final { scan-assembler-times "srdi" 1 } } */
+/* { dg-final { scan-assembler-times "ld" 1 { target lp64 } } } */
+/* { dg-final { scan-assembler-times "srdi" 1 { target lp64 } } } */
+/* { dg-final { scan-assembler-times "lwz" 1 { target ilp32 } } } */
+/* { dg-final { scan-assembler-times "rlwinm" 1 { target ilp32 } } } */
On Thu, Oct 29, 2020 at 12:53 AM Alan Modra <amodra@gmail.com> wrote: > > On Wed, Oct 28, 2020 at 11:35:07PM -0400, David Edelsohn wrote: > > Alan, > > > > It is disrespectful for you to ignore the review of a maintainer and > > your colleague. You may not pick and choose amongst maintainers. And > > Segher should not be so disrespectful as to contradict his colleague > > and co-maintainer. > > I'm sorry you see this as a matter of respect. I didn't see it that > way at all. Segher disagreed with your review, and gave sufficient > technical reason for me to commit the patch. Alan, This isn't how patch review works, and you know that. Segher also knows that. Ignoring my decision while accepting Segher's clearly is a measure of respect. You don't get to decide whose justification or reasons are appropriate -- that is a flimsy rationalization for your unprofessional behavior. If you or Segher disagree with my review, we can discuss it like mature adults and reach a mutually-acceptable outcome. I'm a reasonable person. Collaboratively developing software is not a game of playing maintainers off one another like children gaming parents until they receive the answer that they want. Both you and Segher are adults and have leadership roles in the GNU Toolchain. Everyone is under additional stress because of the global health and economic situations, as well as technical projects, but that is not an excuse. Both of you are capable of better behavior and I wish to see it demonstrated. Thank You, David
Hi David, all, On Thu, Oct 29, 2020 at 10:12:31AM -0400, David Edelsohn wrote: > > I'm sorry you see this as a matter of respect. I didn't see it that > > way at all. Segher disagreed with your review, and gave sufficient > > technical reason for me to commit the patch. > If you or Segher disagree with my review, we can discuss it like > mature adults and reach a mutually-acceptable outcome. I'm a > reasonable person. Like I said, I do *not* disagree with what you said. I just thought you missed the point of the patch, which was to fix another (much more trivial) problem, not the bigger problem that you want to see fixed (and I agree with that, as I said). But we cannot ask contributors to solve unrelated problems. We can try to interest them in solving things, but not much more. I thought it would be obvious to you as well that Alan's patch just was for the one simple problem (like some related patches I acked at about the same time), which is why I didn't talk to you first. I wasn't overriding your review, your points are well taken. Take care, Segher
On Thu, Oct 29, 2020 at 10:10:58PM +1030, Alan Modra wrote: > Fixes > FAIL: gcc.target/powerpc/signbit-1.c scan-assembler-not stxvd2x > FAIL: gcc.target/powerpc/signbit-1.c scan-assembler-times mfvsrd 3 > FAIL: gcc.target/powerpc/signbit-1.c scan-assembler-times srdi 3 > FAIL: gcc.target/powerpc/signbit-2.c scan-assembler-times ld 1 > FAIL: gcc.target/powerpc/signbit-2.c scan-assembler-times srdi 1 > on powerpc-linux (or powerpc64-linux biarch -m32). David, This patch is fixing a regression caused by one of your testsuite patches. Please review. https://gcc.gnu.org/pipermail/gcc-patches/2020-October/557443.html > signbit-1.c is quite obviously a 64-bit only testcase given the > scan-assembler directives, and the purpose of the testcase to verify > the 64-bit only UNSPEC_SIGNBIT patterns. It could be made to pass for > -m32 by adding -mpowerpc64, but that option that isn't very effective > when bi-arch testing and results in errors on rs6000-aix. And it is > pointless to match -m32 stores to the stack followed by loads, which > is what we do at the moment. > > signbit-2.c on the other hand has more reasonable 32-bit output. > > Regression tested powerpc64-linux biarch. > > * gcc.target/powerpc/signbit-1.c: Reinstate lp64 condition. > * gcc.target/powerpc/signbit-2.c: Match 32-bit output too. > > diff --git a/gcc/testsuite/gcc.target/powerpc/signbit-1.c b/gcc/testsuite/gcc.target/powerpc/signbit-1.c > index eb4f53e397d..1642bf46d7a 100644 > --- a/gcc/testsuite/gcc.target/powerpc/signbit-1.c > +++ b/gcc/testsuite/gcc.target/powerpc/signbit-1.c > @@ -1,4 +1,5 @@ > /* { dg-do compile } */ > +/* { dg-require-effective-target lp64 } */ > /* { dg-require-effective-target ppc_float128_sw } */ > /* { dg-require-effective-target powerpc_p8vector_ok } */ > /* { dg-options "-mdejagnu-cpu=power8 -O2 -mfloat128" } */ > diff --git a/gcc/testsuite/gcc.target/powerpc/signbit-2.c b/gcc/testsuite/gcc.target/powerpc/signbit-2.c > index ff6af963dda..1b792916eba 100644 > --- a/gcc/testsuite/gcc.target/powerpc/signbit-2.c > +++ b/gcc/testsuite/gcc.target/powerpc/signbit-2.c > @@ -13,5 +13,7 @@ int do_signbit_kf (__float128 *a) { return __builtin_signbit (*a); } > /* { dg-final { scan-assembler-not "lxvw4x" } } */ > /* { dg-final { scan-assembler-not "lxsd" } } */ > /* { dg-final { scan-assembler-not "lxsdx" } } */ > -/* { dg-final { scan-assembler-times "ld" 1 } } */ > -/* { dg-final { scan-assembler-times "srdi" 1 } } */ > +/* { dg-final { scan-assembler-times "ld" 1 { target lp64 } } } */ > +/* { dg-final { scan-assembler-times "srdi" 1 { target lp64 } } } */ > +/* { dg-final { scan-assembler-times "lwz" 1 { target ilp32 } } } */ > +/* { dg-final { scan-assembler-times "rlwinm" 1 { target ilp32 } } } */
On Sat, Dec 5, 2020 at 6:12 AM Alan Modra <amodra@gmail.com> wrote: > > On Thu, Oct 29, 2020 at 10:10:58PM +1030, Alan Modra wrote: > > Fixes > > FAIL: gcc.target/powerpc/signbit-1.c scan-assembler-not stxvd2x > > FAIL: gcc.target/powerpc/signbit-1.c scan-assembler-times mfvsrd 3 > > FAIL: gcc.target/powerpc/signbit-1.c scan-assembler-times srdi 3 > > FAIL: gcc.target/powerpc/signbit-2.c scan-assembler-times ld 1 > > FAIL: gcc.target/powerpc/signbit-2.c scan-assembler-times srdi 1 > > on powerpc-linux (or powerpc64-linux biarch -m32). > > David, > This patch is fixing a regression caused by one of your testsuite > patches. Please review. > > https://gcc.gnu.org/pipermail/gcc-patches/2020-October/557443.html > > > signbit-1.c is quite obviously a 64-bit only testcase given the > > scan-assembler directives, and the purpose of the testcase to verify > > the 64-bit only UNSPEC_SIGNBIT patterns. It could be made to pass for > > -m32 by adding -mpowerpc64, but that option that isn't very effective > > when bi-arch testing and results in errors on rs6000-aix. And it is > > pointless to match -m32 stores to the stack followed by loads, which > > is what we do at the moment. > > > > signbit-2.c on the other hand has more reasonable 32-bit output. > > > > Regression tested powerpc64-linux biarch. > > > > * gcc.target/powerpc/signbit-1.c: Reinstate lp64 condition. > > * gcc.target/powerpc/signbit-2.c: Match 32-bit output too. Alan, I agree that signbit-1.c clearly checks for 64-bit only instructions, and signbit-2.c was checking for 64-bit only prior to this patch. The PPC port has an explosion of 128 bit options (float128, ieee128, long double, ieee128 in hardware, float128 in vsx, float128 in quad float library), and less than ideal clarity about the differences. As much as possible, I would like the testcase requirements set correctly, once, and then not repeatedly adjust the testcases manually for targets that we later discover should or should not succeed. We have too many ISA/ABI/OS variations and a lot of testcases. We don't have the resources to continually tweak them. I would like to differentiate among "this works on PPC64LE Linux" versus "this work on Power8" or "Power9" or "Power10" or "MMA" versus "IEEE128" versus "!aix && !darwin". Not substituting PPC64 Linux or Power9 to mean IEEE128. Where possible we should check for the requirements that we are testing, not convenient stand-ins. That being said, it's now clear that there is no target test in the testsuite that captures float128 VSX functionality. So testing float128_sw and lp64 is the best alternative. Or maybe the testcase should test for the VSX level where those instructions were introduced? Thanks for investigating this and creating a patch. The patch is okay. Thanks, David
diff --git a/gcc/testsuite/gcc.target/powerpc/float128-type-1.c b/gcc/testsuite/gcc.target/powerpc/float128-type-1.c index 13152ac7c26..53f9e357535 100644 --- a/gcc/testsuite/gcc.target/powerpc/float128-type-1.c +++ b/gcc/testsuite/gcc.target/powerpc/float128-type-1.c @@ -1,4 +1,4 @@ -/* { dg-do compile { target { powerpc64*-*-linux* && lp64 } } } */ +/* { dg-do compile { target { *-*-linux* && lp64 } } } */ /* { dg-require-effective-target powerpc_p8vector_ok } */ /* { dg-options "-mdejagnu-cpu=power8 -O2 -mno-float128" } */ diff --git a/gcc/testsuite/gcc.target/powerpc/float128-type-2.c b/gcc/testsuite/gcc.target/powerpc/float128-type-2.c index 5644281c3d4..02dbad1fa4f 100644 --- a/gcc/testsuite/gcc.target/powerpc/float128-type-2.c +++ b/gcc/testsuite/gcc.target/powerpc/float128-type-2.c @@ -1,4 +1,4 @@ -/* { dg-do compile { target { powerpc64-*-linux* && lp64 } } } */ +/* { dg-do compile { target { *-*-linux* && lp64 } } } */ /* { dg-require-effective-target powerpc_p9vector_ok } */ /* { dg-options "-mdejagnu-cpu=power9 -O2 -mno-float128" } */
From e7ce33cef478a826a2fe4e110b43b49586ef2438 Mon Sep 17 00:00:00 2001 From: Alan Modra <amodra@gmail.com> Date: Wed, 28 Oct 2020 15:57:57 +1030 Subject: I noticed this test is unsupported on power10 when looking through test logs. There seems no reason why that should be the case, ie. likely the target test was meant to be powerpc64*-*-linux*. And that simplifies down further. Regression tested powerpc64le-linux. OK? * gcc.target/powerpc/float128-type-1.c: Simplify target test. * gcc.target/powerpc/float128-type-2.c: Likewise.