Re: increase alignment of global structs in increase_alignment pass

Re: increase alignment of global structs in increase_alignment pass

[Prathamesh Kulkarni]

On 7 June 2016 at 20:17, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
>
> After your commit these tests fail on AArch64:
>
> UNRESOLVED: gcc.dg/vect/section-anchors-vect-70.c scan-ipa-dump-times
> increase_alignment "Increasing alignment of decl" 0
> UNRESOLVED: gcc.dg/vect/section-anchors-vect-70.c scan-ipa-dump-times
> increase_alignment "Increasing alignment of decl" 3
> UNRESOLVED: gcc.dg/vect/section-anchors-vect-71.c scan-ipa-dump-times
> increase_alignment "Increasing alignment of decl" 0
> UNRESOLVED: gcc.dg/vect/section-anchors-vect-72.c scan-ipa-dump-times
> increase_alignment "Increasing alignment of decl" 0
> UNRESOLVED: gcc.dg/vect/section-anchors-vect-72.c scan-ipa-dump-times
> increase_alignment "Increasing alignment of decl" 3
>
> Did you mean to commit these tests as aligned-section-anchors-vect-*.c? That
> would enable the -fdump-ipa-increase_alignment-details that appears to be
> required by these tests.
Oops, sorry for that. I had bootstrapped and tested the patch on
aarch64-linux-gnu before committing,
I wonder why these didn't show up with compare_tests script or maybe I
missed something :/
I renamed the above files in r237207 and the tests change from
UNRESOLVED to PASS.

Thanks,
Prathamesh
>
> Wilco
>

Re: increase alignment of global structs in increase_alignment pass

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 8619 bytes --]

