Patchwork Patch to align local PowerPC AltiVec arrays

login
register
mail settings
Submitter Wellander, Pat - Code Sourcery
Date March 21, 2011, 9:54 p.m.
Message ID <4D87C91C.6080709@codesourcery.com>
Download mbox | patch
Permalink /patch/87837/
State New
Headers show

Comments

Wellander, Pat - Code Sourcery - March 21, 2011, 9:54 p.m.
This is my first patch submittal so constructive criticism is appreciated.
This patch causes PowerPC AltiVec local stack arrays > 16 bytes to be 
aligned at 16 bytes.

2011-01-27  Pat Wellander <pwelland@codesourcery.com>

   * config/rs6000/rs6000.h (LOCAL_ALIGNMENT):
     AltiVec local arrays >= 16 bytes are now aligned to at least 16 bytes.
Andrew Pinski - March 21, 2011, 10:01 p.m.
On Mon, Mar 21, 2011 at 2:54 PM, Wellander, Pat - Code Sourcery
<pwelland@codesourcery.com> wrote:
> This is my first patch submittal so constructive criticism is appreciated.
> This patch causes PowerPC AltiVec local stack arrays > 16 bytes to be
> aligned at 16 bytes.

Wait I thought the alignment of the stack is 16 bytes so this patch is
normally not needed except for the case where someone does not use
-mabi=altivect.
Wellander, Pat - Code Sourcery - March 21, 2011, 10:21 p.m.
The stack is aligned at 16 bytes with AltiVec (if not, this patch would 
not make sense).
Also, all char arrays are aligned at 16 bytes.
This patch causes other local stack arrays >= 16 bytes to be aligned at 
16 bytes in order to
make more effective use of AltiVec instructions.

On 3/21/2011 3:01 PM, Andrew Pinski wrote:
> On Mon, Mar 21, 2011 at 2:54 PM, Wellander, Pat - Code Sourcery
> <pwelland@codesourcery.com>  wrote:
>> This is my first patch submittal so constructive criticism is appreciated.
>> This patch causes PowerPC AltiVec local stack arrays>  16 bytes to be
>> aligned at 16 bytes.
> Wait I thought the alignment of the stack is 16 bytes so this patch is
> normally not needed except for the case where someone does not use
> -mabi=altivect.
Andrew Pinski - March 21, 2011, 10:25 p.m.
On Mon, Mar 21, 2011 at 3:21 PM, Wellander, Pat - Code Sourcery
<pwelland@codesourcery.com> wrote:
> The stack is aligned at 16 bytes with AltiVec (if not, this patch would not
> make sense).
> Also, all char arrays are aligned at 16 bytes.
> This patch causes other local stack arrays >= 16 bytes to be aligned at 16
> bytes in order to
> make more effective use of AltiVec instructions.

Then why not change it so not just auto arrays are aligned at 16
bytes, just like char arrays?

Thanks,
Andrew Pinski
Andrew Pinski - March 21, 2011, 10:26 p.m.
On Mon, Mar 21, 2011 at 3:25 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Mon, Mar 21, 2011 at 3:21 PM, Wellander, Pat - Code Sourcery
> <pwelland@codesourcery.com> wrote:
>> The stack is aligned at 16 bytes with AltiVec (if not, this patch would not
>> make sense).
>> Also, all char arrays are aligned at 16 bytes.
>> This patch causes other local stack arrays >= 16 bytes to be aligned at 16
>> bytes in order to
>> make more effective use of AltiVec instructions.
>
> Then why not change it so not just auto arrays are aligned at 16
> bytes, just like char arrays?

Also what about structs containing just arrays?  Should you have those
aligned too?

-- Pinski
Wellander, Pat - Code Sourcery - March 21, 2011, 10:39 p.m.
On 3/21/2011 3:26 PM, Andrew Pinski wrote:
> On Mon, Mar 21, 2011 at 3:25 PM, Andrew Pinski<pinskia@gmail.com>  wrote:
>> On Mon, Mar 21, 2011 at 3:21 PM, Wellander, Pat - Code Sourcery
>> <pwelland@codesourcery.com>  wrote:
>>> The stack is aligned at 16 bytes with AltiVec (if not, this patch would not
>>> make sense).
>>> Also, all char arrays are aligned at 16 bytes.
>>> This patch causes other local stack arrays>= 16 bytes to be aligned at 16
>>> bytes in order to
>>> make more effective use of AltiVec instructions.
>> Then why not change it so not just auto arrays are aligned at 16
>> bytes, just like char arrays?
> Also what about structs containing just arrays?  Should you have those
> aligned too?
>
> -- Pinski
What you are suggesting makes sense to me.  This change was made per a 
request from Freescale which was limited to local arrays.  As this is my 
first gcc change, I just followed the request.
I would think if all larger structs were aligned also, this could help 
make struct assignments more efficient.
Of course there is more wasted memory, but maybe AltiVec users don't 
care about this.

