ppc: testsuite: pr79004 needs -mlong-double-128 (was: Re: ppc: testsuite: prune float128 partial support warnings)

On Apr 13, 2022, Alexandre Oliva <oliva@adacore.com> wrote:

> 	* gcc.target/powerpc/pr79004.c: Prune the -mfloat128 warning.

I failed to mention that this fixed a problem in the test, but that was
not enough for this test to pass; here's an incremental patch that is.


Some of the asm opcodes expected by pr79004 depend on
-mlong-double-128 to be output.  E.g., without this flag, the
conditions of patterns @extenddf<mode>2 and extendsf<mode>2 do not
hold, and so GCC resorts to libcalls instead of even trying
rs6000_expand_float128_convert.

Perhaps the conditions are too strict, and they could enable the use
of conversion insns involving __ieee128/_Float128 even with 64-bit
long doubles.  Alas, for now, we need this flag for the test to pass
on target variants that use 64-bit long doubles.

Tested on x86_64-linux-gnu x ppc64-vx7r2 with gcc-11.  Ok to install?


for  gcc/testsuite/ChangeLog

	* gcc.target/powerpr/pr79004.c: Add -mlong-double-128.
---
 gcc/testsuite/gcc.target/powerpc/pr79004.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Apr 23, 2024, "Kewen.Lin" <linkw@linux.ibm.com> wrote:

> This patch seemed to miss to CC gcc-patches list. :)

Oops, sorry, thanks for catching that.

Here it is.  FTR, you've already responded suggesting an apparent
preference for addressing PR105359, but since I meant to contribute it,
I'm reposting is to gcc-patches, now with a reference to the PR.


ppc: testsuite: pr79004 needs -mlong-double-128

Some of the asm opcodes expected by pr79004 depend on
-mlong-double-128 to be output.  E.g., without this flag, the
conditions of patterns @extenddf<mode>2 and extendsf<mode>2 do not
hold, and so GCC resorts to libcalls instead of even trying
rs6000_expand_float128_convert.

Perhaps the conditions are too strict, and they could enable the use
of conversion insns involving __ieee128/_Float128 even with 64-bit
long doubles.  Alas, for now, we need this flag for the test to pass
on target variants that use 64-bit long doubles.


for  gcc/testsuite/ChangeLog

	* gcc.target/powerpr/pr79004.c: Add -mlong-double-128.
---
 gcc/testsuite/gcc.target/powerpc/pr79004.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/testsuite/gcc.target/powerpc/pr79004.c b/gcc/testsuite/gcc.target/powerpc/pr79004.c
index e411702dc98a9..061a0e83fe2ad 100644
--- a/gcc/testsuite/gcc.target/powerpc/pr79004.c
+++ b/gcc/testsuite/gcc.target/powerpc/pr79004.c
@@ -1,6 +1,6 @@
 /* { dg-do compile { target { powerpc*-*-* && lp64 } } } */
 /* { dg-require-effective-target powerpc_p9vector_ok } */
-/* { dg-options "-mdejagnu-cpu=power9 -O2 -mfloat128" } */
+/* { dg-options "-mdejagnu-cpu=power9 -O2 -mfloat128 -mlong-double-128" } */
 /* { dg-prune-output ".-mfloat128. option may not be fully supported" } */
 
 #include <math.h>

Hi,

on 2024/4/28 16:20, Alexandre Oliva wrote:
> On Apr 23, 2024, "Kewen.Lin" <linkw@linux.ibm.com> wrote:
> 
>> This patch seemed to miss to CC gcc-patches list. :)
> 
> Oops, sorry, thanks for catching that.
> 
> Here it is.  FTR, you've already responded suggesting an apparent
> preference for addressing PR105359, but since I meant to contribute it,
> I'm reposting is to gcc-patches, now with a reference to the PR.

OK, from this perspective IMHO it seems more clear to adopt xfail
with effective target long_double_64bit?

BR,
Kewen

