diff mbox

, Fix PR/target 80099 (internal error with -mno-upper-regs-sf)

Message ID 20170412224530.GA29642@ibm-tiger.the-meissners.org
State New
Headers show

Commit Message

Michael Meissner April 12, 2017, 10:45 p.m. UTC 
The problem is rs6000_expand_vector_extract did not check for SFmode being
allowed in the Altivec (upper) registers, but the insn implementing the
variable extract had it as a condition.

In looking at the variable extract code, it currently does not require SFmode
to go in the Altivec registers, but it does require DImode to go into the
Altivec registers (vec_extract of V2DFmode will require DFmode to go in Altivec
registers instead of DImode).

I have tested this patch on a little endian power8 system and there were no
regressions with either bootstrap or make check.

[gcc]
2017-04-12  Michael Meissner  <meissner@linux.vnet.ibm.com>

	PR target/80099
	* config/rs6000/rs6000.c (rs6000_expand_vector_extract): Make sure
	that DFmode or DImode as appropriate can go in Altivec registers
	before generating the faster sequences for variable vec_extracts.
	* config/rs6000/vsx.md (vsx_extract_v4sf): Remove unneeded
	TARGET_UPPER_REGS_SF condition.

[gcc/testsuite]
2017-04-12  Michael Meissner  <meissner@linux.vnet.ibm.com>

	PR target/80099
	* gcc.target/powerpc/pr80099.c: New test.

Comments

Segher Boessenkool April 14, 2017, 8:48 a.m. UTC | #1 
On Wed, Apr 12, 2017 at 06:45:31PM -0400, Michael Meissner wrote:
> The problem is rs6000_expand_vector_extract did not check for SFmode being
> allowed in the Altivec (upper) registers, but the insn implementing the
> variable extract had it as a condition.
> 
> In looking at the variable extract code, it currently does not require SFmode
> to go in the Altivec registers, but it does require DImode to go into the
> Altivec registers (vec_extract of V2DFmode will require DFmode to go in Altivec
> registers instead of DImode).


