diff mbox series

enable __ieee128 for p9vector tests

Message ID orczvczl04.fsf@lxoliva.fsfla.org
State New
Headers show
Series enable __ieee128 for p9vector tests | expand

Commit Message

Alexandre Oliva April 2, 2021, 4:52 p.m. UTC 
Several compile tests that use the __ieee128 type do not ensure it is
defined.  This patch adds -mfloat128 to their command lines, and
disregards the warning that may be issued by it.

Tested on x86_64-linux-gnu with a cross to powerpc-wrs-vxworks7r2,
configured for a CPU without altivec/vsx support.  Ok to install?


for  gcc/testsuite/ChangeLog

	* gcc_target/powerpc/bfp/scalar-extract-exp-5.c: Add
	-mfloat128, and disregard warning about it.
	* gcc_target/powerpc/bfp/scalar-extract-sig-5.c: Likewise.
	* gcc_target/powerpc/bfp/scalar-insert-exp-11.c: Likewise.
	* gcc_target/powerpc/bfp/scalar-insert-exp-8.c: Likewise.
	* gcc_target/powerpc/bfp/scalar-test-data-class-11.c: Likewise.
	* gcc_target/powerpc/bfp/scalar-test-neg-5.c: Likewise.
---
 .../gcc.target/powerpc/bfp/scalar-extract-exp-5.c  |    3 ++-
 .../gcc.target/powerpc/bfp/scalar-extract-sig-5.c  |    3 ++-
 .../gcc.target/powerpc/bfp/scalar-insert-exp-11.c  |    3 ++-
 .../gcc.target/powerpc/bfp/scalar-insert-exp-8.c   |    3 ++-
 .../powerpc/bfp/scalar-test-data-class-11.c        |    3 ++-
 .../gcc.target/powerpc/bfp/scalar-test-neg-5.c     |    3 ++-
 6 files changed, 12 insertions(+), 6 deletions(-)

Comments

Segher Boessenkool April 12, 2021, 9:04 p.m. UTC | #1 
Hi!

Sorry for the late answer.

On Fri, Apr 02, 2021 at 01:52:59PM -0300, Alexandre Oliva wrote:
> Several compile tests that use the __ieee128 type do not ensure it is
> defined.  This patch adds -mfloat128 to their command lines, and
> disregards the warning that may be issued by it.

But they do make sure it is defined, they use -mcpu=power9 (etc.).  What
is different in your setup that that does not work?


Segher
Alexandre Oliva April 17, 2021, 9:19 a.m. UTC | #2 
On Apr 12, 2021, Segher Boessenkool <segher@kernel.crashing.org> wrote:

> Hi!
> Sorry for the late answer.

> On Fri, Apr 02, 2021 at 01:52:59PM -0300, Alexandre Oliva wrote:
>> Several compile tests that use the __ieee128 type do not ensure it is
>> defined.  This patch adds -mfloat128 to their command lines, and
>> disregards the warning that may be issued by it.

> But they do make sure it is defined, they use -mcpu=power9 (etc.).  What
> is different in your setup that that does not work?

I suppose it's either -mno-altivec -mno-vsx in our self-specs, or the
very old default CPU.  I imagine it's also possible that the issue,
initially observed with GCC 10, is different or absent with the trunk.

I started trying to figure out what led __ieee128 to not be enabled
there, back then, but decided it was not so important, given that other
tests used this flag explicitly, and that it wouldn't hurt to have it
even if it wasn't always necessary.

Now, if you tell me that, even with our implicit flags and old CPU
selection, -mfloat128 should not be necessary to enable the __ieee128
type, I would be glad to dig further to try and fix the underlying bug.

Thanks,
Alexandre Oliva April 13, 2022, 11:37 p.m. UTC | #3 
On Apr 17, 2021, Alexandre Oliva <oliva@adacore.com> wrote:

> On Apr 12, 2021, Segher Boessenkool <segher@kernel.crashing.org> wrote:
>> Hi!
>> Sorry for the late answer.

Likewise :-)

>> On Fri, Apr 02, 2021 at 01:52:59PM -0300, Alexandre Oliva wrote:
>>> Several compile tests that use the __ieee128 type do not ensure it is
>>> defined.  This patch adds -mfloat128 to their command lines, and
>>> disregards the warning that may be issued by it.

>> But they do make sure it is defined, they use -mcpu=power9 (etc.).  What
>> is different in your setup that that does not work?

> I suppose it's either -mno-altivec -mno-vsx in our self-specs, or the
> very old default CPU.  I imagine it's also possible that the issue,
> initially observed with GCC 10, is different or absent with the trunk.

