[PATCH ] PR 844422 Fix FCTID, FCTIW with -mcpu=power7

[PATCH ] PR 844422 Fix FCTID, FCTIW with -mcpu=power7

[Carl Love]

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(-)

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")
 \f
 /* 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)
-- 
2.7.4

Re: [PATCH ] PR 844422 Fix FCTID, FCTIW with -mcpu=power7

[Segher Boessenkool]

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

Re: [PATCH ] PR 844422 Fix FCTID, FCTIW with -mcpu=power7

[Bill Schmidt]

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
> 

Re: [PATCH ] PR 844422 Fix FCTID, FCTIW with -mcpu=power7

[Bill Schmidt]

> 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

Re: [PATCH ] PR 844422 Fix FCTID, FCTIW with -mcpu=power7

[Segher Boessenkool]

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

Re: [PATCH ] PR 844422 Fix FCTID, FCTIW with -mcpu=power7

[Peter Bergner]

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