Pat
Wellander, Pat - Code Sourcery - April 17, 2011, 9:56 p.m.
Actually, after reinvestigating this, "AFAIK, the vectorizer is smart
enough to automagically increase alignment on global arrays" so is not
of concern to Freescale. Also, Changing the alignment of arrays within
structs would change the field layout and would violate the ABI and be
incompatible with other non-AltiVec hardware.

Pat

On 3/21/2011 3:26 PM, Andrew Pinski wrote:
> On Mon, Mar 21, 2011 at 3:25 PM, Andrew Pinski <pinskia@gmail.com> wrote:
>> On Mon, Mar 21, 2011 at 3:21 PM, Wellander, Pat - Code Sourcery
>> <pwelland@codesourcery.com> wrote:
>>> The stack is aligned at 16 bytes with AltiVec (if not, this patch would not
>>> make sense).
>>> Also, all char arrays are aligned at 16 bytes.
>>> This patch causes other local stack arrays >= 16 bytes to be aligned at 16
>>> bytes in order to
>>> make more effective use of AltiVec instructions.
>>
>> Then why not change it so not just auto arrays are aligned at 16
>> bytes, just like char arrays?
> 
> Also what about structs containing just arrays?  Should you have those
> aligned too?
> 
> -- Pinski

Patch

Index: config/rs6000/rs6000.h
===================================================================
--- config/rs6000/rs6000.h      (revision 171171)
+++ config/rs6000/rs6000.h      (working copy)
@@ -686,9 +686,16 @@ 

  /* A C expression to compute the alignment for a variables in the
     local store.  TYPE is the data type, and ALIGN is the alignment
-   that the object would ordinarily have.  */
-#define LOCAL_ALIGNMENT(TYPE, ALIGN)                           \
-  DATA_ALIGNMENT (TYPE, ALIGN)
+   that the object would ordinarily have.
+   Align most data using the DATA_ALIGNMENT macro but
+   align local Altivec arrays >= 16 bytes
+   to 16 bytes (128 bits).  The DATA_ALIGNMENT macro aligns
+   all char arrays (not struct fields) to 16 bytes. These
+   alignments don't appear to affect struct field arrays.  */
+#define LOCAL_ALIGNMENT(TYPE, ALIGN)                      \
+  (TARGET_ALTIVEC &&  (TREE_CODE (TYPE) == ARRAY_TYPE)    \
+ && tree_low_cst (TYPE_SIZE (TYPE), 1) >= 128 ? 128    \
+    : DATA_ALIGNMENT (TYPE, ALIGN))

  /* Alignment of field after `int : 0' in a structure.  */
  #define EMPTY_FIELD_BOUNDARY 32


2011-01-27  Pat Wellander <pwelland@codesourcery.com>

   * gcc.target/powerpc/altivec-34.c: Test AltiVec local array alignment

Index: gcc.target/powerpc/altivec-34.c
===================================================================
--- gcc.target/powerpc/altivec-34.c       (revision 0)
+++ gcc.target/powerpc/altivec-34.c       (revision 0)
@@ -0,0 +1,27 @@ 
+/* altivec-34.c */
+/* { dg-do run { target powerpc*-*-* } } */
+/* { dg-options "-O0 -maltivec" } */
+
+#include <stdlib.h>
+
+/* Align large local Altivec arrays at 128 bits */
+
+int main(void)
+{
+  long a1[5], a2[6], a3[4];
+  char c1[4], c2[17];
+  if (((long)(&a1[0])) & 0xf)
+    abort ();
+  if (((long)(&a2[0])) & 0xf)
+    abort ();
+  if (((long)(&a3[0])) & 0xf)
+    abort ();
+  if (((long)(&c1[0])) & 0xf)
+    abort ();
+  if (((long)(&c2[0])) & 0xf)
+    abort ();
+  exit (0);
+}
+
+