My supposition was wrong.  It turned out to be just because in
vxworks.h, for TARGET_VXWORKS7, there's:

#define TARGET_FLOAT128_ENABLE_TYPE 0

This disables TARGET_FLOAT128_TYPE by default, and causes the warning to
be issued when -mfloat128 is explicitly enabled.


So ping https://gcc.gnu.org/pipermail/gcc-patches/2021-April/567630.html
upthread, and expect 3 new patches related with -mfloat128 momentarily.
Segher Boessenkool April 14, 2022, 4:02 p.m. UTC | #4 
Hi!

On Sat, Apr 17, 2021 at 06:19:02AM -0300, Alexandre Oliva wrote:
> On Apr 12, 2021, Segher Boessenkool <segher@kernel.crashing.org> wrote:
> > On Fri, Apr 02, 2021 at 01:52:59PM -0300, Alexandre Oliva wrote:
> >> Several compile tests that use the __ieee128 type do not ensure it is
> >> defined.  This patch adds -mfloat128 to their command lines, and
> >> disregards the warning that may be issued by it.
> 
> > But they do make sure it is defined, they use -mcpu=power9 (etc.).  What
> > is different in your setup that that does not work?
> 
> I suppose it's either -mno-altivec -mno-vsx in our self-specs,

Yes, that is a problem.  None of our testcases are set up for compilers
with weird defaults (and this is not specific to rs6000).

I do not want to change many thousands of test cases to not use defaults
anymore, to specify everything everywhere instead :-(  This would make
things more unmaintainable than they already are.

> or the very old default CPU.

powerpc-linux uses 603, introduced at the same time as 604 (in 1994),
which is what vxworks appears to use.  It has all the same features.

> I imagine it's also possible that the issue,
> initially observed with GCC 10, is different or absent with the trunk.
> 
> I started trying to figure out what led __ieee128 to not be enabled
> there, back then, but decided it was not so important, given that other
> tests used this flag explicitly, and that it wouldn't hurt to have it
> even if it wasn't always necessary.

GCC for PowerPC does not currently support IEEE QP float on CPUs without
VSX.  Other than that, it should work (but no doubt there still are
problems).


Segher
Segher Boessenkool April 14, 2022, 4:06 p.m. UTC | #5 
On Wed, Apr 13, 2022 at 08:37:40PM -0300, Alexandre Oliva wrote:
> On Apr 17, 2021, Alexandre Oliva <oliva@adacore.com> wrote:
> > On Apr 12, 2021, Segher Boessenkool <segher@kernel.crashing.org> wrote:
> My supposition was wrong.  It turned out to be just because in
> vxworks.h, for TARGET_VXWORKS7, there's:
> 
> #define TARGET_FLOAT128_ENABLE_TYPE 0

This is the default as well, so what vsworks.h does is a no-op.

> This disables TARGET_FLOAT128_TYPE by default, and causes the warning to
> be issued when -mfloat128 is explicitly enabled.
> 
> So ping https://gcc.gnu.org/pipermail/gcc-patches/2021-April/567630.html

NAK on that patch, as explained upthread.

> upthread, and expect 3 new patches related with -mfloat128 momentarily.

Thanks,


Segher
Alexandre Oliva April 14, 2022, 4:56 p.m. UTC | #6 
On Apr 14, 2022, Segher Boessenkool <segher@kernel.crashing.org> wrote:

> Hi!
> On Sat, Apr 17, 2021 at 06:19:02AM -0300, Alexandre Oliva wrote:
>> On Apr 12, 2021, Segher Boessenkool <segher@kernel.crashing.org> wrote:
>> > On Fri, Apr 02, 2021 at 01:52:59PM -0300, Alexandre Oliva wrote:
>> >> Several compile tests that use the __ieee128 type do not ensure it is
>> >> defined.  This patch adds -mfloat128 to their command lines, and
>> >> disregards the warning that may be issued by it.
>> 
>> > But they do make sure it is defined, they use -mcpu=power9 (etc.).  What
>> > is different in your setup that that does not work?
>> 
>> I suppose it's either -mno-altivec -mno-vsx in our self-specs,

> Yes, that is a problem.

Sorry, that message from last year was an unfounded suspicion of mine
based on incorrect information.  Indeed, -mcpu=power9 combined with
-mno-vsx raise an error.

The relevant fact, described in yesterday's message, is that -mfloat128
is not enabled by default, even with -mcpu=power9, except on target
variants that define TARGET_FLOAT128_ENABLE_TYPE to nonzero.  As you
stated, its overall default is zero (though GNU/Linux overrides it to
nonzero), so the existing tests do not conform with the machine's
defaults in assuming -mfloat128 is enabled by -mcpu=power9.

Would you please reconsider your assessment, disregarding my incorrect
and irrelevant suspicions from last year, and instead taking the updated
and corrected information about float128 defaults into account?

Thanks,

>> or the very old default CPU.

> powerpc-linux uses 603, introduced at the same time as 604 (in 1994),
> which is what vxworks appears to use.  It has all the same features.

Yup, this was another incorrect suspicion of mine, based on another
piece of irrelevant information.
Segher Boessenkool April 14, 2022, 5:11 p.m. UTC | #7 
On Thu, Apr 14, 2022 at 01:56:39PM -0300, Alexandre Oliva wrote:
> On Apr 14, 2022, Segher Boessenkool <segher@kernel.crashing.org> wrote:
> > On Sat, Apr 17, 2021 at 06:19:02AM -0300, Alexandre Oliva wrote:
> >> On Apr 12, 2021, Segher Boessenkool <segher@kernel.crashing.org> wrote:
> >> > On Fri, Apr 02, 2021 at 01:52:59PM -0300, Alexandre Oliva wrote:
> >> >> Several compile tests that use the __ieee128 type do not ensure it is
> >> >> defined.  This patch adds -mfloat128 to their command lines, and
> >> >> disregards the warning that may be issued by it.
> >> 
> >> > But they do make sure it is defined, they use -mcpu=power9 (etc.).  What
> >> > is different in your setup that that does not work?
> >> 
> >> I suppose it's either -mno-altivec -mno-vsx in our self-specs,
> 
> > Yes, that is a problem.
> 
> Sorry, that message from last year was an unfounded suspicion of mine
> based on incorrect information.  Indeed, -mcpu=power9 combined with
> -mno-vsx raise an error.

Lol, the dates line up very well, I didn't realise it was from 2021 :-)

> The relevant fact, described in yesterday's message, is that -mfloat128
> is not enabled by default, even with -mcpu=power9, except on target
> variants that define TARGET_FLOAT128_ENABLE_TYPE to nonzero.  As you
> stated, its overall default is zero (though GNU/Linux overrides it to
> nonzero), so the existing tests do not conform with the machine's
> defaults in assuming -mfloat128 is enabled by -mcpu=power9.

First off, vxworks.h should not disable it again.

Then, this needs to be fixed, indeed.  But that would be a code fix, not
a testsuite workaround.  If you use -mcpu=power9 it should support QP
float.


Segher
Alexandre Oliva April 14, 2022, 6:42 p.m. UTC | #8 
On Apr 14, 2022, Segher Boessenkool <segher@kernel.crashing.org> wrote:

> Lol, the dates line up very well, I didn't realise it was from 2021 :-)

Heh, indeed.  Same testsuite results cleanup season, too ;-)

