[PATCH] rs6000: Guard bifs {un,}pack_{longdouble,ibm128} under hard float [PR103623]

[PATCH] rs6000: Guard bifs {un,}pack_{longdouble,ibm128} under hard float [PR103623]

[Kewen.Lin]

[-- Attachment #1: Type: text/plain, Size: 1346 bytes --]

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.

Any thoughts?

[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

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.


[-- Attachment #2: pr103623.patch --]
[-- Type: text/plain, Size: 1414 bytes --]

---
 gcc/config/rs6000/rs6000-builtins.def | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gcc/config/rs6000/rs6000-builtins.def b/gcc/config/rs6000/rs6000-builtins.def
index ae2760c3338..b78ec525d5f 100644
--- a/gcc/config/rs6000/rs6000-builtins.def
+++ b/gcc/config/rs6000/rs6000-builtins.def
@@ -219,7 +219,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}
@@ -234,18 +234,18 @@
     MTFSF rs6000_mtfsf {}
 
   const __ibm128 __builtin_pack_ibm128 (double, double);
-    PACK_IF packif {}
+    PACK_IF packif {nosoft}
 
   void __builtin_set_fpscr_rn (const int[0,3]);
     SET_FPSCR_RN rs6000_set_fpscr_rn {}
 
   const double __builtin_unpack_ibm128 (__ibm128, const int<1>);
-    UNPACK_IF unpackif {}
+    UNPACK_IF unpackif {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.
-- 
2.27.0

PING^1 [PATCH] rs6000: Guard bifs {un, }pack_{longdouble, ibm128} under hard float [PR103623]

[Kewen.Lin]

Hi,

Gentle ping: https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591147.html

BR,
Kewen

on 2022/3/3 10:11 AM, Kewen.Lin via Gcc-patches wrote:
> 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.
> 
> Any thoughts?
> 
> [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
> 
> 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.
> 

Re: [PATCH] rs6000: Guard bifs {un, }pack_{longdouble, ibm128} under hard float [PR103623]

[Segher Boessenkool]

Hi!

On Thu, Mar 03, 2022 at 10:11:32AM +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.

Or you could just not take away the existing functionality.

What makes it ICE on (at least some configurations of) 32-bit now?  Can
you exclude just 32-bit soft float?


Segher

Re: [PATCH] rs6000: Guard bifs {un, }pack_{longdouble, ibm128} under hard float [PR103623]

[Jakub Jelinek]

On Thu, Apr 07, 2022 at 06:09:52AM -0500, Segher Boessenkool wrote:
> Hi!
> 
> On Thu, Mar 03, 2022 at 10:11:32AM +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.
> 
> Or you could just not take away the existing functionality.

Have those builtins ever worked with 64-bit soft-float?
When I try e.g. gcc 11 on the #c0 testcase from the PR, I get:
./cc1.r11-1 -quiet -nostdinc pr103623.c -m64 -mlong-double-128 -msoft-float
pr103623.c: In function ‘main’:
pr103623.c:2:16: error: ‘__builtin_unpack_longdouble’ requires the ‘-mhard-float’ option
    2 | #define UNPACK __builtin_unpack_longdouble
      |                ^
pr103623.c:11:15: note: in expansion of macro ‘UNPACK’
   11 |   double x0 = UNPACK (a, 0);
      |               ^~~~~~

From what I can see, those builtins were using:
/* 128-bit long double floating point builtins.  */
#define BU_LDBL128_2(ENUM, NAME, ATTR, ICODE)                           \
  RS6000_BUILTIN_2 (MISC_BUILTIN_ ## ENUM,              /* ENUM */      \
                    "__builtin_" NAME,                  /* NAME */      \
                    (RS6000_BTM_HARD_FLOAT              /* MASK */      \
                     | RS6000_BTM_LDBL128),                             \
                    (RS6000_BTC_ ## ATTR                /* ATTR */      \
                     | RS6000_BTC_BINARY),                              \
                    CODE_FOR_ ## ICODE)                 /* ICODE */
   
/* 128-bit __ibm128 floating point builtins (use -mfloat128 to indicate that
   __ibm128 is available).  */
#define BU_IBM128_2(ENUM, NAME, ATTR, ICODE)                            \
  RS6000_BUILTIN_2 (MISC_BUILTIN_ ## ENUM,              /* ENUM */      \
                    "__builtin_" NAME,                  /* NAME */      \
                    (RS6000_BTM_HARD_FLOAT              /* MASK */      \
                     | RS6000_BTM_FLOAT128),                            \
                    (RS6000_BTC_ ## ATTR                /* ATTR */      \
                     | RS6000_BTC_BINARY),                              \
                    CODE_FOR_ ## ICODE)                 /* ICODE */
macros and rs6000_builtin_is_supported_p was checking whether
all the bits in the mask are set:
  HOST_WIDE_INT fnmask = rs6000_builtin_info[fncode].mask;
  if ((fnmask & rs6000_builtin_mask) != fnmask)
    return false;
  else
    return true;
(so logical and of all of them).

	Jakub

Re: [PATCH] rs6000: Guard bifs {un, }pack_{longdouble, ibm128} under hard float [PR103623]

[Kewen.Lin]

Hi Segher & Jakub,

Thanks for the comments!

on 2022/4/7 9:00 PM, Jakub Jelinek wrote:
> On Thu, Apr 07, 2022 at 06:09:52AM -0500, Segher Boessenkool wrote:
>> Hi!
>>
>> On Thu, Mar 03, 2022 at 10:11:32AM +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.
>>
>> Or you could just not take away the existing functionality.
> 
> Have those builtins ever worked with 64-bit soft-float?
> When I try e.g. gcc 11 on the #c0 testcase from the PR, I get:
> ./cc1.r11-1 -quiet -nostdinc pr103623.c -m64 -mlong-double-128 -msoft-float
> pr103623.c: In function ‘main’:
> pr103623.c:2:16: error: ‘__builtin_unpack_longdouble’ requires the ‘-mhard-float’ option
>     2 | #define UNPACK __builtin_unpack_longdouble
>       |                ^
> pr103623.c:11:15: note: in expansion of macro ‘UNPACK’
>    11 |   double x0 = UNPACK (a, 0);
>       |               ^~~~~~
> 
> From what I can see, those builtins were using:
> /* 128-bit long double floating point builtins.  */
> #define BU_LDBL128_2(ENUM, NAME, ATTR, ICODE)                           \
>   RS6000_BUILTIN_2 (MISC_BUILTIN_ ## ENUM,              /* ENUM */      \
>                     "__builtin_" NAME,                  /* NAME */      \
>                     (RS6000_BTM_HARD_FLOAT              /* MASK */      \
>                      | RS6000_BTM_LDBL128),                             \
>                     (RS6000_BTC_ ## ATTR                /* ATTR */      \
>                      | RS6000_BTC_BINARY),                              \
>                     CODE_FOR_ ## ICODE)                 /* ICODE */
>    
> /* 128-bit __ibm128 floating point builtins (use -mfloat128 to indicate that
>    __ibm128 is available).  */
> #define BU_IBM128_2(ENUM, NAME, ATTR, ICODE)                            \
>   RS6000_BUILTIN_2 (MISC_BUILTIN_ ## ENUM,              /* ENUM */      \
>                     "__builtin_" NAME,                  /* NAME */      \
>                     (RS6000_BTM_HARD_FLOAT              /* MASK */      \
>                      | RS6000_BTM_FLOAT128),                            \
>                     (RS6000_BTC_ ## ATTR                /* ATTR */      \
>                      | RS6000_BTC_BINARY),                              \
>                     CODE_FOR_ ## ICODE)                 /* ICODE */
> macros and rs6000_builtin_is_supported_p was checking whether
> all the bits in the mask are set:
>   HOST_WIDE_INT fnmask = rs6000_builtin_info[fncode].mask;
>   if ((fnmask & rs6000_builtin_mask) != fnmask)
>     return false;
>   else
>     return true;
> (so logical and of all of them).
> 

As Jakub noted here, we don't have the soft-float support for both m32 and m64
before, as the bifs are always guarded under hard-float previously.  I just did
a further check with a powerpc-e300c3-linux-gnu cross-build, the ICE exists for
both m32 and m64.

I guessed the discussions in the PR saying a little more difficulty on m32 than
m64 for extending with soft-float support caused the confusion?

>> What makes it ICE on (at least some configurations of) 32-bit now?  Can
>> you exclude just 32-bit soft float?

As clarified above, both 32-bit and 64-bit has the same root cause for the ICE,
the existing define_insn* supports for these bifs only consider hard-float, such
as for the given test case in the PR, it fails in reload as the recognized
unpacktf_nodm doesn't have any available alternatives at soft-float.  eg: we only
have register constraint "d" for
  (match_operand:FMOVE128 1 "register_operand" "d,d") 
but it's only available for hard-float.

BR,
Kewen

Re: [PATCH] rs6000: Guard bifs {un, }pack_{longdouble, ibm128} under hard float [PR103623]

[Segher Boessenkool]

On Fri, Apr 08, 2022 at 10:09:44AM +0800, Kewen.Lin wrote:
> As Jakub noted here, we don't have the soft-float support for both m32 and m64
> before, as the bifs are always guarded under hard-float previously.

But that bug was fixed for GCC 12.  Or we thought so, at least :-(

> >> What makes it ICE on (at least some configurations of) 32-bit now?  Can
> >> you exclude just 32-bit soft float?
> 
> As clarified above, both 32-bit and 64-bit has the same root cause for the ICE,
> the existing define_insn* supports for these bifs only consider hard-float, such
> as for the given test case in the PR, it fails in reload as the recognized
> unpacktf_nodm doesn't have any available alternatives at soft-float.  eg: we only
> have register constraint "d" for
>   (match_operand:FMOVE128 1 "register_operand" "d,d") 
> but it's only available for hard-float.

For me it fails during combine: the unspec suddenly doesn't recog
anymore.  That might be that "d" thing yes, that is problematical.

If you want to add "nosoft" now, please add a FIXME comment everywhere
you do, so we do not forget to fix this for GCC 13.

Or, try this patch?

===
diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index fdfbc6566a5c..f05b8358ba0a 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -14580,10 +14580,10 @@ (define_insn_and_split "unpack<mode>_dm"
   [(set_attr "type" "fp,fpstore,mtvsr,mfvsr,store")])
 
 (define_insn_and_split "unpack<mode>_nodm"
-  [(set (match_operand:<FP128_64> 0 "nonimmediate_operand" "=d,m")
+  [(set (match_operand:<FP128_64> 0 "nonimmediate_operand" "=d,m,m")
        (unspec:<FP128_64>
-        [(match_operand:FMOVE128 1 "register_operand" "d,d")
-         (match_operand:QI 2 "const_0_to_1_operand" "i,i")]
+        [(match_operand:FMOVE128 1 "register_operand" "d,d,r")
+         (match_operand:QI 2 "const_0_to_1_operand" "i,i,i")]
         UNSPEC_UNPACK_128BIT))]
   "(!TARGET_POWERPC64 || !TARGET_DIRECT_MOVE) && FLOAT128_2REG_P (<MODE>mode)"
   "#"
@@ -14600,7 +14600,7 @@ (define_insn_and_split "unpack<mode>_nodm"
 
   operands[3] = gen_rtx_REG (<FP128_64>mode, fp_regno);
 }
-  [(set_attr "type" "fp,fpstore")])
+  [(set_attr "type" "fp,fpstore,store")])
 
 (define_insn_and_split "pack<mode>"
   [(set (match_operand:FMOVE128 0 "register_operand" "=&d")
===


Segher

Re: [PATCH] rs6000: Guard bifs {un, }pack_{longdouble, ibm128} under hard float [PR103623]

[Kewen.Lin]

Hi Segher,

on 2022/4/9 1:31 AM, Segher Boessenkool wrote:
> On Fri, Apr 08, 2022 at 10:09:44AM +0800, Kewen.Lin wrote:
>> As Jakub noted here, we don't have the soft-float support for both m32 and m64
>> before, as the bifs are always guarded under hard-float previously.
> 
> But that bug was fixed for GCC 12.  Or we thought so, at least :-(
> 

Actually it wasn't fixed due to the incomplete support.  :(

>>>> What makes it ICE on (at least some configurations of) 32-bit now?  Can
>>>> you exclude just 32-bit soft float?
>>
>> As clarified above, both 32-bit and 64-bit has the same root cause for the ICE,
>> the existing define_insn* supports for these bifs only consider hard-float, such
>> as for the given test case in the PR, it fails in reload as the recognized
>> unpacktf_nodm doesn't have any available alternatives at soft-float.  eg: we only
>> have register constraint "d" for
>>   (match_operand:FMOVE128 1 "register_operand" "d,d") 
>> but it's only available for hard-float.
> 
> For me it fails during combine: the unspec suddenly doesn't recog
> anymore.  That might be that "d" thing yes, that is problematical.
> 

I must miss something, I found in combine pass we still have the
the insn_code unpacktf_nodm (recog-ed).

> If you want to add "nosoft" now, please add a FIXME comment everywhere
> you do, so we do not forget to fix this for GCC 13.
> 

Does the patch v3 look good to you?

https://gcc.gnu.org/pipermail/gcc-patches/2022-April/593053.html

> Or, try this patch?
> 
> ===
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index fdfbc6566a5c..f05b8358ba0a 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -14580,10 +14580,10 @@ (define_insn_and_split "unpack<mode>_dm"
>    [(set_attr "type" "fp,fpstore,mtvsr,mfvsr,store")])
>  
>  (define_insn_and_split "unpack<mode>_nodm"
> -  [(set (match_operand:<FP128_64> 0 "nonimmediate_operand" "=d,m")
> +  [(set (match_operand:<FP128_64> 0 "nonimmediate_operand" "=d,m,m")
>         (unspec:<FP128_64>
> -        [(match_operand:FMOVE128 1 "register_operand" "d,d")
> -         (match_operand:QI 2 "const_0_to_1_operand" "i,i")]
> +        [(match_operand:FMOVE128 1 "register_operand" "d,d,r")
> +         (match_operand:QI 2 "const_0_to_1_operand" "i,i,i")]
>          UNSPEC_UNPACK_128BIT))]
>    "(!TARGET_POWERPC64 || !TARGET_DIRECT_MOVE) && FLOAT128_2REG_P (<MODE>mode)"
>    "#"
> @@ -14600,7 +14600,7 @@ (define_insn_and_split "unpack<mode>_nodm"
>  
>    operands[3] = gen_rtx_REG (<FP128_64>mode, fp_regno);
>  }
> -  [(set_attr "type" "fp,fpstore")])
> +  [(set_attr "type" "fp,fpstore,store")])
>  
>  (define_insn_and_split "pack<mode>"
>    [(set (match_operand:FMOVE128 0 "register_operand" "=&d")
> ===
> 
> 

Nice, I confirmed this makes ICE gone, I've filed one new PR
PR105213 for GCC13 further tracking by associating this patch there.

BR,
Kewen

Re: [PATCH] rs6000: Guard bifs {un, }pack_{longdouble, ibm128} under hard float [PR103623]

[Segher Boessenkool]

Hi!

On Mon, Apr 11, 2022 at 04:29:40PM +0800, Kewen.Lin wrote:
> on 2022/4/9 1:31 AM, Segher Boessenkool wrote:
> > On Fri, Apr 08, 2022 at 10:09:44AM +0800, Kewen.Lin wrote:
> > For me it fails during combine: the unspec suddenly doesn't recog
> > anymore.  That might be that "d" thing yes, that is problematical.
> > 
> 
> I must miss something, I found in combine pass we still have the
> the insn_code unpacktf_nodm (recog-ed).

That is recognised many passes earlier though.  When combine runs it
will ICE because recog failed (for powerpc64-linux anyway, everything
default, no -mcpu= etc.).

> > Or, try this patch?
> > 
> > ===
> > diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> > index fdfbc6566a5c..f05b8358ba0a 100644
> > --- a/gcc/config/rs6000/rs6000.md
> > +++ b/gcc/config/rs6000/rs6000.md
> > @@ -14580,10 +14580,10 @@ (define_insn_and_split "unpack<mode>_dm"
> >    [(set_attr "type" "fp,fpstore,mtvsr,mfvsr,store")])
> >  
> >  (define_insn_and_split "unpack<mode>_nodm"
> > -  [(set (match_operand:<FP128_64> 0 "nonimmediate_operand" "=d,m")
> > +  [(set (match_operand:<FP128_64> 0 "nonimmediate_operand" "=d,m,m")
> >         (unspec:<FP128_64>
> > -        [(match_operand:FMOVE128 1 "register_operand" "d,d")
> > -         (match_operand:QI 2 "const_0_to_1_operand" "i,i")]
> > +        [(match_operand:FMOVE128 1 "register_operand" "d,d,r")
> > +         (match_operand:QI 2 "const_0_to_1_operand" "i,i,i")]
> >          UNSPEC_UNPACK_128BIT))]
> >    "(!TARGET_POWERPC64 || !TARGET_DIRECT_MOVE) && FLOAT128_2REG_P (<MODE>mode)"
> >    "#"
> > @@ -14600,7 +14600,7 @@ (define_insn_and_split "unpack<mode>_nodm"
> >  
> >    operands[3] = gen_rtx_REG (<FP128_64>mode, fp_regno);
> >  }
> > -  [(set_attr "type" "fp,fpstore")])
> > +  [(set_attr "type" "fp,fpstore,store")])
> >  
> >  (define_insn_and_split "pack<mode>"
> >    [(set (match_operand:FMOVE128 0 "register_operand" "=&d")
> > ===
> > 
> > 
> 
> Nice, I confirmed this makes ICE gone, I've filed one new PR
> PR105213 for GCC13 further tracking by associating this patch there.

Cool, I'll commit it later today then (after a final regstrap).  The
_nodm pattern just missed the alternative for no FP regs (the _dm
pattern has it, so just an oversight).



Segher

Re: [PATCH] rs6000: Guard bifs {un, }pack_{longdouble, ibm128} under hard float [PR103623]

[Kewen.Lin]

on 2022/4/11 11:42 PM, Segher Boessenkool wrote:
> Hi!
> 
> On Mon, Apr 11, 2022 at 04:29:40PM +0800, Kewen.Lin wrote:
>> on 2022/4/9 1:31 AM, Segher Boessenkool wrote:
>>> On Fri, Apr 08, 2022 at 10:09:44AM +0800, Kewen.Lin wrote:
>>> For me it fails during combine: the unspec suddenly doesn't recog
>>> anymore.  That might be that "d" thing yes, that is problematical.
>>>
>>
>> I must miss something, I found in combine pass we still have the
>> the insn_code unpacktf_nodm (recog-ed).
> 
> That is recognised many passes earlier though.  When combine runs it
> will ICE because recog failed (for powerpc64-linux anyway, everything
> default, no -mcpu= etc.).
> 

OK, I guess that's why I didn't meet this ICE as I specified -mcpu=
for testing all the time.

>>> Or, try this patch?
>>>
>>> ===
>>> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
>>> index fdfbc6566a5c..f05b8358ba0a 100644
>>> --- a/gcc/config/rs6000/rs6000.md
>>> +++ b/gcc/config/rs6000/rs6000.md
>>> @@ -14580,10 +14580,10 @@ (define_insn_and_split "unpack<mode>_dm"
>>>    [(set_attr "type" "fp,fpstore,mtvsr,mfvsr,store")])
>>>  
>>>  (define_insn_and_split "unpack<mode>_nodm"
>>> -  [(set (match_operand:<FP128_64> 0 "nonimmediate_operand" "=d,m")
>>> +  [(set (match_operand:<FP128_64> 0 "nonimmediate_operand" "=d,m,m")
>>>         (unspec:<FP128_64>
>>> -        [(match_operand:FMOVE128 1 "register_operand" "d,d")
>>> -         (match_operand:QI 2 "const_0_to_1_operand" "i,i")]
>>> +        [(match_operand:FMOVE128 1 "register_operand" "d,d,r")
>>> +         (match_operand:QI 2 "const_0_to_1_operand" "i,i,i")]
>>>          UNSPEC_UNPACK_128BIT))]
>>>    "(!TARGET_POWERPC64 || !TARGET_DIRECT_MOVE) && FLOAT128_2REG_P (<MODE>mode)"
>>>    "#"
>>> @@ -14600,7 +14600,7 @@ (define_insn_and_split "unpack<mode>_nodm"
>>>  
>>>    operands[3] = gen_rtx_REG (<FP128_64>mode, fp_regno);
>>>  }
>>> -  [(set_attr "type" "fp,fpstore")])
>>> +  [(set_attr "type" "fp,fpstore,store")])
>>>  
>>>  (define_insn_and_split "pack<mode>"
>>>    [(set (match_operand:FMOVE128 0 "register_operand" "=&d")
>>> ===
>>>
>>>
>>
>> Nice, I confirmed this makes ICE gone, I've filed one new PR
>> PR105213 for GCC13 further tracking by associating this patch there.
> 
> Cool, I'll commit it later today then (after a final regstrap).  The
> _nodm pattern just missed the alternative for no FP regs (the _dm
> pattern has it, so just an oversight).
> 

Thanks!  So it can be counted as a regression fix instead of tiny
feature work?  Maybe we also need bif documentation change, and
gcc12 changes html update (as bif behavior changes), or it's
too small so no?

BR,
Kewen

Re: [PATCH] rs6000: Guard bifs {un, }pack_{longdouble, ibm128} under hard float [PR103623]

[Segher Boessenkool]

On Tue, Apr 12, 2022 at 10:02:06AM +0800, Kewen.Lin wrote:
> on 2022/4/11 11:42 PM, Segher Boessenkool wrote:
> > On Mon, Apr 11, 2022 at 04:29:40PM +0800, Kewen.Lin wrote:
> >> Nice, I confirmed this makes ICE gone, I've filed one new PR
> >> PR105213 for GCC13 further tracking by associating this patch there.
> > 
> > Cool, I'll commit it later today then (after a final regstrap).  The
> > _nodm pattern just missed the alternative for no FP regs (the _dm
> > pattern has it, so just an oversight).
> 
> Thanks!  So it can be counted as a regression fix instead of tiny
> feature work?  Maybe we also need bif documentation change, and
> gcc12 changes html update (as bif behavior changes), or it's
> too small so no?

It is just a bugfix.  Everything in anything that is not a new feature
is arguably a regression fix, so that classification isn't useful
normally.  I'd rather fix more bugs than do more filing work ;-)

The documentation already has this under "The following built-in
functions are also available on all PowerPC processors:", so no doc
changes are needed either: this is a bugfix! :-)


Segher

Re: [PATCH] rs6000: Guard bifs {un, }pack_{longdouble, ibm128} under hard float [PR103623]

[Kewen.Lin]

on 2022/4/13 3:11 AM, Segher Boessenkool wrote:
> On Tue, Apr 12, 2022 at 10:02:06AM +0800, Kewen.Lin wrote:
>> on 2022/4/11 11:42 PM, Segher Boessenkool wrote:
>>> On Mon, Apr 11, 2022 at 04:29:40PM +0800, Kewen.Lin wrote:
>>>> Nice, I confirmed this makes ICE gone, I've filed one new PR
>>>> PR105213 for GCC13 further tracking by associating this patch there.
>>>
>>> Cool, I'll commit it later today then (after a final regstrap).  The
>>> _nodm pattern just missed the alternative for no FP regs (the _dm
>>> pattern has it, so just an oversight).
>>
>> Thanks!  So it can be counted as a regression fix instead of tiny
>> feature work?  Maybe we also need bif documentation change, and
>> gcc12 changes html update (as bif behavior changes), or it's
>> too small so no?
> 
> It is just a bugfix.  Everything in anything that is not a new feature
> is arguably a regression fix, so that classification isn't useful
> normally.  I'd rather fix more bugs than do more filing work ;-)
> 
> The documentation already has this under "The following built-in
> functions are also available on all PowerPC processors:", so no doc
> changes are needed either: this is a bugfix! :-)
> 
> 
Thanks for the explanation!!

As to the documentation, there seem some contradictions.

On page[1], we only have the description for {un,}pack_{ibm128}, that is:

"
The following built-in functions are also available on all PowerPC processors:

uint64_t __builtin_ppc_get_timebase ();
unsigned long __builtin_ppc_mftb ();
double __builtin_unpack_ibm128 (__ibm128, int);
__ibm128 __builtin_pack_ibm128 (double, double);
double __builtin_mffs (void);
void __builtin_mtfsf (const int, double);
void __builtin_mtfsb0 (const int);
void __builtin_mtfsb1 (const int);
void __builtin_set_fpscr_rn (int);
"

On page[2], we have the description for both {un,}pack_{ibm128} and {un,}pack_{longdouble}

"
**The following functions require -mhard-float and -mmultiple options.**

The __builtin_unpack_longdouble function takes a long double argument and a compile time constant of 0 or 1. If the constant is 0, the first double within the long double is returned, otherwise the second double is returned. The __builtin_unpack_longdouble function is only available if long double uses the IBM extended double representation.

The __builtin_pack_longdouble function takes two double arguments and returns a long double value that combines the two arguments. The __builtin_pack_longdouble function is only available if long double uses the IBM extended double representation.

The __builtin_unpack_ibm128 function takes a __ibm128 argument and a compile time constant of 0 or 1. If the constant is 0, the first double within the __ibm128 is returned, otherwise the second double is returned.

The __builtin_pack_ibm128 function takes two double arguments and returns a __ibm128 value that combines the two arguments.

Additional built-in functions are available for the 64-bit PowerPC family of processors, for efficient use of 128-bit floating point (__float128) values. 
"

By comparing the above, we seemed to place the documentation for {un,}pack_{ibm128} in the former wrongly.
And need to update page[2] according to the recent change?

[1] https://gcc.gnu.org/onlinedocs/gcc/Basic-PowerPC-Built-in-Functions-Available-on-all-Configurations.html#Basic-PowerPC-Built-in-Functions-Available-on-all-Configurations
[2] 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

BR,
Kewen

Re: [PATCH] rs6000: Guard bifs {un, }pack_{longdouble, ibm128} under hard float [PR103623]

[Segher Boessenkool]

On Thu, Apr 07, 2022 at 03:00:14PM +0200, Jakub Jelinek wrote:
> On Thu, Apr 07, 2022 at 06:09:52AM -0500, Segher Boessenkool wrote:
> > On Thu, Mar 03, 2022 at 10:11:32AM +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.
> > 
> > Or you could just not take away the existing functionality.
> 
> Have those builtins ever worked with 64-bit soft-float?

I thought they did right now, but apparently not :-(

> When I try e.g. gcc 11 on the #c0 testcase from the PR, I get:

> >From what I can see, those builtins were using:

Yes, and we fixed that bug.

Now everything is fine until the insn

(insn 6 5 8 2 (set (reg/v:DF 117 [ x0 ])
        (unspec:DF [
                (reg:TF 119)
                (const_int 0 [0])
            ] UNSPEC_UNPACK_128BIT)) "103623.c":11:15 1067 {unpacktf_nodm}
     (expr_list:REG_DEAD (reg:TF 119)
        (expr_list:REG_EQUAL (unspec:DF [
                    (const_double:TF 1.152921504606846978e+18 [0x0.8000000000000
01p+61])
                    (const_int 0 [0])
                ] UNSPEC_UNPACK_128BIT)
            (nil))))

no longer recognises during combine.  It did in earlier passes, and
nothing changed as far as I can see.  Pretty baffled.


Segher