diff mbox

Vector alignment tracking

Message ID CABYV9SX18iNfXO-j+ZhVuYDg3Z3L+7apWq6crBfnT-0XWrSfOg@mail.gmail.com
State New
Headers show

Commit Message

Artem Shinkarov Oct. 13, 2011, 3:40 p.m. UTC
Hi

I would like to share some plans about improving the situation with
vector alignment tracking.  First of all, I would like to start with a
well-known bug: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50716.

There are several aspects of the problem:
1) We would like to avoid the quiet segmentation fault.
2) We would like to warn a user about the potential problems
considering assignment of vectors with different alignment.
3) We would like to replace obvious aligned vector assignments with
aligned move, and unaligned with unaligned.

All these aspects are interconnected and in order to find the problem,
we have to improve the alignment tracking facilities.

1) Currently in C we cannot provide information that an array is
aligned to a certain number.  The problem is hidden in the fact, that
pointer can represents an array or an address of an object.  And it
turns out that current aligned attribute doesn't help here.  My
proposal is to introduce an attribute called array_alligned (I am very
flexible on the name) which can be applied only to the pointers and
which would show that the pointer of this type represents an array,
where the first element is aligned to the given number.

2) After we have the new attribute, we can have a pass which would
check all the pointer arithmetic expressions, and in case of vectors,
mark the assignments with __builtin_assume_aligned.

3) In the separate pass we need to mark an alignments of the function
return types, in order to propagate this information through the
flow-graph.

4) In case of LTO, it becomes possible to track all the pointer
dereferences, and depending on the parameters warn, or change aligned
assignment to unaligned and vice-versa.


As a very first draft of (1) I include the patch, that introduces
array_aligned attribute.  The attribute sets is_array_flag in the
type, ans uses alignment number to store the alignment of the array.
In this implementation, we loose information about the alignment of
the pointer itself, but I don't know if we need it in this particular
situation.  Alternatively we can keep array_alignment in a separate
field, which one is better I am not sure.


Thanks,
Artem.

Comments

Andi Kleen Oct. 13, 2011, 3:54 p.m. UTC | #1
Artem Shinkarov <artyom.shinkaroff@gmail.com> writes:
>
> 1) Currently in C we cannot provide information that an array is
> aligned to a certain number.  The problem is hidden in the fact, that

Have you considered doing it the other way round: when an optimization
needs something to be aligned, make the declaration aligned?

-Andi
Artem Shinkarov Oct. 13, 2011, 4:01 p.m. UTC | #2
On Thu, Oct 13, 2011 at 4:54 PM, Andi Kleen <andi@firstfloor.org> wrote:
> Artem Shinkarov <artyom.shinkaroff@gmail.com> writes:
>>
>> 1) Currently in C we cannot provide information that an array is
>> aligned to a certain number.  The problem is hidden in the fact, that
>
> Have you considered doing it the other way round: when an optimization
> needs something to be aligned, make the declaration aligned?
>
> -Andi

Andi, I can't realistically imagine how could it work.  The problem is
that for an arbitrary arr[x], I have no idea whether it should be
aligned or not.

what if

arr = ptr +  5;
v = *(vec *) arr;

I can make arr aligned, because it would be better for performance,
but obviously, the pointer expression breaks this alignment.  But the
code is valid, because unaligned move is still possible.  So I think
that checking is a more conservative approach.

Or I am missing someting?

Thanks,
Artem.
> --
> ak@linux.intel.com -- Speaking for myself only
>
Andi Kleen Oct. 13, 2011, 4:57 p.m. UTC | #3
> Or I am missing someting?

I often see the x86 vectorizer with -mtune=generic generate a lot of
complicated code just to adjust for potential misalignment.

My thought was just if the alias oracle knows what the original
declaration is, and it's available for changes (e.g. LTO), it would be 
likely be better to just add an __attribute__((aligned()))
there.

In the general case it's probably harder, you would need some 
cost model to decide when it's worth it.

Your approach of course would still be needed for cases where this
isn't possible. But it sounded like the infrastructure you're building
could in principle do both.

-Andi
Jakub Jelinek Oct. 13, 2011, 5:04 p.m. UTC | #4
On Thu, Oct 13, 2011 at 06:57:47PM +0200, Andi Kleen wrote:
> > Or I am missing someting?
> 
> I often see the x86 vectorizer with -mtune=generic generate a lot of
> complicated code just to adjust for potential misalignment.
> 
> My thought was just if the alias oracle knows what the original
> declaration is, and it's available for changes (e.g. LTO), it would be 
> likely be better to just add an __attribute__((aligned()))
> there.
> 
> In the general case it's probably harder, you would need some 
> cost model to decide when it's worth it.