>> The relevant fact, described in yesterday's message, is that -mfloat128
>> is not enabled by default, even with -mcpu=power9, except on target
>> variants that define TARGET_FLOAT128_ENABLE_TYPE to nonzero.  As you
>> stated, its overall default is zero (though GNU/Linux overrides it to
>> nonzero), so the existing tests do not conform with the machine's
>> defaults in assuming -mfloat128 is enabled by -mcpu=power9.

> First off, vxworks.h should not disable it again.

Erhm...  I'm not sure what the 'it' is.

For abundance of clarity, we do *not* disable vsx when -mcpu=power9 is
given.  vsx is enabled for these tests.  But neither -mcpu=power9 nor
having vsx enabled are enough for the _Float128/_ieee128 type to be
defined.

The target-specific option that controls whether _Float128/_ieee128 is
defined when VSX is enabled is TARGET_FLOAT128_ENABLE_TYPE.  The only
file that defines it as nonzero is rs6000/linux64.h, which backs up the
comment in rs6000.cc before the statement that carries out this choice:

  /* Enable the default support for IEEE 128-bit floating point on
     [GNU/]Linux VSX sytems.  [...]  */
  TARGET_FLOAT128_TYPE = TARGET_FLOAT128_ENABLE_TYPE && TARGET_VSX;


So, if the 'it' refers to VSX, I reaffirm it's enabled as it should.
But if 'it' refers to TARGET_FLOAT128_ENABLE_TYPE, then it would seem
that you're saying that this is no longer a choice available to targets,
and that _Float128/_ieee128 are now mandatory when VSX is available.
That would be quite a departure from the current state.

Now, we are looking into the possibility of enabling _Float128/_ieee128
on ppc64-vx7r2, but keep in mind it's a nonfree system, so if system
libraries (or kernel) aren't up to it, that would be a blocker.  So I'd
prefer if both choices for TARGET_FLOAT128_ENABLE_TYPE remained
available.


