diff mbox series

[PATCH.,rs6000] Fix PR84912: ICE using -m32 on __builtin_divde*, patch #2

Message ID 1f024cd8-59ec-ff67-cadf-6700ad119000@vnet.ibm.com
State New
Headers show
Series [PATCH.,rs6000] Fix PR84912: ICE using -m32 on __builtin_divde*, patch #2 | expand

Commit Message

Peter Bergner March 23, 2018, 5:41 p.m. UTC 
This is the second patch to fix PR84912, which is an ICE when calling some
extended divide builtin functions.  This patch is relative to the first
patch.  This fixes the ICE by adding a new mask to the builtin functions
that are ICEing and then enforcing it is set.  I have also added a helpful
error message in the case it is not set.

This passed bootstrap and regtesting on powerpc64-linux with no regressions.
Ok for mainline?

Do we also want this backported to the open release branches too?

Peter

gcc/
	PR target/84912
	* config/rs6000/rs6000.h (RS6000_BTM_POWERPC64): New define.
	(RS6000_BTM_COMMON): Add RS6000_BTM_POWERPC64.
	* config/rs6000/rs6000.c (rs6000_builtin_mask_calculate): Add support
	for RS6000_BTM_POWERPC64.
	(rs6000_invalid_builtin): Add handling for RS6000_BTM_POWERPC64
	(rs6000_builtin_mask_names): Add RS6000_BTM_POWERPC64.
	* config/rs6000/rs6000-builtin.def (BU_P7_POWERPC64_MISC_2): New macro
	definition.
	(DIVDE): Use it.
	(DIVDEU): Likewise.

Comments

Segher Boessenkool March 27, 2018, 10:02 p.m. UTC | #1 
Hi!

On Fri, Mar 23, 2018 at 12:41:38PM -0500, Peter Bergner wrote:
> This is the second patch to fix PR84912, which is an ICE when calling some
> extended divide builtin functions.  This patch is relative to the first
> patch.  This fixes the ICE by adding a new mask to the builtin functions
> that are ICEing and then enforcing it is set.  I have also added a helpful
> error message in the case it is not set.

> @@ -15952,6 +15953,10 @@ rs6000_invalid_builtin (enum rs6000_buil
>  	   name);
>    else if ((fnmask & RS6000_BTM_FLOAT128) != 0)
>      error ("builtin function %qs requires the %qs option", name, "-mfloat128");
> +  else if ((fnmask & (RS6000_BTM_POPCNTD | RS6000_BTM_POWERPC64))
> +	   == (RS6000_BTM_POPCNTD | RS6000_BTM_POWERPC64))
> +    error ("builtin function %qs requires the %qs and %qs options",
> +	   name, "-mcpu=power7 (or newer)", "-m64 or -mpowerpc64");

This does not work for translation, and it quotes the wrong things.
Each %qs should be for exactly one option string.

Looks good otherwise.


Segher
Peter Bergner March 28, 2018, 3:38 p.m. UTC | #2 
On 3/27/18 5:02 PM, Segher Boessenkool wrote:
>> @@ -15952,6 +15953,10 @@ rs6000_invalid_builtin (enum rs6000_buil
>>  	   name);
>>    else if ((fnmask & RS6000_BTM_FLOAT128) != 0)
>>      error ("builtin function %qs requires the %qs option", name, "-mfloat128");
>> +  else if ((fnmask & (RS6000_BTM_POPCNTD | RS6000_BTM_POWERPC64))
>> +	   == (RS6000_BTM_POPCNTD | RS6000_BTM_POWERPC64))
>> +    error ("builtin function %qs requires the %qs and %qs options",
>> +	   name, "-mcpu=power7 (or newer)", "-m64 or -mpowerpc64");
> 
> This does not work for translation, and it quotes the wrong things.
> Each %qs should be for exactly one option string.

I'm confused. :-)   What is it I need to do to fix this?  I just cut/pasted
usage higher up in the function, so does that need fixing too or ???

