Re: [PATCH], Fix PR/target 80099 (internal error with -mno-upper-regs-sf)

[Segher Boessenkool]

On Sat, Apr 15, 2017 at 01:43:18AM -0400, Michael Meissner wrote:
> You are right, and my patch was too complicated.  All I needed to do was 
> remove
> the upper register checks.  In looking at it, since the insn before being 
> split
> has both register and memory versions, if the register allocator can't 
> allocate
> a register, it will push the value on to the stack, and adjust the address 
> with
> the variable index and do a load.  Performance with the store and load, likely
> will not be ideal, but it should work.
> 
> Because of the interactions with the debug switches -mno-upper-regs-, I
> decided to add tests for all of the variable extract built-ins with each of 
> the
> no-upper regs switches.
> 
> I've tested this on a little endian power8 system and it bootstrapped and ran
> make check with no regressions.  Is it ok for the trunk?

Much simpler indeed :-)  Looks fine to me, please commit.  Thanks,


Segher


> 2017-04-15  Michael Meissner  
> 
>   PR target/80099
>   * config/rs6000/rs6000.c (rs6000_expand_vector_extract): Eliminate
>   unneeded test for TARGET_UPPER_REGS_SF.
>   * config/rs6000/vsx.md (vsx_extract_v4sf_var): Likewise.
> 
> [gcc/testsuite]
> 2017-04-15  Michael Meissner  
> 
>   PR target/80099
>   * gcc.target/powerpc/pr80099-1.c: New test.
>   * gcc.target/powerpc/pr80099-2.c: Likewise.
>   * gcc.target/powerpc/pr80099-3.c: Likewise.
>   * gcc.target/powerpc/pr80099-4.c: Likewise.
>   * gcc.target/powerpc/pr80099-5.c: Likewise.

Re: [PATCH], Fix PR/target 80099 (internal error with -mno-upper-regs-sf)

[Michael Meissner]

On Fri, Apr 14, 2017 at 05:07:17PM -0500, Segher Boessenkool wrote:
> 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.

You are right, and my patch was too complicated.  All I needed to do was remove
the upper register checks.  In looking at it, since the insn before being split
has both register and memory versions, if the register allocator can't allocate
a register, it will push the value on to the stack, and adjust the address with
the variable index and do a load.  Performance with the store and load, likely
will not be ideal, but it should work.

Because of the interactions with the debug switches -mno-upper-regs-, I
decided to add tests for all of the variable extract built-ins with each of the
no-upper regs switches.

I've tested this on a little endian power8 system and it bootstrapped and ran
make check with no regressions.  Is it ok for the trunk?

[gcc]
2017-04-15  Michael Meissner  

PR target/80099
* config/rs6000/rs6000.c (rs6000_expand_vector_extract): Eliminate
unneeded test for TARGET_UPPER_REGS_SF.
* config/rs6000/vsx.md (vsx_extract_v4sf_var): Likewise.

[gcc/testsuite]
2017-04-15  Michael Meissner  

PR target/80099
* gcc.target/powerpc/pr80099-1.c: New test.
* gcc.target/powerpc/pr80099-2.c: Likewise.
* gcc.target/powerpc/pr80099-3.c: Likewise.
* gcc.target/powerpc/pr80099-4.c: Likewise.
* gcc.target/powerpc/pr80099-5.c: Likewise.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.vnet.ibm.com, phone: +1 (978) 899-4797
Index: gcc/config/rs6000/rs6000.c
===
--- gcc/config/rs6000/rs6000.c  (revision 246815)
+++ gcc/config/rs6000/rs6000.c  (working copy)
@@ -7562,12 +7562,8 @@ rs6000_expand_vector_extract (rtx target
  return;
 
case V4SFmode:
- if (TARGET_UPPER_REGS_SF)
-   {
- emit_insn (gen_vsx_extract_v4sf_var (target, vec, elt));
- return;
-   }
- break;
+ emit_insn (gen_vsx_extract_v4sf_var (target, vec, elt));
+ return;
 
case V4SImode:
  emit_insn (gen_vsx_extract_v4si_var (target, vec, elt));
Index: gcc/config/rs6000/vsx.md
===
--- gcc/config/rs6000/vsx.md(revision 246815)
+++ 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,,"))
(clobber (match_scratch:V2DI 4 "=,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-1.c
===
--- gcc/testsuite/gcc.target/powerpc/pr80099-1.c(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr80099-1.c(revision 0)
@@ -0,0 +1,12 @@
+/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */
+/* { dg-require-effective-target 

Re: [PATCH], Fix PR/target 80099 (internal error with -mno-upper-regs-sf)

[Segher Boessenkool]

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

Re: [PATCH], Fix PR/target 80099 (internal error with -mno-upper-regs-sf)

[Michael Meissner]

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:



;; 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.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.vnet.ibm.com, phone: +1 (978) 899-4797

Re: [PATCH], Fix PR/target 80099 (internal error with -mno-upper-regs-sf)

[Segher Boessenkool]

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

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

[Michael Meissner]

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  

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  

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

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.vnet.ibm.com, phone: +1 (978) 899-4797
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,,"))
(clobber (match_scratch:V2DI 4 "=,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);
+}