[rs6000] Require Power 8 for vec_float2 builtin

Message ID 1518800329.7508.37.camel@us.ibm.com
State New
Headers show
Series
  • [rs6000] Require Power 8 for vec_float2 builtin
Related show

Commit Message

Carl Love Feb. 16, 2018, 4:58 p.m.
GCC maintainers:

The following patch fixes a GCC internal compiler error.

Several of the builtins use the wrong enable conditions as mentioned in
Bill's issue data base (https://github.ibm.com/wschmidt/power-gcc/issue
s/282).  The implementation for the vec_float2() builtin, as
implemented, uses the pre Power 8 macro expansions BU_VSX_2 and
BU_VSX_OVERLOAD_2.  The result is GCC generates an internal compiler
error when the test file is compiled with -mcpu=power7.  The builtin
support uses RTL code that is only supported on Power 8 and beyond.  

Additionally, the 64-bit ABI document that lists the vec_float2()
builtin says the listed builtins are supported on Power 8 or newer
platforms.  

This patch changes the expansion macros to the equivalent Power 8
macros.  The test now cleanly exits with the message 

    error: builtin function ‘__builtin_vsx_float2_v2di’ requires the ‘-
    mpower8-vector’ option

rather then giving an internal compiler error when compiled with the
cpu=power7 option.

The patch was tested by running the full regression suite to ensure no
new regressions were introduced.  Additionally, the patch was tested by
hand compiling with the -mcpu=power7 option to verify the issue is
fixed.  The testing was don on  powerpc64le-unknown-linux-gnu (Power 8
LE)

Please let me know if the patch looks OK or not. Thanks.

                       Carl Love

---------------------------------------------------------------------------------

gcc/ChangeLog:

2018-02-16  Carl Love  <cel@us.ibm.com>

	* config/rs6000/rs6000-builtin.def: Add BU_P8V_VSX_2 macro definition.
	Change FLOAT2 expansions from BU_VSX_2 to BU_P8V_VSX_2 and
	from BU_VSX_OVERLOAD_2 to BU_P8V_OVERLOAD_2.
	* config/rs6000/rs6000-c.c: Changed macro VSX_BUILTIN_VEC_FLOAT2
	expansion to P8V_BUILTIN_VEC_FLOAT2.
---
 gcc/config/rs6000/rs6000-builtin.def | 20 +++++++++++++++-----
 gcc/config/rs6000/rs6000-c.c         |  6 +++---
 2 files changed, 18 insertions(+), 8 deletions(-)

-- 
2.7.4

Comments

Segher Boessenkool Feb. 16, 2018, 5:56 p.m. | #1
Hi!

On Fri, Feb 16, 2018 at 08:58:49AM -0800, Carl Love wrote:
> This patch changes the expansion macros to the equivalent Power 8

> macros.  The test now cleanly exits with the message 

> 

>     error: builtin function ‘__builtin_vsx_float2_v2di’ requires the ‘-

>     mpower8-vector’ option

> 

> rather then giving an internal compiler error when compiled with the

> cpu=power7 option.


:-)

> The patch was tested by running the full regression suite to ensure no

> new regressions were introduced.  Additionally, the patch was tested by

> hand compiling with the -mcpu=power7 option to verify the issue is

> fixed.  The testing was don on  powerpc64le-unknown-linux-gnu (Power 8

> LE)


Do you have a new testcase, too?  Or is this all covered by existing
tests (which?)

Looks great, thanks!  Okay for trunk (and for backports after a while,
if needed).


Segher


> 	* config/rs6000/rs6000-builtin.def: Add BU_P8V_VSX_2 macro definition.

> 	Change FLOAT2 expansions from BU_VSX_2 to BU_P8V_VSX_2 and

> 	from BU_VSX_OVERLOAD_2 to BU_P8V_OVERLOAD_2.

> 	* config/rs6000/rs6000-c.c: Changed macro VSX_BUILTIN_VEC_FLOAT2

> 	expansion to P8V_BUILTIN_VEC_FLOAT2.
Carl Love Feb. 16, 2018, 6:21 p.m. | #2
On Fri, 2018-02-16 at 11:56 -0600, Segher Boessenkool wrote:
> Hi!

Segher:

Per our discussion on IRC.  I created the bugzilla:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84422

for this issue so it is visible to people outside IBM.  
I have included this test file int the above bugzilla along with the
other four builtin test case files that ICE when compiled with the
-mcpu=power7 option.
  
> On Fri, Feb 16, 2018 at 08:58:49AM -0800, Carl Love wrote:

> > This patch changes the expansion macros to the equivalent Power 8

> > macros.  The test now cleanly exits with the message 

> > 

> >     error: builtin function ‘__builtin_vsx_float2_v2di’ requires

> > the ‘-

> >     mpower8-vector’ option

> > 

> > rather then giving an internal compiler error when compiled with

> > the

> > cpu=power7 option.

> 

> :-)

> 

> > The patch was tested by running the full regression suite to ensure

> > no

> > new regressions were introduced.  Additionally, the patch was

> > tested by

> > hand compiling with the -mcpu=power7 option to verify the issue is

> > fixed.  The testing was don on  powerpc64le-unknown-linux-gnu

> > (Power 8

> > LE)

> 

> Do you have a new testcase, too?  Or is this all covered by existing

> tests (which?)


The issue was found when compiling the existing test case file with
-mcpu=power7.  No additional tests are needed for this issue.

> 

> Looks great, thanks!  Okay for trunk (and for backports after a

> while,

> if needed).


                     Carl Love
Carl Love Feb. 16, 2018, 6:33 p.m. | #3
On Fri, 2018-02-16 at 10:21 -0800, Carl Love wrote:
> 

> > 

> > Do you have a new testcase, too?  Or is this all covered by

> > existing

> > tests (which?)

> 

> The issue was found when compiling the existing test case file with

> -mcpu=power7.  No additional tests are needed for this issue.

> 


Sorry, my bad.  The exiting test case file is:
 
gcc/testsuite/gcc.target/powerpc/builtins-3-runnable.c

> > 

> > Looks great, thanks!  Okay for trunk (and for backports after a

> > while,

> > if needed).

> 

>                      Carl Love

>

Patch

diff --git a/gcc/config/rs6000/rs6000-builtin.def b/gcc/config/rs6000/rs6000-builtin.def
index 86604da..05eb356 100644
--- a/gcc/config/rs6000/rs6000-builtin.def
+++ b/gcc/config/rs6000/rs6000-builtin.def
@@ -403,6 +403,14 @@ 
 		     | RS6000_BTC_UNARY),				\
 		    CODE_FOR_ ## ICODE)			/* ICODE */
 