Peter
Segher Boessenkool March 28, 2018, 5:59 p.m. UTC | #3 
On Wed, Mar 28, 2018 at 10:38:49AM -0500, Peter Bergner wrote:
> On 3/27/18 5:02 PM, Segher Boessenkool wrote:
> >> @@ -15952,6 +15953,10 @@ rs6000_invalid_builtin (enum rs6000_buil
> >>  	   name);
> >>    else if ((fnmask & RS6000_BTM_FLOAT128) != 0)
> >>      error ("builtin function %qs requires the %qs option", name, "-mfloat128");
> >> +  else if ((fnmask & (RS6000_BTM_POPCNTD | RS6000_BTM_POWERPC64))
> >> +	   == (RS6000_BTM_POPCNTD | RS6000_BTM_POWERPC64))
> >> +    error ("builtin function %qs requires the %qs and %qs options",
> >> +	   name, "-mcpu=power7 (or newer)", "-m64 or -mpowerpc64");
> > 
> > This does not work for translation, and it quotes the wrong things.
> > Each %qs should be for exactly one option string.
> 
> I'm confused. :-)   What is it I need to do to fix this?  I just cut/pasted
> usage higher up in the function, so does that need fixing too or ???

It should be something like

+    error ("builtin function %qs requires the %qs (or newer), and "
	   "%qs or %qs options",
+	   name, "-mcpu=power7", "-m64", "-mpowerpc64");

I don't see other such strings that quote incorrectly?


Segher
Peter Bergner March 28, 2018, 6:57 p.m. UTC | #4 
On 3/28/18 12:59 PM, Segher Boessenkool wrote:
> It should be something like
> 
> +    error ("builtin function %qs requires the %qs (or newer), and "
> 	   "%qs or %qs options",
> +	   name, "-mcpu=power7", "-m64", "-mpowerpc64");
> 
> I don't see other such strings that quote incorrectly?

Ah, I guess I misunderstood what you were saying.  So ok for trunk
with that change then?

Peter
Segher Boessenkool March 28, 2018, 9:13 p.m. UTC | #5 
On Wed, Mar 28, 2018 at 01:57:34PM -0500, Peter Bergner wrote:
> On 3/28/18 12:59 PM, Segher Boessenkool wrote:
> > It should be something like
> > 
> > +    error ("builtin function %qs requires the %qs (or newer), and "
> > 	   "%qs or %qs options",
> > +	   name, "-mcpu=power7", "-m64", "-mpowerpc64");
> > 
> > I don't see other such strings that quote incorrectly?
> 
> Ah, I guess I misunderstood what you were saying.  So ok for trunk
> with that change then?

"Something like", I haven't tested anything.  Please do test :-)

Okay with that.  Thanks!


Segher
Peter Bergner March 29, 2018, 12:13 a.m. UTC | #6 
On 3/28/18 4:13 PM, Segher Boessenkool wrote:
> On Wed, Mar 28, 2018 at 01:57:34PM -0500, Peter Bergner wrote:
>> On 3/28/18 12:59 PM, Segher Boessenkool wrote:
>>> It should be something like
>>>
>>> +    error ("builtin function %qs requires the %qs (or newer), and "
>>> 	   "%qs or %qs options",
>>> +	   name, "-mcpu=power7", "-m64", "-mpowerpc64");
>>>
>>> I don't see other such strings that quote incorrectly?
>>
>> Ah, I guess I misunderstood what you were saying.  So ok for trunk
>> with that change then?
> 
> "Something like", I haven't tested anything.  Please do test :-)
> 
> Okay with that.  Thanks!

Tested and committed...both patches.  Thanks.

Do we care enough to fix these on the release branches?  If so, I
can backport them easily, since they're not that involved.
I'll leave it up to you to decide.

Peter
Segher Boessenkool March 29, 2018, 12:21 a.m. UTC | #7 
On Wed, Mar 28, 2018 at 07:13:36PM -0500, Peter Bergner wrote:
> On 3/28/18 4:13 PM, Segher Boessenkool wrote:
> > On Wed, Mar 28, 2018 at 01:57:34PM -0500, Peter Bergner wrote:
> >> On 3/28/18 12:59 PM, Segher Boessenkool wrote:
> >>> It should be something like
> >>>
> >>> +    error ("builtin function %qs requires the %qs (or newer), and "
> >>> 	   "%qs or %qs options",
> >>> +	   name, "-mcpu=power7", "-m64", "-mpowerpc64");
> >>>
> >>> I don't see other such strings that quote incorrectly?
> >>
> >> Ah, I guess I misunderstood what you were saying.  So ok for trunk
> >> with that change then?
> > 
> > "Something like", I haven't tested anything.  Please do test :-)
> > 
> > Okay with that.  Thanks!
> 
> Tested and committed...both patches.  Thanks.
> 
> Do we care enough to fix these on the release branches?  If so, I
> can backport them easily, since they're not that involved.
> I'll leave it up to you to decide.