> 
> 
> ppc: testsuite: pr79004 needs -mlong-double-128
> 
> Some of the asm opcodes expected by pr79004 depend on
> -mlong-double-128 to be output.  E.g., without this flag, the
> conditions of patterns @extenddf<mode>2 and extendsf<mode>2 do not
> hold, and so GCC resorts to libcalls instead of even trying
> rs6000_expand_float128_convert.
> 
> Perhaps the conditions are too strict, and they could enable the use
> of conversion insns involving __ieee128/_Float128 even with 64-bit
> long doubles.  Alas, for now, we need this flag for the test to pass
> on target variants that use 64-bit long doubles.
> 
> 
> for  gcc/testsuite/ChangeLog
> 
> 	* gcc.target/powerpr/pr79004.c: Add -mlong-double-128.
> ---
>  gcc/testsuite/gcc.target/powerpc/pr79004.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr79004.c b/gcc/testsuite/gcc.target/powerpc/pr79004.c
> index e411702dc98a9..061a0e83fe2ad 100644
> --- a/gcc/testsuite/gcc.target/powerpc/pr79004.c
> +++ b/gcc/testsuite/gcc.target/powerpc/pr79004.c
> @@ -1,6 +1,6 @@
>  /* { dg-do compile { target { powerpc*-*-* && lp64 } } } */
>  /* { dg-require-effective-target powerpc_p9vector_ok } */
> -/* { dg-options "-mdejagnu-cpu=power9 -O2 -mfloat128" } */
> +/* { dg-options "-mdejagnu-cpu=power9 -O2 -mfloat128 -mlong-double-128" } */
>  /* { dg-prune-output ".-mfloat128. option may not be fully supported" } */
>  
>  #include <math.h>
> 
>

On Apr 28, 2024, "Kewen.Lin" <linkw@linux.ibm.com> wrote:

> OK, from this perspective IMHO it seems more clear to adopt xfail
> with effective target long_double_64bit?

*nod*, yeah, that makes sense.

I'm going to travel this week, to speak at FSF's LibrePlanet conference,
so I'll look into massaging the patch into that when I get back, if you
haven't rendered it obsolete by then ;-)

Thanks,

On Apr 28, 2024, "Kewen.Lin" <linkw@linux.ibm.com> wrote:

> OK, from this perspective IMHO it seems more clear to adopt xfail
> with effective target long_double_64bit?

That's effective target is quite broken, alas.  I doubt it's used
anywhere: it calls an undefined proc, and its memcmp call seems to have
the size cut&pasto-ed from the 128-bit functions.  (a patchlet that
fixes these most glaring issues is below)

Furthermore, it doesn't really work.  Since it adds -mlong-double-64 for
the effective target test, it overrides the default, so it sort of
always passes, even on a 128-bit long double target.  But since the test
itself doesn't add that option, any xfails on long_double_64bit would be
flagged as XPASS.

There's no effective target test for 64-bit long double that doesn't
override the default, so we'd have to add one.  Alas, the natural name
for it is the one that's taken with overriding behavior, and the current
option-overriding tests, that need to be used along with the
corresponding add-options in testcases, might benefit from a renaming to
make them fit the already-established (?) naming standards.  Yuck.


diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index 182d80129de9b..603da25c97d67 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -2961,12 +2961,12 @@ proc check_effective_target_long_double_64bit { } {
 	  /* eliminate removing volatile cast warning.  */
 	  a2 = a;
 	  b2 = b;
-	  if (memcmp (&a2, &b2, 16) != 0)
+	  if (memcmp (&a2, &b2, 8) != 0)
 	    return 1;
 	  sprintf (buffer, "%lg", b);
 	  return strcmp (buffer, "3") != 0;
 	}
-    }  [add_options_for_ppc_long_double_override_64bit ""]]
+    }  [add_options_for_long_double_64bit ""]]
 }
 
 # Return the appropriate options to specify that long double uses the IEEE