+#define BU_P8V_VSX_2(ENUM, NAME, ATTR, ICODE)				\
+  RS6000_BUILTIN_2 (P8V_BUILTIN_ ## ENUM,		/* ENUM */	\
+		    "__builtin_vsx_" NAME,		/* NAME */	\
+		    RS6000_BTM_P8_VECTOR,		/* MASK */	\
+		    (RS6000_BTC_ ## ATTR		/* ATTR */	\
+		     | RS6000_BTC_BINARY),				\
+		    CODE_FOR_ ## ICODE)			/* ICODE */
+
 #define BU_P8V_OVERLOAD_1(ENUM, NAME)					\
   RS6000_BUILTIN_1 (P8V_BUILTIN_VEC_ ## ENUM,		/* ENUM */	\
 		    "__builtin_vec_" NAME,		/* NAME */	\
@@ -1659,9 +1667,6 @@  BU_VSX_2 (CMPLE_U16QI,        "cmple_u16qi",    CONST,  vector_ngtuv16qi)
 BU_VSX_2 (CMPLE_U8HI,         "cmple_u8hi",     CONST,  vector_ngtuv8hi)
 BU_VSX_2 (CMPLE_U4SI,         "cmple_u4si",     CONST,  vector_ngtuv4si)
 BU_VSX_2 (CMPLE_U2DI,         "cmple_u2di",     CONST,  vector_ngtuv2di)
-BU_VSX_2 (FLOAT2_V2DF,        "float2_v2df",    CONST,  float2_v2df)
-BU_VSX_2 (FLOAT2_V2DI,        "float2_v2di",    CONST,  float2_v2di)
-BU_VSX_2 (UNS_FLOAT2_V2DI,    "uns_float2_v2di",    CONST,  uns_float2_v2di)
 
 BU_VSX_2 (VEC_VSIGNED2_V2DF,      "vsigned2_v2df",    CONST,  vsigned2_v2df)
 BU_VSX_2 (VEC_VUNSIGNED2_V2DF,    "vunsigned2_v2df",  CONST,  vunsigned2_v2df)
@@ -1856,8 +1861,6 @@  BU_VSX_OVERLOAD_2 (XXMRGHW,  "xxmrghw")
 BU_VSX_OVERLOAD_2 (XXMRGLW,  "xxmrglw")
 BU_VSX_OVERLOAD_2 (XXSPLTD,  "xxspltd")
 BU_VSX_OVERLOAD_2 (XXSPLTW,  "xxspltw")
-BU_VSX_OVERLOAD_2 (FLOAT2,   "float2")
-BU_VSX_OVERLOAD_2 (UNS_FLOAT2,   "uns_float2")
 BU_VSX_OVERLOAD_2 (VSIGNED2,     "vsigned2")
 BU_VSX_OVERLOAD_2 (VUNSIGNED2,   "vunsigned2")
 
@@ -1910,6 +1913,11 @@  BU_P8V_VSX_1 (REVB_V16QI,     "revb_v16qi",	CONST,	revb_v16qi)
 BU_P8V_VSX_1 (REVB_V2DF,      "revb_v2df",	CONST,	revb_v2df)
 BU_P8V_VSX_1 (REVB_V4SF,      "revb_v4sf",	CONST,	revb_v4sf)
 
+/* 2 argument VSX instructions added in ISA 2.07.  */
+BU_P8V_VSX_2 (FLOAT2_V2DF,        "float2_v2df",	CONST,  float2_v2df)
+BU_P8V_VSX_2 (FLOAT2_V2DI,        "float2_v2di",	CONST,  float2_v2di)
+BU_P8V_VSX_2 (UNS_FLOAT2_V2DI,    "uns_float2_v2di",    CONST,  uns_float2_v2di)
+
 /* 1 argument altivec instructions added in ISA 2.07.  */
 BU_P8V_AV_1 (ABS_V2DI,	      "abs_v2di",	CONST,	absv2di2)
 BU_P8V_AV_1 (VUPKHSW,	      "vupkhsw",	CONST,	altivec_vupkhsw)
@@ -2052,6 +2060,8 @@  BU_P8V_OVERLOAD_2 (VSRD,	"vsrd")
 BU_P8V_OVERLOAD_2 (VSUBCUQ,	"vsubcuq")
 BU_P8V_OVERLOAD_2 (VSUBUDM,	"vsubudm")
 BU_P8V_OVERLOAD_2 (VSUBUQM,	"vsubuqm")
+BU_P8V_OVERLOAD_2 (FLOAT2,   "float2")
+BU_P8V_OVERLOAD_2 (UNS_FLOAT2,   "uns_float2")
 
 /* ISA 2.07 vector overloaded 3 argument functions.  */
 BU_P8V_OVERLOAD_3 (VADDECUQ,	"vaddecuq")
diff --git a/gcc/config/rs6000/rs6000-c.c b/gcc/config/rs6000/rs6000-c.c
index a68be51..236003f 100644
--- a/gcc/config/rs6000/rs6000-c.c
+++ b/gcc/config/rs6000/rs6000-c.c
@@ -1542,11 +1542,11 @@  const struct altivec_builtin_types altivec_overloaded_builtins[] = {
     RS6000_BTI_V4SF, RS6000_BTI_V4SI, 0, 0 },
   { VSX_BUILTIN_VEC_FLOAT, VSX_BUILTIN_XVCVUXWSP_V4SF,
     RS6000_BTI_V4SF, RS6000_BTI_unsigned_V4SI, 0, 0 },
-  { VSX_BUILTIN_VEC_FLOAT2, VSX_BUILTIN_FLOAT2_V2DF,
+  { P8V_BUILTIN_VEC_FLOAT2, P8V_BUILTIN_FLOAT2_V2DF,
     RS6000_BTI_V4SF, RS6000_BTI_V2DF, RS6000_BTI_V2DF, 0 },
-  { VSX_BUILTIN_VEC_FLOAT2, VSX_BUILTIN_FLOAT2_V2DI,
+  { P8V_BUILTIN_VEC_FLOAT2, P8V_BUILTIN_FLOAT2_V2DI,
     RS6000_BTI_V4SF, RS6000_BTI_V2DI, RS6000_BTI_V2DI, 0 },
-  { VSX_BUILTIN_VEC_FLOAT2, VSX_BUILTIN_UNS_FLOAT2_V2DI,
+  { P8V_BUILTIN_VEC_FLOAT2, P8V_BUILTIN_UNS_FLOAT2_V2DI,
     RS6000_BTI_V4SF, RS6000_BTI_unsigned_V2DI,
     RS6000_BTI_unsigned_V2DI, 0 },
   { VSX_BUILTIN_VEC_FLOATE, VSX_BUILTIN_FLOATE_V2DF,