On 19 May 2016 at 13:19, Richard Biener <rguenther@suse.de> wrote:
> On Thu, 19 May 2016, Prathamesh Kulkarni wrote:
>
>> On 18 May 2016 at 19:38, Richard Biener <rguenther@suse.de> wrote:
>> > On Wed, 18 May 2016, Prathamesh Kulkarni wrote:
>> >
>> >> On 17 May 2016 at 18:36, Richard Biener <rguenther@suse.de> wrote:
>> >> > On Wed, 11 May 2016, Prathamesh Kulkarni wrote:
>> >> >
>> >> >> On 6 May 2016 at 17:20, Richard Biener <rguenther@suse.de> wrote:
>> >> >> >
>> >> >> > You can't simply use
>> >> >> >
>> >> >> > +      offset = int_byte_position (field);
>> >> >> >
>> >> >> > as it can be placed at variable offset which will make int_byte_position
>> >> >> > ICE.  Note it also returns a truncated byte position (bit position
>> >> >> > stripped) which may be undesirable here.  I think you want to use
>> >> >> > bit_position and if and only if DECL_FIELD_OFFSET and
>> >> >> > DECL_FIELD_BIT_OFFSET are INTEGER_CST.
>> >> >> oops, I didn't realize offsets could be variable.
>> >> >> Will that be the case only for VLA member inside struct ?
>> >> >
>> >> > And non-VLA members after such member.
>> >> >
>> >> >> > Your observation about the expensiveness of the walk still stands I guess
>> >> >> > and eventually you should at least cache the
>> >> >> > get_vec_alignment_for_record_decl cases.  Please make those workers
>> >> >> > _type rather than _decl helpers.
>> >> >> Done
>> >> >> >
>> >> >> > You seem to simply get at the maximum vectorized field/array element
>> >> >> > alignment possible for all arrays - you could restrict that to
>> >> >> > arrays with at least vector size (as followup).
>> >> >> Um sorry, I didn't understand this part.
>> >> >
>> >> > It doesn't make sense to align
>> >> >
>> >> > struct { int a; int b; int c; int d; float b[3]; int e; };
>> >> >
>> >> > because we have a float[3] member.  There is no vector size that
>> >> > would cover the float[3] array.
>> >> Thanks for the explanation.
>> >> So we do not want to align struct if sizeof (array_field) < sizeof
>> >> (vector_type).
>> >> This issue is also present without patch for global arrays, so I modified
>> >> get_vec_alignment_for_array_type, to return 0 if sizeof (array_type) <
>> >> sizeof (vectype).
>> >> >
>> >> >> >
>> >> >> > +  /* Skip artificial decls like typeinfo decls or if
>> >> >> > +     record is packed.  */
>> >> >> > +  if (DECL_ARTIFICIAL (record_decl) || TYPE_PACKED (type))
>> >> >> > +    return 0;
>> >> >> >
>> >> >> > I think we should honor DECL_USER_ALIGN as well and not mess with those
>> >> >> > decls.
>> >> >> Done
>> >> >> >
>> >> >> > Given the patch now does quite some extra work it might make sense
>> >> >> > to split the symtab part out of the vect_can_force_dr_alignment_p
>> >> >> > predicate and call that early.
>> >> >> In the patch I call symtab_node::can_increase_alignment_p early. I tried
>> >> >> moving that to it's callers - vect_compute_data_ref_alignment and
>> >> >> increase_alignment::execute, however that failed some tests in vect, and
>> >> >> hence I didn't add the following hunk in the patch. Did I miss some
>> >> >> check ?
>> >> >
>> >> > Not sure.
>> >> >
>> >> >> diff --git a/gcc/tree-vect-data-refs.c b/gcc/tree-vect-data-refs.c
>> >> >> index 7652e21..2c1acee 100644
>> >> >> --- a/gcc/tree-vect-data-refs.c
>> >> >> +++ b/gcc/tree-vect-data-refs.c
>> >> >> @@ -795,7 +795,10 @@ vect_compute_data_ref_alignment (struct data_reference *dr)
>> >> >>    && TREE_CODE (TREE_OPERAND (base, 0)) == ADDR_EXPR)
>> >> >>   base = TREE_OPERAND (TREE_OPERAND (base, 0), 0);
>> >> >>
>> >> >> -      if (!vect_can_force_dr_alignment_p (base, TYPE_ALIGN (vectype)))
>> >> >> +      if (!(TREE_CODE (base) == VAR_DECL
>> >> >> +            && decl_in_symtab_p (base)
>> >> >> +            && symtab_node::get (base)->can_increase_alignment_p ()
>> >> >> +            && vect_can_force_dr_alignment_p (base, TYPE_ALIGN (vectype))))
>> >> >>   {
>> >> >>    if (dump_enabled_p ())
>> >> >>      {
>> >> >
>> >> > +  for (tree field = first_field (type);
>> >> > +       field != NULL_TREE;
>> >> > +       field = DECL_CHAIN (field))
>> >> > +    {
>> >> > +      /* Skip if not FIELD_DECL or has variable offset.  */
>> >> > +      if (TREE_CODE (field) != FIELD_DECL
>> >> > +         || TREE_CODE (DECL_FIELD_OFFSET (field)) != INTEGER_CST
>> >> > +         || TREE_CODE (DECL_FIELD_BIT_OFFSET (field)) != INTEGER_CST
>> >> > +         || DECL_USER_ALIGN (field)
>> >> > +         || DECL_ARTIFICIAL (field))
>> >> > +       continue;
>> >> >
>> >> > you can stop processing the type and return 0 here if the offset
>> >> > is not an INTEGER_CST.  All following fields will have the same issue.
>> >> >
>> >> > +      /* FIXME: is this check necessary since we check for variable
>> >> > offset above ?  */
>> >> > +      if (TREE_CODE (offset_tree) != INTEGER_CST)
>> >> > +       continue;
>> >> >
>> >> > the check should not be necessary.
>> >> >
>> >> >       offset = tree_to_uhwi (offset_tree);
>> >> >
>> >> > but in theory offset_tree might not fit a unsigned HOST_WIDE_INT, so
>> >> > instead of for INTEGER_CST please check ! tree_fits_uhwi_p (offset_tree).
>> >> > As above all following fields will also fail the check if this fails so
>> >> > you can return 0 early.
>> >> >
>> >> > +static unsigned
>> >> > +get_vec_alignment_for_type (tree type)
>> >> > +{
>> >> > +  if (type == NULL_TREE)
>> >> > +    return 0;
>> >> > +
>> >> > +  gcc_assert (TYPE_P (type));
>> >> > +
>> >> > +  unsigned *slot = type_align_map->get (type);
>> >> > +  if (slot)
>> >> > +    return *slot;
>> >> >
>> >> > I suggest to apply the caching only for the RECORD_TYPE case to keep
>> >> > the size of the map low.
>> >> >
>> >> > Otherwise the patch looks ok now.
>> >> I have done the suggested changes in attached patch.
>> >> Is this version OK to commit after bootstrap+test ?
>> >> The patch survives cross-testing on arm*-*-* and aarch64*-*-*.
>> >
>> > +  tree type_size_tree = TYPE_SIZE (type);
>> > +  if (!type_size_tree)
>> > +    return TYPE_ALIGN (vectype);
>> > +
>> > +  if (TREE_CODE (type_size_tree) != INTEGER_CST)
>> > +    return TYPE_ALIGN (vectype);
>> > +
>> > +  /* If array has constant size, ensure that it's at least equal to
>> > +     size of vector type.  */
>> > +  if (!tree_fits_uhwi_p (type_size_tree))
>> > +    return TYPE_ALIGN (vectype);
>> > +  unsigned HOST_WIDE_INT type_size = tree_to_uhwi (type_size_tree);
>> > +
>> > +  tree vectype_size_tree = TYPE_SIZE (vectype);
>> > +  if (!tree_fits_uhwi_p (vectype_size_tree))
>> > +    return TYPE_ALIGN (vectype);
>> > +
>> > +  unsigned HOST_WIDE_INT vectype_size = tree_to_uhwi (vectype_size_tree);
>> > +  return (type_size > vectype_size) ? TYPE_ALIGN (vectype) : 0;
>> >
>> > please change this to
>> >
>> >   if (! TYPE_SIZE (type)
>> >       || TREE_CODE (TYPE_SIZE (type)) != INTEGER_CST
>> >       || tree_int_cst_lt (TYPE_SIZE (type), TYPE_SIZE (vectype)))
>> >     return 0;
>> >
>> >   return TYPE_ALIGN (vectype);
>> >
>> > consistent with the handling for "VLA" records.
>> >
>> > +  unsigned *slot = type_align_map->get (type);
>> > +  if (slot)
>> > +    return *slot;
>> > +
>> > +  unsigned max_align = 0, alignment;
>> > +  HOST_WIDE_INT offset;
>> > +  tree offset_tree;
>> > +
>> > +  if (TYPE_PACKED (type))
>> > +    return 0;
>> > +
>> >
>> > move the tye_align_map query after the TYPE_PACKED check.
>> >
>> > +      /* We don't need to process the type further if offset is variable,
>> > +        since the offsets of remaining members will also be variable.  */
>> > +      if (TREE_CODE (DECL_FIELD_OFFSET (field)) != INTEGER_CST
>> > +         || TREE_CODE (DECL_FIELD_BIT_OFFSET (field)) != INTEGER_CST)
>> > +       return 0;
>> >
>> > just break; here (and in the other case returning zero).  If a
>> > previous record field said we'd want to align we still should align.
>> > Also we want to store 0 into the type_align_map as well.
>> >
>> > Ok with those changes.
>> Hi,
>> Is this version OK ?
>> Cross tested on arm*-*-* and aarch64*-*-*
>
> Yes.  Btw, please always do a regular bootstrap and regtest, cross-testing
> alone is _not_ sufficient!
Thanks, bootstrapped and tested on aarch64-linux-gnu, ppc64le-linux-gnu.
Unfortunately I screwed up while committing (added the patch but not
the test-cases :/).
Patch committed in r236502, test-cases committed in r236503.

Thanks,
Prathamesh
>
> Thanks,
> Richard.
>
>> Thanks,
>> Prathamesh
>> >
>> > Thanks,
>> > Richard.
>>
>
> --
> Richard Biener <rguenther@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

[-- Attachment #2: patch-4.diff --]
[-- Type: text/plain, Size: 8336 bytes --]

diff --git a/gcc/testsuite/gcc.dg/vect/section-anchors-vect-70.c b/gcc/testsuite/gcc.dg/vect/section-anchors-vect-70.c
new file mode 100644
index 0000000..7010a52
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vect/section-anchors-vect-70.c
@@ -0,0 +1,33 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target section_anchors } */
+/* { dg-require-effective-target vect_int } */
+
+#define N 32
+
+/* Increase alignment of struct if an array's offset is multiple of alignment of
+   vector type corresponding to it's scalar type.
+   For the below test-case:
+   offsetof(e) == 8 bytes. 
+   i) For arm: let x = alignment of vector type corresponding to int,
+   x == 8 bytes.
+   Since offsetof(e) % x == 0, set DECL_ALIGN(a, b, c) to x.
+   ii) For aarch64, ppc: x == 16 bytes.
+   Since offsetof(e) % x != 0, don't increase alignment of a, b, c.
+*/
+
+static struct A {
+  int p1, p2;
+  int e[N];
+} a, b, c;
+
+int foo(void)
+{
+  for (int i = 0; i < N; i++)
+    a.e[i] = b.e[i] + c.e[i];
+
+   return a.e[0];
+}
+
+/* { dg-final { scan-ipa-dump-times "Increasing alignment of decl" 0 "increase_alignment" { target aarch64*-*-* } } } */
+/* { dg-final { scan-ipa-dump-times "Increasing alignment of decl" 0 "increase_alignment" { target powerpc64*-*-* } } } */
+/* { dg-final { scan-ipa-dump-times "Increasing alignment of decl" 3 "increase_alignment" { target arm*-*-* } } } */
diff --git a/gcc/testsuite/gcc.dg/vect/section-anchors-vect-71.c b/gcc/testsuite/gcc.dg/vect/section-anchors-vect-71.c
new file mode 100644
index 0000000..7cbd1dc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vect/section-anchors-vect-71.c
@@ -0,0 +1,25 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target section_anchors } */
+/* { dg-require-effective-target vect_int } */
+
+/* Should not increase alignment of the struct because
+   sizeof (A.e) < sizeof(corresponding vector type).  */
+
+#define N 3
+
+static struct A {
+  int p1, p2;
+  int e[N];
+} a, b, c;
+
+int foo(void)
+{
+  for (int i = 0; i < N; i++)
+    a.e[i] = b.e[i] + c.e[i];
+
+   return a.e[0];
+}
+
+/* { dg-final { scan-ipa-dump-times "Increasing alignment of decl" 0 "increase_alignment" { target aarch64*-*-* } } } */
+/* { dg-final { scan-ipa-dump-times "Increasing alignment of decl" 0 "increase_alignment" { target powerpc64*-*-* } } } */
+/* { dg-final { scan-ipa-dump-times "Increasing alignment of decl" 0 "increase_alignment" { target arm*-*-* } } } */
diff --git a/gcc/testsuite/gcc.dg/vect/section-anchors-vect-72.c b/gcc/testsuite/gcc.dg/vect/section-anchors-vect-72.c
new file mode 100644
index 0000000..873fabe
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/vect/section-anchors-vect-72.c
@@ -0,0 +1,29 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target section_anchors } */
+/* { dg-require-effective-target vect_int } */
+
+#define N 32
+
+/* Clone of section-anchors-vect-70.c having nested struct.  */
+
+struct S
+{
+  int e[N];
+};
+
+static struct A {
+  int p1, p2;
+  struct S s; 
+} a, b, c;
+
+int foo(void)
+{
+  for (int i = 0; i < N; i++)
+    a.s.e[i] = b.s.e[i] + c.s.e[i];
+
+   return a.s.e[0];
+}
+
+/* { dg-final { scan-ipa-dump-times "Increasing alignment of decl" 0 "increase_alignment" { target aarch64*-*-* } } } */
+/* { dg-final { scan-ipa-dump-times "Increasing alignment of decl" 0 "increase_alignment" { target powerpc64*-*-* } } } */
+/* { dg-final { scan-ipa-dump-times "Increasing alignment of decl" 3 "increase_alignment" { target arm*-*-* } } } */
diff --git a/gcc/tree-vectorizer.c b/gcc/tree-vectorizer.c
index 2b25b45..bd04cd7 100644
--- a/gcc/tree-vectorizer.c
+++ b/gcc/tree-vectorizer.c
@@ -794,38 +793,142 @@ make_pass_slp_vectorize (gcc::context *ctxt)
      This should involve global alignment analysis and in the future also
      array padding.  */
 
+static unsigned get_vec_alignment_for_type (tree);
+static hash_map<tree, unsigned> *type_align_map;
+
+/* Return alignment of array's vector type corresponding to scalar type.
+   0 if no vector type exists.  */
+static unsigned
+get_vec_alignment_for_array_type (tree type) 
+{
+  gcc_assert (TREE_CODE (type) == ARRAY_TYPE);
+
+  tree vectype = get_vectype_for_scalar_type (strip_array_types (type));
+  if (!vectype
+      || !TYPE_SIZE (type)
+      || TREE_CODE (TYPE_SIZE (type)) != INTEGER_CST
+      || tree_int_cst_lt (TYPE_SIZE (type), TYPE_SIZE (vectype)))
+    return 0;
+
+  return TYPE_ALIGN (vectype);
+}
+
+/* Return alignment of field having maximum alignment of vector type
+   corresponding to it's scalar type. For now, we only consider fields whose
+   offset is a multiple of it's vector alignment.
+   0 if no suitable field is found.  */
+static unsigned
+get_vec_alignment_for_record_type (tree type) 
+{
+  gcc_assert (TREE_CODE (type) == RECORD_TYPE);
+
+  unsigned max_align = 0, alignment;
+  HOST_WIDE_INT offset;
+  tree offset_tree;
+
+  if (TYPE_PACKED (type))
+    return 0;
+
+  unsigned *slot = type_align_map->get (type);
+  if (slot)
+    return *slot;
+
+  for (tree field = first_field (type);
+       field != NULL_TREE;
+       field = DECL_CHAIN (field))
+    {
+      /* Skip if not FIELD_DECL or if alignment is set by user.  */ 
+      if (TREE_CODE (field) != FIELD_DECL
+	  || DECL_USER_ALIGN (field)
+	  || DECL_ARTIFICIAL (field))
+	continue;
+
+      /* We don't need to process the type further if offset is variable,
+	 since the offsets of remaining members will also be variable.  */
+      if (TREE_CODE (DECL_FIELD_OFFSET (field)) != INTEGER_CST
+	  || TREE_CODE (DECL_FIELD_BIT_OFFSET (field)) != INTEGER_CST)
+	break;
+
+      /* Similarly stop processing the type if offset_tree
+	 does not fit in unsigned HOST_WIDE_INT.  */
+      offset_tree = bit_position (field);
+      if (!tree_fits_uhwi_p (offset_tree))
+	break;
+
+      offset = tree_to_uhwi (offset_tree); 
+      alignment = get_vec_alignment_for_type (TREE_TYPE (field));
+
+      /* Get maximum alignment of vectorized field/array among those members
+	 whose offset is multiple of the vector alignment.  */ 
+      if (alignment
+	  && (offset % alignment == 0)
+	  && (alignment > max_align))
+	max_align = alignment;
+    }
+
+  type_align_map->put (type, max_align);
+  return max_align;
+}
+
+/* Return alignment of vector type corresponding to decl's scalar type
+   or 0 if it doesn't exist or the vector alignment is lesser than
+   decl's alignment.  */
+static unsigned
+get_vec_alignment_for_type (tree type)
+{
+  if (type == NULL_TREE)
+    return 0;
+
+  gcc_assert (TYPE_P (type));
+
+  static unsigned alignment = 0;
+  switch (TREE_CODE (type))
+    {
+      case ARRAY_TYPE:
+	alignment = get_vec_alignment_for_array_type (type);
+	break;
+      case RECORD_TYPE:
+	alignment = get_vec_alignment_for_record_type (type);
+	break;
+      default:
+	alignment = 0;
+	break;
+    }
+
+  return (alignment > TYPE_ALIGN (type)) ? alignment : 0;
+}
+
+/* Entry point to increase_alignment pass.  */
 static unsigned int
 increase_alignment (void)
 {
   varpool_node *vnode;
 
   vect_location = UNKNOWN_LOCATION;
+  type_align_map = new hash_map<tree, unsigned>;
 
   /* Increase the alignment of all global arrays for vectorization.  */
   FOR_EACH_DEFINED_VARIABLE (vnode)
     {
-      tree vectype, decl = vnode->decl;
-      tree t;
+      tree decl = vnode->decl;
       unsigned int alignment;
 
-      t = TREE_TYPE (decl);
-      if (TREE_CODE (t) != ARRAY_TYPE)
-        continue;
-      vectype = get_vectype_for_scalar_type (strip_array_types (t));
-      if (!vectype)
-        continue;
-      alignment = TYPE_ALIGN (vectype);
-      if (DECL_ALIGN (decl) >= alignment)
-        continue;
-
-      if (vect_can_force_dr_alignment_p (decl, alignment))
+      if ((decl_in_symtab_p (decl)
+	  && !symtab_node::get (decl)->can_increase_alignment_p ())
+	  || DECL_USER_ALIGN (decl) || DECL_ARTIFICIAL (decl))
+	continue;
+
+      alignment = get_vec_alignment_for_type (TREE_TYPE (decl));
+      if (alignment && vect_can_force_dr_alignment_p (decl, alignment))
         {
-	  vnode->increase_alignment (TYPE_ALIGN (vectype));
+	  vnode->increase_alignment (alignment);
           dump_printf (MSG_NOTE, "Increasing alignment of decl: ");
           dump_generic_expr (MSG_NOTE, TDF_SLIM, decl);
           dump_printf (MSG_NOTE, "\n");
         }
     }
+
+  delete type_align_map;
   return 0;
 }
 

[-- Attachment #3: ChangeLog.patch --]
[-- Type: text/x-patch, Size: 574 bytes --]

2016-05-19  Prathamesh Kulkarni  <prathamesh.kulkarni@linaro.org>

	* tree-vectorizer.c (get_vec_alignment_for_decl): New static function.
	(get_vec_alignment_for_array_decl): Likewise.
	(get_vec_alignment_for_record_decl): Likewise.
	(increase_alignment::execute): Move code to find alignment to
	get_vec_alignment_for_array_decl and call get_vec_alignment_for_decl.
	(type_align_map): New hash_map.

testsuite/
	* gcc.dg/vect/section-anchors-vect-70.c: New test-case.
	* gcc.dg/vect/section-anchors-vect-71.c: Likewise.
	* gcc.dg/vect/section-anchors-vect-72.c: Likewise.