If it's easy, yes please.  After the usual wait.


Segher
Peter Bergner April 3, 2018, 12:03 a.m. UTC | #8 
On 3/28/18 7:21 PM, Segher Boessenkool wrote:
> On Wed, Mar 28, 2018 at 07:13:36PM -0500, Peter Bergner wrote:
>> Do we care enough to fix these on the release branches?  If so, I
>> can backport them easily, since they're not that involved.
>> I'll leave it up to you to decide.
> 
> If it's easy, yes please.  After the usual wait.

Yes it was easy.  Committed now to GCC 7 and GCC 6.  Thanks.

Peter
diff mbox series

Patch

diff -urpN -X /home/bergner/cvs/dontdiff gcc-fsf-mainline-pr84912-2/gcc/config/rs6000/rs6000.h gcc-fsf-mainline-pr84912/gcc/config/rs6000/rs6000.h
--- gcc-fsf-mainline-pr84912-2/gcc/config/rs6000/rs6000.h	2018-03-19 19:59:55.911285043 -0500
+++ gcc-fsf-mainline-pr84912/gcc/config/rs6000/rs6000.h	2018-03-22 19:57:21.051923201 -0500
@@ -2506,6 +2506,7 @@  extern int frame_pointer_needed;
 #define RS6000_BTM_HARD_FLOAT	MASK_SOFT_FLOAT	/* Hardware floating point.  */
 #define RS6000_BTM_LDBL128	MASK_MULTIPLE	/* 128-bit long double.  */
 #define RS6000_BTM_64BIT	MASK_64BIT	/* 64-bit addressing.  */
+#define RS6000_BTM_POWERPC64	MASK_POWERPC64	/* 64-bit registers.  */
 #define RS6000_BTM_FLOAT128	MASK_FLOAT128_KEYWORD /* IEEE 128-bit float.  */
 #define RS6000_BTM_FLOAT128_HW	MASK_FLOAT128_HW /* IEEE 128-bit float h/w.  */
 
@@ -2526,6 +2527,7 @@  extern int frame_pointer_needed;
 				 | RS6000_BTM_DFP			\
 				 | RS6000_BTM_HARD_FLOAT		\
 				 | RS6000_BTM_LDBL128			\
+				 | RS6000_BTM_POWERPC64			\
 				 | RS6000_BTM_FLOAT128			\
 				 | RS6000_BTM_FLOAT128_HW)
 
diff -urpN -X /home/bergner/cvs/dontdiff gcc-fsf-mainline-pr84912-2/gcc/config/rs6000/rs6000.c gcc-fsf-mainline-pr84912/gcc/config/rs6000/rs6000.c
--- gcc-fsf-mainline-pr84912-2/gcc/config/rs6000/rs6000.c	2018-03-23 08:03:55.257469347 -0500
+++ gcc-fsf-mainline-pr84912/gcc/config/rs6000/rs6000.c	2018-03-23 08:03:42.707253487 -0500
@@ -3916,6 +3916,7 @@  rs6000_builtin_mask_calculate (void)
 	  | ((TARGET_P9_MISC)		    ? RS6000_BTM_P9_MISC   : 0)
 	  | ((TARGET_MODULO)		    ? RS6000_BTM_MODULO    : 0)
 	  | ((TARGET_64BIT)		    ? RS6000_BTM_64BIT     : 0)
+	  | ((TARGET_POWERPC64)		    ? RS6000_BTM_POWERPC64 : 0)
 	  | ((TARGET_CRYPTO)		    ? RS6000_BTM_CRYPTO	   : 0)
 	  | ((TARGET_HTM)		    ? RS6000_BTM_HTM	   : 0)
 	  | ((TARGET_DFP)		    ? RS6000_BTM_DFP	   : 0)
