| Message ID | 8bef013a-50e7-f830-9634-a3c012e0dd99@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] rs6000: Guard bifs {un,}pack_{longdouble,ibm128} under hard float [PR103623] | expand |
On Fri, Apr 08, 2022 at 03:09:00PM +0800, Kewen.Lin wrote: > + /* Let's ignore all error messages about built-in function > + unsupported due to soft-float, since they are not test > + points here (this case is to check no ICE). */ > + /* { dg-error ".*" "pr103623" { target *-*-* } .-19 } */ Maybe better /* { dg-excess-errors "pr103623" } */ ? > + double x1 = UNPACK (a, 1); > + LDOUBLE b = PACK (x0, x1); > + LDOUBLE c = bar (b); > + > + return c > a; > +} Jakub
on 2022/4/8 4:04 PM, Jakub Jelinek wrote: > On Fri, Apr 08, 2022 at 03:09:00PM +0800, Kewen.Lin wrote: >> + /* Let's ignore all error messages about built-in function >> + unsupported due to soft-float, since they are not test >> + points here (this case is to check no ICE). */ >> + /* { dg-error ".*" "pr103623" { target *-*-* } .-19 } */ > > Maybe better /* { dg-excess-errors "pr103623" } */ ? > Thanks for the suggestion! It works perfectly as verified (got expected results separately w/ and w/o this fix). BR, Kewen >> + double x1 = UNPACK (a, 1); >> + LDOUBLE b = PACK (x0, x1); >> + LDOUBLE c = bar (b); >> + >> + return c > a; >> +} > > Jakub >
On Fri, Apr 08, 2022 at 03:09:00PM +0800, Kewen.Lin wrote: > As PR103623 shows, it's a regression failure due to new built-in > function framework, previously we guard __builtin_{un,}pack_{longdouble, > ibm128} built-in functions under hard float, so they are unavailable > with the given configuration. While with new bif infrastructure, it > becomes available and gets ICE due to incomplete supports. > > Segher and Peter pointed out that we should make them available with > soft float, I agree we can extend them to cover both soft and hard > float. But considering it's stage 4 now and this regression is > classified as P1, also the previous behavior requiring hard float > aligns with what document [1] says, I guess it may be a good idea to > fix it with the attached small patch to be consistent with the previous > behavior. Then we can extend the functionality in upcoming stage 1. Please answer the questions in <https://gcc.gnu.org/pipermail/gcc-patches/2022-April/592922.html>? Segher
diff --git a/gcc/config/rs6000/rs6000-builtins.def b/gcc/config/rs6000/rs6000-builtins.def index 0f527c5d78f..1030f5aa4cd 100644 --- a/gcc/config/rs6000/rs6000-builtins.def +++ b/gcc/config/rs6000/rs6000-builtins.def @@ -220,7 +220,7 @@ ; This is redundant with __builtin_pack_ibm128, as it requires long ; double to be __ibm128. Should probably be deprecated. const long double __builtin_pack_longdouble (double, double); - PACK_TF packtf {ibmld} + PACK_TF packtf {ibmld,nosoft} unsigned long __builtin_ppc_mftb (); MFTB rs6000_mftb_di {32bit} @@ -235,18 +235,18 @@ MTFSF rs6000_mtfsf {} const __ibm128 __builtin_pack_ibm128 (double, double); - PACK_IF packif {ibm128} + PACK_IF packif {ibm128,nosoft} void __builtin_set_fpscr_rn (const int[0,3]); SET_FPSCR_RN rs6000_set_fpscr_rn {nosoft} const double __builtin_unpack_ibm128 (__ibm128, const int<1>); - UNPACK_IF unpackif {ibm128} + UNPACK_IF unpackif {ibm128,nosoft} ; This is redundant with __builtin_unpack_ibm128, as it requires long ; double to be __ibm128. Should probably be deprecated. const double __builtin_unpack_longdouble (long double, const int<1>); - UNPACK_TF unpacktf {ibmld} + UNPACK_TF unpacktf {ibmld,nosoft} ; Builtins that have been around just about forever, but not quite. diff --git a/gcc/testsuite/gcc.target/powerpc/pr103623.c b/gcc/testsuite/gcc.target/powerpc/pr103623.c new file mode 100644 index 00000000000..d6bd47c630f --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr103623.c @@ -0,0 +1,47 @@ +/* { dg-skip-if "" { powerpc*-*-darwin* } } */ +/* { dg-options "-mlong-double-128 -msoft-float" } */ + +/* Verify there is no ICE. */ + +#include <stddef.h> +#include <stdlib.h> +#include <math.h> + +#if defined(__LONG_DOUBLE_IEEE128__) +/* If long double is IEEE 128-bit, we need to use the __ibm128 type instead of + long double, and to use the appropriate pack/unpack routines. We can't use + __ibm128 on systems that don't support IEEE 128-bit floating point, because + the type is not enabled on those systems. */ +#define PACK __builtin_pack_ibm128 +#define UNPACK __builtin_unpack_ibm128 +#define LDOUBLE __ibm128 + +#elif defined(__LONG_DOUBLE_IBM128__) +#define PACK __builtin_pack_longdouble +#define UNPACK __builtin_unpack_longdouble +#define LDOUBLE long double + +#else +#error "long double must be either IBM 128-bit or IEEE 128-bit" +#endif + +extern LDOUBLE bar (LDOUBLE); + +int +main (void) +{ + double high = pow (2.0, 60); + double low = 2.0; + LDOUBLE a = ((LDOUBLE) high) + ((LDOUBLE) low); + double x0 = UNPACK (a, 0); + /* Let's ignore all error messages about built-in function + unsupported due to soft-float, since they are not test + points here (this case is to check no ICE). */ + /* { dg-error ".*" "pr103623" { target *-*-* } .-19 } */ + double x1 = UNPACK (a, 1); + LDOUBLE b = PACK (x0, x1); + LDOUBLE c = bar (b); + + return c > a; +} +
Hi, As PR103623 shows, it's a regression failure due to new built-in function framework, previously we guard __builtin_{un,}pack_{longdouble, ibm128} built-in functions under hard float, so they are unavailable with the given configuration. While with new bif infrastructure, it becomes available and gets ICE due to incomplete supports. Segher and Peter pointed out that we should make them available with soft float, I agree we can extend them to cover both soft and hard float. But considering it's stage 4 now and this regression is classified as P1, also the previous behavior requiring hard float aligns with what document [1] says, I guess it may be a good idea to fix it with the attached small patch to be consistent with the previous behavior. Then we can extend the functionality in upcoming stage 1. Bootstrapped and regtested on powerpc64-linux-gnu P8 and powerpc64le-linux-gnu P9 and P10. [1] https://gcc.gnu.org/onlinedocs/gcc/Basic-PowerPC-Built-in-Functions-Available-on-ISA-2_002e05.html#Basic-PowerPC-Built-in-Functions-Available-on-ISA-2_002e05 v1: https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591147.html v2: Rebase and add one test case. I'm not sure about dg-skip-if thing in test case is required or not which refers to the existing case gcc.target/powerpc/pack02.c. I can't test it but considering it's stage 4 end now, I tried to be more conservative. Any thoughts? Is it ok for trunk? BR, Kewen ------ PR target/103623 gcc/ChangeLog: * config/rs6000/rs6000-builtins.def (__builtin_pack_longdouble): Add nosoft attribute. (__builtin_unpack_longdouble): Likewise. (__builtin_pack_ibm128): Likewise. (__builtin_unpack_ibm128): Likewise. gcc/testsuite/ChangeLog: * gcc.target/powerpc/pr103623.c: New test. --- gcc/config/rs6000/rs6000-builtins.def | 8 ++-- gcc/testsuite/gcc.target/powerpc/pr103623.c | 47 +++++++++++++++++++++ 2 files changed, 51 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/gcc.target/powerpc/pr103623.c -- 2.27.0