| Message ID | b83edabe-b4f7-18f2-2486-a4287014c372@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | rs6000: Guard bifs {un,}pack_{longdouble,ibm128} under hard float [PR103623] | expand |
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. >
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
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
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
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
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
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
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
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
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
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
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.
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. --- gcc/config/rs6000/rs6000-builtins.def | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)