| Message ID | 1521041228.7699.7.camel@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | PR 844422 Fix FCTID, FCTIW with -mcpu=power7 | expand |
Hi Carl, On Wed, Mar 14, 2018 at 08:27:08AM -0700, Carl Love wrote: > The following patch fixes an ICE when compiling the test case > > gcc -mcpu=power7 builtin-fctid-fctiw-runnable.c > > The GCC compiler now gives a message > > "error: builtin function ‘__builtin_fctiw’ requires the ‘-mpower8-vector’ option" > > and exits without generating an internal error. This is an improvement over the ICE, for sure. But fctiw is an ISA 1.xx instruction already (and fctid is as well, but explicitly undefined for 32-bit implementations until some 2.xx, I forgot which, 2.02 I think?) Requiring power8 for it is weird and surprising. power8-vector doubly so. It does not seem to be a good idea to only enable the builtin for cases where the current implementation is not broken. (The issue is that pre-power8 we do not allow SImode in FPR registers. Which makes the current fctiw implementation fail). I think we have two good options: 1) Remove these builtins; or 2) Make them work. Segher
On Mar 16, 2018, at 5:51 PM, Segher Boessenkool <segher@kernel.crashing.org> wrote: > > Hi Carl, > > On Wed, Mar 14, 2018 at 08:27:08AM -0700, Carl Love wrote: >> The following patch fixes an ICE when compiling the test case >> >> gcc -mcpu=power7 builtin-fctid-fctiw-runnable.c >> >> The GCC compiler now gives a message >> >> "error: builtin function ‘__builtin_fctiw’ requires the ‘-mpower8-vector’ option" >> >> and exits without generating an internal error. > > This is an improvement over the ICE, for sure. > > But fctiw is an ISA 1.xx instruction already (and fctid is as well, > but explicitly undefined for 32-bit implementations until some 2.xx, > I forgot which, 2.02 I think?) > > Requiring power8 for it is weird and surprising. power8-vector doubly so. > > It does not seem to be a good idea to only enable the builtin for cases > where the current implementation is not broken. > > (The issue is that pre-power8 we do not allow SImode in FPR registers. > Which makes the current fctiw implementation fail). > > I think we have two good options: > > 1) Remove these builtins; > or > 2) Make them work. > I don't think we can remove them, as for a while they were the only way to access these instructions. We now have some vec_* intrinsics that are the preferred interfaces, but for backward compatibility I think we have to keep the old ones. Thanks, Bill > > Segher >
> On Mar 19, 2018, at 8:19 AM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote: > > On Mar 16, 2018, at 5:51 PM, Segher Boessenkool <segher@kernel.crashing.org> wrote: >> >> Hi Carl, >> >> On Wed, Mar 14, 2018 at 08:27:08AM -0700, Carl Love wrote: >>> The following patch fixes an ICE when compiling the test case >>> >>> gcc -mcpu=power7 builtin-fctid-fctiw-runnable.c >>> >>> The GCC compiler now gives a message >>> >>> "error: builtin function ‘__builtin_fctiw’ requires the ‘-mpower8-vector’ option" >>> >>> and exits without generating an internal error. >> >> This is an improvement over the ICE, for sure. >> >> But fctiw is an ISA 1.xx instruction already (and fctid is as well, >> but explicitly undefined for 32-bit implementations until some 2.xx, >> I forgot which, 2.02 I think?) >> >> Requiring power8 for it is weird and surprising. power8-vector doubly so. >> >> It does not seem to be a good idea to only enable the builtin for cases >> where the current implementation is not broken. >> >> (The issue is that pre-power8 we do not allow SImode in FPR registers. >> Which makes the current fctiw implementation fail). >> >> I think we have two good options: >> >> 1) Remove these builtins; >> or >> 2) Make them work. >> > I don't think we can remove them, as for a while they were the only way to > access these instructions. We now have some vec_* intrinsics that are > the preferred interfaces, but for backward compatibility I think we have to > keep the old ones. Sorry, never mind, I was thinking of vector forms. Please ignore me. Bill > > Thanks, > Bill > >> >> Segher
On Mon, Mar 19, 2018 at 08:19:18AM -0500, Bill Schmidt wrote: > > Requiring power8 for it is weird and surprising. power8-vector doubly so. > > > > It does not seem to be a good idea to only enable the builtin for cases > > where the current implementation is not broken. > > > > (The issue is that pre-power8 we do not allow SImode in FPR registers. > > Which makes the current fctiw implementation fail). > > > > I think we have two good options: > > > > 1) Remove these builtins; > > or > > 2) Make them work. > > > I don't think we can remove them, as for a while they were the only way to > access these instructions. We now have some vec_* intrinsics that are > the preferred interfaces, but for backward compatibility I think we have to > keep the old ones. It is new for GCC 8, added in r253238. We can still remove it for GCC 8 if it needs more work; or we can fix it. I'd rather not ship a broken feature (that no one yet uses). Segher
On 3/16/18 5:51 PM, Segher Boessenkool wrote: > But fctiw is an ISA 1.xx instruction already (and fctid is as well, > but explicitly undefined for 32-bit implementations until some 2.xx, > I forgot which, 2.02 I think?) > > Requiring power8 for it is weird and surprising. power8-vector doubly so. Completely agree. > (The issue is that pre-power8 we do not allow SImode in FPR registers. > Which makes the current fctiw implementation fail). A similar issue was true of SDmode, in that we couldn't save/restore SDmode values out of FP regs without corrupting them. I can say I didn't like the solution we were forced to use. :-( > I think we have two good options: > > 1) Remove these builtins; > or > 2) Make them work. Agreed. If we make them work, then the pattern that was added in revision 253238 should be fixed. The type of the source operand is defined as DFmode, but the pattern is named as a SFmode to SImode conversion. (define_insn "lrintsfsi2" [(set (match_operand:SI 0 "gpc_reg_operand" "=d") (unspec:SI [(match_operand:DF 1 "gpc_reg_operand" "d")] UNSPEC_FCTIW))] "TARGET_SF_FPR && TARGET_FPRND" "fctiw %0,%1" [(set_attr "type" "fp")]) Peter
diff --git a/gcc/config/rs6000/rs6000-builtin.def b/gcc/config/rs6000/rs6000-builtin.def index 9942d65..6e6dab0 100644 --- a/gcc/config/rs6000/rs6000-builtin.def +++ b/gcc/config/rs6000/rs6000-builtin.def @@ -658,6 +658,14 @@ /* Miscellaneous builtins for instructions added in ISA 2.07. These instructions do require the ISA 2.07 vector support, but they aren't vector instructions. */ +#define BU_P8V_MISC_1(ENUM, NAME, ATTR, ICODE) \ + RS6000_BUILTIN_1 (MISC_BUILTIN_ ## ENUM, /* ENUM */ \ + "__builtin_" NAME, /* NAME */ \ + RS6000_BTM_P8_VECTOR, /* MASK */ \ + (RS6000_BTC_ ## ATTR /* ATTR */ \ + | RS6000_BTC_UNARY), \ + CODE_FOR_ ## ICODE) /* ICODE */ + #define BU_P8V_MISC_3(ENUM, NAME, ATTR, ICODE) \ RS6000_BUILTIN_3 (MISC_BUILTIN_ ## ENUM, /* ENUM */ \ "__builtin_" NAME, /* NAME */ \ @@ -1881,8 +1889,8 @@ BU_VSX_OVERLOAD_X (XST, "xst") BU_VSX_OVERLOAD_X (XST_BE, "xst_be") /* 1 argument builtins pre ISA 2.04. */ -BU_FP_MISC_1 (FCTID, "fctid", CONST, lrintdfdi2) -BU_FP_MISC_1 (FCTIW, "fctiw", CONST, lrintsfsi2) +BU_P8V_MISC_1 (FCTID, "fctid", CONST, lrintdfdi2) +BU_P8V_MISC_1 (FCTIW, "fctiw", CONST, lrintsfsi2) /* 2 argument CMPB instructions added in ISA 2.05. */ BU_P6_2 (CMPB_32, "cmpb_32", CONST, cmpbsi3)
GCC Maintainers: The following patch fixes an ICE when compiling the test case gcc -mcpu=power7 builtin-fctid-fctiw-runnable.c The GCC compiler now gives a message "error: builtin function ‘__builtin_fctiw’ requires the ‘-mpower8-vector’ option" and exits without generating an internal error. The patch has been tested by compiling by hand as given above. The regression testing has also been done on powerpc64-unknown-linux-gnu (Power 8 BE) powerpc64le-unknown-linux-gnu (Power 8 LE) powerpc64le-unknown-linux-gnu (Power 9 LE) with no regressions. Let me know if the patch looks OK or not. Thanks. Carl Love ------------------------------------------------------------- gcc/ChangeLog: 2018-03-13 Carl Love <cel@us.ibm.com> PR 84422 - ICE on various builtin test functions when compiled with -mcpu=power7. * config/rs6000/rs6000-builtin.def: Change macro expansion for FCTID, FCTIW to BU_P8V_MISC_1. --- gcc/config/rs6000/rs6000-builtin.def | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)