> Then, this needs to be fixed, indeed.  But that would be a code fix, not
> a testsuite workaround.  If you use -mcpu=power9 it should support QP
> float.

I guess there's room for improvement indeed, especially in light of the
second patch for pr79004.c sent out ealier today, but I don't think I'd
risk such changes at this stage of development of gcc-12, let alone when
maintainer and implementation seem to me to disagree as to what the
expected behavior is :-(
diff mbox series

Patch

diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-5.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-5.c
index 34184812dc5cf..f57a388d8628f 100644
--- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-5.c
+++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-5.c
@@ -1,7 +1,8 @@ 
 /* { dg-do compile { target { powerpc*-*-* } } } */
 /* { dg-require-effective-target ilp32 } */
 /* { dg-require-effective-target powerpc_p9vector_ok } */
-/* { dg-options "-mdejagnu-cpu=power9" } */
+/* { dg-options "-mdejagnu-cpu=power9 -mfloat128" } */
+/* { dg-prune-output ".-mfloat128. option may not be fully supported" } */
 
 /* This test only runs on 32-bit configurations, where a compiler error
    should be issued because this builtin is not available on
diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-5.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-5.c
index 13c64fc3acfef..786740b2b8404 100644
--- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-5.c
+++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-5.c
@@ -1,7 +1,8 @@ 
 /* { dg-do compile { target { powerpc*-*-* } } } */
 /* { dg-require-effective-target ilp32 } */
 /* { dg-require-effective-target powerpc_p9vector_ok } */
-/* { dg-options "-mdejagnu-cpu=power9" } */
+/* { dg-options "-mdejagnu-cpu=power9 -mfloat128" } */
+/* { dg-prune-output ".-mfloat128. option may not be fully supported" } */
 
 /* This test only runs on 32-bit configurations, producing a compiler
    error because the builtin requires 64 bits.  */
diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-11.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-11.c
index a5dd852e60f0a..fd055c8a1fc31 100644
--- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-11.c
+++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-11.c
@@ -1,7 +1,8 @@ 
 /* { dg-do compile { target { powerpc*-*-* } } } */
 /* { dg-require-effective-target ilp32 } */
 /* { dg-require-effective-target powerpc_p9vector_ok } */
-/* { dg-options "-mdejagnu-cpu=power9" } */
+/* { dg-options "-mdejagnu-cpu=power9 -mfloat128" } */
+/* { dg-prune-output ".-mfloat128. option may not be fully supported" } */
 
 /* This test only runs on 32-bit configurations, where a compiler error
    should be issued because this builtin is not available on
diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-8.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-8.c
index bd68f77098568..795106b936c88 100644
--- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-8.c
+++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-8.c
@@ -1,7 +1,8 @@ 
 /* { dg-do compile { target { powerpc*-*-* } } } */
 /* { dg-require-effective-target ilp32 } */
 /* { dg-require-effective-target powerpc_p9vector_ok } */
-/* { dg-options "-mdejagnu-cpu=power9" } */
+/* { dg-options "-mdejagnu-cpu=power9 -mfloat128" } */
+/* { dg-prune-output ".-mfloat128. option may not be fully supported" } */
 
 /* This test only runs on 32-bit configurations, where a compiler error
    should be issued because this builtin is not available on
diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-data-class-11.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-data-class-11.c
index 7c6fca2b7292b..945257762c1dd 100644
--- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-data-class-11.c
+++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-data-class-11.c
@@ -1,6 +1,7 @@ 
 /* { dg-do compile { target { powerpc*-*-* } } } */
 /* { dg-require-effective-target powerpc_p9vector_ok } */
-/* { dg-options "-mdejagnu-cpu=power8" } */
+/* { dg-options "-mdejagnu-cpu=power8 -mfloat128" } */
+/* { dg-prune-output ".-mfloat128. option may not be fully supported" } */
 
 #include <altivec.h>
 #include <stdbool.h>
diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-5.c b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-5.c
index bab86040a7bf4..74b82aee40877 100644
--- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-5.c
+++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-5.c
@@ -1,6 +1,7 @@ 
 /* { dg-do compile { target { powerpc*-*-* } } } */
 /* { dg-require-effective-target powerpc_p9vector_ok } */
-/* { dg-options "-mdejagnu-cpu=power8" } */
+/* { dg-options "-mdejagnu-cpu=power8 -mfloat128" } */
+/* { dg-prune-output ".-mfloat128. option may not be fully supported" } */
 
 #include <altivec.h>
 #include <stdbool.h>