@@ -15952,6 +15953,10 @@  rs6000_invalid_builtin (enum rs6000_buil
 	   name);
   else if ((fnmask & RS6000_BTM_FLOAT128) != 0)
     error ("builtin function %qs requires the %qs option", name, "-mfloat128");
+  else if ((fnmask & (RS6000_BTM_POPCNTD | RS6000_BTM_POWERPC64))
+	   == (RS6000_BTM_POPCNTD | RS6000_BTM_POWERPC64))
+    error ("builtin function %qs requires the %qs and %qs options",
+	   name, "-mcpu=power7 (or newer)", "-m64 or -mpowerpc64");
   else
     error ("builtin function %qs is not supported with the current options",
 	   name);
@@ -36612,6 +36617,7 @@  static struct rs6000_opt_mask const rs60
   { "hard-dfp",		 RS6000_BTM_DFP,	false, false },
   { "hard-float",	 RS6000_BTM_HARD_FLOAT,	false, false },
   { "long-double-128",	 RS6000_BTM_LDBL128,	false, false },
+  { "powerpc64",	 RS6000_BTM_POWERPC64,  false, false },
   { "float128",		 RS6000_BTM_FLOAT128,   false, false },
   { "float128-hw",	 RS6000_BTM_FLOAT128_HW,false, false },
 };
diff -urpN -X /home/bergner/cvs/dontdiff gcc-fsf-mainline-pr84912-2/gcc/config/rs6000/rs6000-builtin.def gcc-fsf-mainline-pr84912/gcc/config/rs6000/rs6000-builtin.def
--- gcc-fsf-mainline-pr84912-2/gcc/config/rs6000/rs6000-builtin.def	2018-03-23 08:03:55.257469347 -0500
+++ gcc-fsf-mainline-pr84912/gcc/config/rs6000/rs6000-builtin.def	2018-03-23 08:03:42.707253487 -0500
@@ -646,6 +646,15 @@ 
 		     | RS6000_BTC_BINARY),				\
 		    CODE_FOR_ ## ICODE)			/* ICODE */
 
+#define BU_P7_POWERPC64_MISC_2(ENUM, NAME, ATTR, ICODE)			\
+  RS6000_BUILTIN_2 (MISC_BUILTIN_ ## ENUM,		/* ENUM */	\
+		    "__builtin_" NAME,			/* NAME */	\
+		    RS6000_BTM_POPCNTD					\
+		    | RS6000_BTM_POWERPC64,		/* MASK */	\
+		    (RS6000_BTC_ ## ATTR		/* ATTR */	\
+		     | RS6000_BTC_BINARY),				\
+		    CODE_FOR_ ## ICODE)			/* ICODE */
+
 #define BU_P7_MISC_X(ENUM, NAME, ATTR)					\
   RS6000_BUILTIN_X (MISC_BUILTIN_ ## ENUM,		/* ENUM */	\
 		    "__builtin_" NAME,			/* NAME */	\
@@ -2311,8 +2320,8 @@  BU_P9V_OVERLOAD_1 (VCTZLSBB,	"vctzlsbb")
 /* 2 argument extended divide functions added in ISA 2.06.  */
 BU_P7_MISC_2 (DIVWE,		"divwe",	CONST,	dive_si)
 BU_P7_MISC_2 (DIVWEU,		"divweu",	CONST,	diveu_si)
-BU_P7_MISC_2 (DIVDE,		"divde",	CONST,	dive_di)
-BU_P7_MISC_2 (DIVDEU,		"divdeu",	CONST,	diveu_di)
+BU_P7_POWERPC64_MISC_2 (DIVDE,	"divde",	CONST,	dive_di)
+BU_P7_POWERPC64_MISC_2 (DIVDEU,	"divdeu",	CONST,	diveu_di)
 
 /* 1 argument DFP (decimal floating point) functions added in ISA 2.05.  */
 BU_DFP_MISC_1 (DXEX,		"dxex",		CONST,	dfp_dxex_dd)