diff mbox

[PowerPC] altivec_expand_vec_perm_const reverses pack pattern arguments in little endian mode

Message ID 1374541766.3633.104.camel@gnopaine
State New
Headers show

Commit Message

Bill Schmidt July 23, 2013, 1:09 a.m. UTC 
On Mon, 2013-07-22 at 19:49 -0400, David Edelsohn wrote:
> On Mon, Jul 22, 2013 at 7:05 PM, Bill Schmidt
> <wschmidt@linux.vnet.ibm.com> wrote:
> > This patch is another fix for vector handling in little endian mode.
> > The first two operands for a pack pattern are two vector registers that
> > form a contiguous array of inputs.  In LE mode the order of the operands
> > must be reversed so that the array remains contiguous in the reverse
> > order.
> >
> > This fixes a failure in the testsuite when run little-endian
> > (gcc.dg/vect/no-scevccp-outer-18.c).  Bootstrapped and tested big-endian
> > on powerpc64-unknown-linux-gnu with no new regressions.  Ok for trunk?
> >
> > Patch by Anton Blanchard.
> >
> > Thanks,
> > Bill
> >
> >
> > 2013-07-22  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
> >             Anton Blanchard <anton@au1.ibm.com>
> >
> >         * config/rs6000/rs6000.c (altivec_expand_vec_perm_const): Reverse
> >         two operands for little-endian.
> 
> Wouldn't it be better to handle this where the code is performing a
> swap a few lines above?

OK, currently testing the following.  OK if it passes?


Thanks,
Bill

> 
> Thanks, David
>

Comments

David Edelsohn July 23, 2013, 6:02 p.m. UTC | #1 
On Mon, Jul 22, 2013 at 9:09 PM, Bill Schmidt
<wschmidt@linux.vnet.ibm.com> wrote:

> OK, currently testing the following.  OK if it passes?
>
> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c  (revision 201149)
> +++ gcc/config/rs6000/rs6000.c  (working copy)
> @@ -28518,6 +28518,11 @@ altivec_expand_vec_perm_const (rtx operands[4])
>           enum machine_mode omode = insn_data[icode].operand[0].mode;
>           enum machine_mode imode = insn_data[icode].operand[1].mode;
>
> +         /* For little-endian, the two input operands must be swapped
> +            (or swapped back) to ensure proper right-to-left numbering
> +            from 0 to 2N-1.  */
> +          if (!BYTES_BIG_ENDIAN)
> +            swapped = !swapped;
>           if (swapped)
>             x = op0, op0 = op1, op1 = x;
>           if (imode != V16QImode)

I would prefer something like

if (swapped ^ ! BYTES_BIG_ENDIAN) ...

to make it more clear that the test only is affecting that specific
swapping of arguments and not the other uses of "swapped" within the
loop, i.e., not the earlier pattern comparison.

Thanks, David
Bill Schmidt July 23, 2013, 10:39 p.m. UTC | #2 
On Tue, 2013-07-23 at 14:02 -0400, David Edelsohn wrote:
> On Mon, Jul 22, 2013 at 9:09 PM, Bill Schmidt
> <wschmidt@linux.vnet.ibm.com> wrote:
> 
> > OK, currently testing the following.  OK if it passes?
> >
> > Index: gcc/config/rs6000/rs6000.c
> > ===================================================================
> > --- gcc/config/rs6000/rs6000.c  (revision 201149)
> > +++ gcc/config/rs6000/rs6000.c  (working copy)
> > @@ -28518,6 +28518,11 @@ altivec_expand_vec_perm_const (rtx operands[4])
> >           enum machine_mode omode = insn_data[icode].operand[0].mode;
> >           enum machine_mode imode = insn_data[icode].operand[1].mode;
> >
> > +         /* For little-endian, the two input operands must be swapped
> > +            (or swapped back) to ensure proper right-to-left numbering
> > +            from 0 to 2N-1.  */
> > +          if (!BYTES_BIG_ENDIAN)
> > +            swapped = !swapped;
> >           if (swapped)
> >             x = op0, op0 = op1, op1 = x;
> >           if (imode != V16QImode)
> 
> I would prefer something like
> 
> if (swapped ^ ! BYTES_BIG_ENDIAN) ...
> 
> to make it more clear that the test only is affecting that specific
> swapping of arguments and not the other uses of "swapped" within the
> loop, i.e., not the earlier pattern comparison.

OK, makes sense.  I tested with that variant and everything checks out.
I went ahead and committed the fix in the form you suggested.

Thanks,
Bill

> 
> Thanks, David
>
diff mbox

Patch

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 201149)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -28518,6 +28518,11 @@  altivec_expand_vec_perm_const (rtx operands[4])
 	  enum machine_mode omode = insn_data[icode].operand[0].mode;
 	  enum machine_mode imode = insn_data[icode].operand[1].mode;
 
+	  /* For little-endian, the two input operands must be swapped
+	     (or swapped back) to ensure proper right-to-left numbering
+	     from 0 to 2N-1.  */
+          if (!BYTES_BIG_ENDIAN)
+            swapped = !swapped;
 	  if (swapped)
 	    x = op0, op0 = op1, op1 = x;
 	  if (imode != V16QImode)