> @@ -7586,15 +7586,23 @@ rs6000_expand_vector_extract (rtx target
>        switch (mode)
>  	{
>  	case V2DFmode:
> -	  emit_insn (gen_vsx_extract_v2df_var (target, vec, elt));
> -	  return;
> +	  if (TARGET_UPPER_REGS_DF)
> +	    {
> +	      emit_insn (gen_vsx_extract_v2df_var (target, vec, elt));
> +	      return;
> +	    }
> +	  break;

If the option is not set, we then ICE later on as far as I see (since
elt is not a CONST_INT).  Is that what we want, can that not happen?
In that case, please just do an assert (TARGET_UPPER_REGS_DF) here?


Segher
Michael Meissner April 14, 2017, 9:43 p.m. UTC | #2 
On Fri, Apr 14, 2017 at 03:48:47AM -0500, Segher Boessenkool wrote:
> On Wed, Apr 12, 2017 at 06:45:31PM -0400, Michael Meissner wrote:
> > The problem is rs6000_expand_vector_extract did not check for SFmode being
> > allowed in the Altivec (upper) registers, but the insn implementing the
> > variable extract had it as a condition.
> > 
> > In looking at the variable extract code, it currently does not require SFmode
> > to go in the Altivec registers, but it does require DImode to go into the
> > Altivec registers (vec_extract of V2DFmode will require DFmode to go in Altivec
> > registers instead of DImode).
> 
> 
> > @@ -7586,15 +7586,23 @@ rs6000_expand_vector_extract (rtx target
> >        switch (mode)
> >  	{
> >  	case V2DFmode:
> > -	  emit_insn (gen_vsx_extract_v2df_var (target, vec, elt));
> > -	  return;
> > +	  if (TARGET_UPPER_REGS_DF)
> > +	    {
> > +	      emit_insn (gen_vsx_extract_v2df_var (target, vec, elt));
> > +	      return;
> > +	    }
> > +	  break;
> 
> If the option is not set, we then ICE later on as far as I see (since
> elt is not a CONST_INT).  Is that what we want, can that not happen?
> In that case, please just do an assert (TARGET_UPPER_REGS_DF) here?

No.  I guess I was unclear.  We DO NOT NEED the test for SFmode in Altivec
registers here, but we do need DImode in Altivec registers.  The problem was
the generator did not not have the test, but this place did.

Before the split, the variable insn looks like:

   (parallel [(set (reg:SF t)			;; constraint ww
		   (unspec:SF [(reg:V4SF u)	;; constraint v
			       (reg:DI v)]	;; constraint r
		   UNSPEC_VSX_EXTRACT))
	      (clobber (reg:DI w))		;; constraint r
	      (clobber (reg:V2DI x))])		;; constraint v

The split generates:

    <various insns to prepared the shift amount for VSLO into 2nd clobber>

    ;; VSLO
    (set (reg:DI x)				;; this is the 2nd clobber
	 (unspec:DI [(reg:V2DI u)		;; this is the vector input
		     (reg:V2DI x)]		;; this is the 2nd clobber
		    UNSPEC_VSX_VSLO))

    ;; XSCVSPDPN
    (set (reg:SF t)				;; this is the destination
	 (unspec:SF [(reg:V2DI x)]		;; this is the 2nd clobber
		    UNSPEC_VSX_CVSPDP))

Because the target constraint is ww, that will be VSX_REGS normally on power8,
and FLOAT_REGS if -mupper-regs-sf is used.  Note, the variable extraction will
not be done this way on power7, since it does not have direct move.

So, since the only place we care about whether or not SFmode can go in Altivec
registers is the desination, the test should be eliminated.  However, in
looking at the code generated, I did discover that we need DImode to go in the
Altivec registers and we weren't checking for that.

Note, variable extraction of DFmode values does not need DImode in Altivec
registers, but it does need DFmode in Altivec registers.

Now that it is on by default, we really should eliminate the -mno-upper-regs-*
options.  They were useful when the code was being debugged, but they are less
useful now.  However, that is not something we should do in stage4.
Segher Boessenkool April 14, 2017, 10:07 p.m. UTC | #3 
On Fri, Apr 14, 2017 at 05:43:28PM -0400, Michael Meissner wrote:
> On Fri, Apr 14, 2017 at 03:48:47AM -0500, Segher Boessenkool wrote:
> > On Wed, Apr 12, 2017 at 06:45:31PM -0400, Michael Meissner wrote:
> > > The problem is rs6000_expand_vector_extract did not check for SFmode being
> > > allowed in the Altivec (upper) registers, but the insn implementing the
> > > variable extract had it as a condition.
> > > 
> > > In looking at the variable extract code, it currently does not require SFmode
> > > to go in the Altivec registers, but it does require DImode to go into the
> > > Altivec registers (vec_extract of V2DFmode will require DFmode to go in Altivec
> > > registers instead of DImode).
> > 
> > 
> > > @@ -7586,15 +7586,23 @@ rs6000_expand_vector_extract (rtx target
> > >        switch (mode)
> > >  	{
> > >  	case V2DFmode:
> > > -	  emit_insn (gen_vsx_extract_v2df_var (target, vec, elt));
> > > -	  return;
> > > +	  if (TARGET_UPPER_REGS_DF)
> > > +	    {
> > > +	      emit_insn (gen_vsx_extract_v2df_var (target, vec, elt));
> > > +	      return;
> > > +	    }
> > > +	  break;
> > 
> > If the option is not set, we then ICE later on as far as I see (since
> > elt is not a CONST_INT).  Is that what we want, can that not happen?
> > In that case, please just do an assert (TARGET_UPPER_REGS_DF) here?
> 
> No.  I guess I was unclear.

I'm asking about this exact code that I quoted.  After the change here,
if not TARGET_UPPER_REGS_DF, it breaks out of the switch, and then
immediately ICEs (failing assert on CONST_INT_P).  Right?  Or do I read
this wrong.


Segher
diff mbox

Patch

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 246852)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -7586,15 +7586,23 @@  rs6000_expand_vector_extract (rtx target
       switch (mode)
 	{
 	case V2DFmode:
-	  emit_insn (gen_vsx_extract_v2df_var (target, vec, elt));
-	  return;
+	  if (TARGET_UPPER_REGS_DF)
+	    {
+	      emit_insn (gen_vsx_extract_v2df_var (target, vec, elt));
+	      return;
+	    }
+	  break;
 
 	case V2DImode:
-	  emit_insn (gen_vsx_extract_v2di_var (target, vec, elt));
-	  return;
+	  if (TARGET_UPPER_REGS_DI)
+	    {
+	      emit_insn (gen_vsx_extract_v2di_var (target, vec, elt));
+	      return;
+	    }
+	  break;
 
 	case V4SFmode:
-	  if (TARGET_UPPER_REGS_SF)
+	  if (TARGET_UPPER_REGS_DI)
 	    {
 	      emit_insn (gen_vsx_extract_v4sf_var (target, vec, elt));
 	      return;
@@ -7602,16 +7610,28 @@  rs6000_expand_vector_extract (rtx target
 	  break;
 
 	case V4SImode:
-	  emit_insn (gen_vsx_extract_v4si_var (target, vec, elt));
-	  return;
+	  if (TARGET_UPPER_REGS_DI)
+	    {
+	      emit_insn (gen_vsx_extract_v4si_var (target, vec, elt));
+	      return;
+	    }
+	  break;
 
 	case V8HImode:
-	  emit_insn (gen_vsx_extract_v8hi_var (target, vec, elt));
-	  return;
+	  if (TARGET_UPPER_REGS_DI)
+	    {
+	      emit_insn (gen_vsx_extract_v8hi_var (target, vec, elt));
+	      return;
+	    }
+	  break;
 
 	case V16QImode:
-	  emit_insn (gen_vsx_extract_v16qi_var (target, vec, elt));
-	  return;
+	  if (TARGET_UPPER_REGS_DI)
+	    {
+	      emit_insn (gen_vsx_extract_v16qi_var (target, vec, elt));
+	      return;
+	    }
+	  break;
 
 	default:
 	  gcc_unreachable ();
Index: gcc/config/rs6000/vsx.md
===================================================================
--- gcc/config/rs6000/vsx.md	(revision 246852)
+++ gcc/config/rs6000/vsx.md	(working copy)
@@ -2419,8 +2419,7 @@  (define_insn_and_split "vsx_extract_v4sf
 		   UNSPEC_VSX_EXTRACT))
    (clobber (match_scratch:DI 3 "=r,&b,&b"))
    (clobber (match_scratch:V2DI 4 "=&v,X,X"))]
-  "VECTOR_MEM_VSX_P (V4SFmode) && TARGET_DIRECT_MOVE_64BIT
-   && TARGET_UPPER_REGS_SF"
+  "VECTOR_MEM_VSX_P (V4SFmode) && TARGET_DIRECT_MOVE_64BIT"
   "#"
   "&& reload_completed"
   [(const_int 0)]
Index: gcc/testsuite/gcc.target/powerpc/pr80099.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr80099.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr80099.c	(working copy)
@@ -0,0 +1,12 @@ 
+/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */
+/* { dg-require-effective-target powerpc_p8vector_ok } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power8" } } */
+/* { dg-options "-mcpu=power8 -O2 -mno-upper-regs-di" } */
+
+/* PR target/80099: compiler internal error if -mno-upper-regs-di used.  */
+
+int a;
+int int_from_mem (vector float *c)
+{
+  return __builtin_vec_extract (*c, a);
+}