on 2024/4/29 15:20, Alexandre Oliva wrote:
> On Apr 28, 2024, "Kewen.Lin" <linkw@linux.ibm.com> wrote:
> 
>> OK, from this perspective IMHO it seems more clear to adopt xfail
>> with effective target long_double_64bit?
> 
> That's effective target is quite broken, alas.  I doubt it's used
> anywhere: it calls an undefined proc, and its memcmp call seems to have
> the size cut&pasto-ed from the 128-bit functions.  (a patchlet that
> fixes these most glaring issues is below)
> 
> Furthermore, it doesn't really work.  Since it adds -mlong-double-64 for
> the effective target test, it overrides the default, so it sort of
> always passes, even on a 128-bit long double target.  But since the test
> itself doesn't add that option, any xfails on long_double_64bit would be
> flagged as XPASS.
> 
> There's no effective target test for 64-bit long double that doesn't
> override the default, so we'd have to add one.  Alas, the natural name
> for it is the one that's taken with overriding behavior, and the current
> option-overriding tests, that need to be used along with the
> corresponding add-options in testcases, might benefit from a renaming to
> make them fit the already-established (?) naming standards.  Yuck.
> 

Oops, it's really out of my expectation, I just noticed that no test cases
are using this effective target and the commit r12-3151-g4c5d76a655b9ab
contributing this even doesn't adopt it.  Thanks for catching this and sorry
that I didn't check it before suggesting it, I think we can aggressively
drop this effective target instead to avoid any possible confusion.

CC Mike for this.

How about the generic one "longdouble64"?  I did a grep and found it has one
use, I'd expect it can work here. :)

gcc/testsuite//gcc.target/powerpc/pr99708.c:/* { dg-xfail-run-if "unsupported type __ibm128 with long-double-64" { longdouble64 } } */

BR,
Kewen

> 
> diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
> index 182d80129de9b..603da25c97d67 100644
> --- a/gcc/testsuite/lib/target-supports.exp
> +++ b/gcc/testsuite/lib/target-supports.exp
> @@ -2961,12 +2961,12 @@ proc check_effective_target_long_double_64bit { } {
>  	  /* eliminate removing volatile cast warning.  */
>  	  a2 = a;
>  	  b2 = b;
> -	  if (memcmp (&a2, &b2, 16) != 0)
> +	  if (memcmp (&a2, &b2, 8) != 0)
>  	    return 1;
>  	  sprintf (buffer, "%lg", b);
>  	  return strcmp (buffer, "3") != 0;
>  	}
> -    }  [add_options_for_ppc_long_double_override_64bit ""]]
> +    }  [add_options_for_long_double_64bit ""]]
>  }
>  
>  # Return the appropriate options to specify that long double uses the IEEE
> 
>

On Apr 29, 2024, "Kewen.Lin" <linkw@linux.ibm.com> wrote:

> Thanks for catching this and sorry
> that I didn't check it before suggesting it, I think we can aggressively
> drop this effective target instead to avoid any possible confusion.

The 128-bit ones, unfortunately, follow the same pattern but are
probably used.  IMHO we should transition all 3 to an '_ok' suffix, but...

> How about the generic one "longdouble64"?  I did a grep and found it has one
> use, I'd expect it can work here. :)

... since this and longdouble128 exist, maybe we can fix it and leave
them all alone, despite the interface oddity.

on 2024/4/30 07:11, Alexandre Oliva wrote:
> On Apr 29, 2024, "Kewen.Lin" <linkw@linux.ibm.com> wrote:
> 
>> Thanks for catching this and sorry
>> that I didn't check it before suggesting it, I think we can aggressively
>> drop this effective target instead to avoid any possible confusion.
> 
> The 128-bit ones, unfortunately, follow the same pattern but are
> probably used.  IMHO we should transition all 3 to an '_ok' suffix, but...
> 

Yeah, I noticed the 128-bit ones are used, I was just suggesting dropping
check_effective_target_long_double_64bit and add_options_for_long_double_64bit
as there is no user (since release 12 when it's introduced r12-3151), IMHO
there would be not any uses in future, ...

>> How about the generic one "longdouble64"?  I did a grep and found it has one
>> use, I'd expect it can work here. :)
> 
> ... since this and longdouble128 exist, maybe we can fix it and leave
> them all alone, despite the interface oddity.
> 
... personally I'm inclined to drop this 64 bit one. :)

BR,
Kewen