GCC already does that on certain targets, see
increase_alignment in tree-vectorizer.c.  Plus, various backends attempt
to align larger arrays more than they have to be aligned.

	Jakub
Richard Biener Oct. 14, 2011, 10:19 a.m. UTC | #5
On Thu, Oct 13, 2011 at 6:57 PM, Andi Kleen <andi@firstfloor.org> wrote:
>> Or I am missing someting?
>
> I often see the x86 vectorizer with -mtune=generic generate a lot of
> complicated code just to adjust for potential misalignment.
>
> My thought was just if the alias oracle knows what the original
> declaration is, and it's available for changes (e.g. LTO), it would be
> likely be better to just add an __attribute__((aligned()))
> there.
>
> In the general case it's probably harder, you would need some
> cost model to decide when it's worth it.
>
> Your approach of course would still be needed for cases where this
> isn't possible. But it sounded like the infrastructure you're building
> could in principle do both.

The vectorizer already does that.

Richard.

> -Andi
>
diff mbox

Patch

Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	(revision 179906)
+++ gcc/c-family/c-common.c	(working copy)
@@ -341,6 +341,7 @@  static tree handle_destructor_attribute
 static tree handle_mode_attribute (tree *, tree, tree, int, bool *);
 static tree handle_section_attribute (tree *, tree, tree, int, bool *);
 static tree handle_aligned_attribute (tree *, tree, tree, int, bool *);
+static tree handle_aligned_array_attribute (tree *, tree, tree, int, bool *);
 static tree handle_weak_attribute (tree *, tree, tree, int, bool *) ;
 static tree handle_alias_ifunc_attribute (bool, tree *, tree, tree, bool *);
 static tree handle_ifunc_attribute (tree *, tree, tree, int, bool *);
@@ -643,6 +644,8 @@  const struct attribute_spec c_common_att
 			      handle_section_attribute, false },
   { "aligned",                0, 1, false, false, false,
 			      handle_aligned_attribute, false },
+  { "aligned_array",          0, 1, false, false, false,
+			      handle_aligned_array_attribute, false },
   { "weak",                   0, 0, true,  false, false,
 			      handle_weak_attribute, false },
   { "ifunc",                  1, 1, true,  false, false,
@@ -6682,6 +6685,26 @@  handle_section_attribute (tree *node, tr
     }
 
   return NULL_TREE;
+}
+
+/* Handle "aligned_array" attribute.  */
+static tree
+handle_aligned_array_attribute (tree *node, tree ARG_UNUSED (name), tree args,
+				int flags, bool *no_add_attrs)
+{
+  if (!TYPE_P (*node) || !POINTER_TYPE_P (*node))
+    {
+      error ("array_alignment attribute must be applied to a pointer-type");
+      *no_add_attrs = true;
+    }
+  else
+    {
+      tree ret = handle_aligned_attribute (node, name, args, flags, no_add_attrs);
+      TYPE_IS_ARRAY (*node) = true;
+      return ret;
+    }
+
+  return NULL_TREE;
 }
 
 /* Handle a "aligned" attribute; arguments as in
Index: gcc/tree.h
===================================================================
--- gcc/tree.h	(revision 179906)
+++ gcc/tree.h	(working copy)
@@ -2149,6 +2149,7 @@  struct GTY(()) tree_block {
 #define TYPE_NEXT_VARIANT(NODE) (TYPE_CHECK (NODE)->type_common.next_variant)
 #define TYPE_MAIN_VARIANT(NODE) (TYPE_CHECK (NODE)->type_common.main_variant)
 #define TYPE_CONTEXT(NODE) (TYPE_CHECK (NODE)->type_common.context)
+#define TYPE_IS_ARRAY(NODE) (TYPE_CHECK (NODE)->type_common.is_array_flag)
 
 /* Vector types need to check target flags to determine type.  */
 extern enum machine_mode vector_type_mode (const_tree);
@@ -2411,6 +2412,7 @@  struct GTY(()) tree_type_common {
   unsigned lang_flag_5 : 1;
   unsigned lang_flag_6 : 1;
 
+  unsigned is_array_flag: 1;
   unsigned int align;
   alias_set_type alias_set;
   tree pointer_to;