diff mbox

Vector misalignment

Message ID AANLkTimAJL33OqDH0Qv=FrGRpkk6q3zkNed9L+5QCp-i@mail.gmail.com
State New
Headers show

Commit Message

Artem Shinkarov Aug. 16, 2010, 6:15 p.m. UTC 
This patch adds several new warnings for possible misaligned vector
types. For example the following code

#define vector(elcount, type)  \
__attribute__((vector_size((elcount)*sizeof(type)))) type

int *array;
vector (4, int) vec;

array = (int *) malloc (128 * sizeof (int));
*((vector (4, int) *) &array[1]) = vec;

would produce no warnings, but causes a segmentation fault on intel
and powerpc architecture.

New warning type Wmisaligned is introduced.

Changelog:

2010-08-16 Artem Shinkarov <artyom.shinkaroff@gmail.com>

        gcc/
        * c-typeck.c (build_c_cast): Add warning.
        * common.opt: New warning type Wmisaligned.
        * tree-vect-generic.c (check_alignment): Check possible misalignment
        in vector types.
        (expand_vector_operations_1): Adjust.
        * Makefile.in: New dependency.
        * convert.c (convert_to_pointer): Add warning.

        gcc/doc/
        * invoke.texi: Adjust.

        gcc/testsuite/
        * gcc.dg/vector-check-align.c: New test-case.


bootstrapped and tested on x86-unknown-linux.

Comments

Andrew Pinski Aug. 16, 2010, 6:18 p.m. UTC | #1 
On Mon, Aug 16, 2010 at 11:15 AM, Artem Shinkarov
<artyom.shinkaroff@gmail.com> wrote:
> would produce no warnings, but causes a segmentation fault on intel
> and powerpc architecture.

How will it produce a seg fault on PPC (VMX) targets?  The VMX load
instructions don't fault but rather just use the "aligned" address.

-- Pinski
Artem Shinkarov Aug. 16, 2010, 6:25 p.m. UTC | #2 
On Mon, Aug 16, 2010 at 7:18 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Mon, Aug 16, 2010 at 11:15 AM, Artem Shinkarov
> <artyom.shinkaroff@gmail.com> wrote:
>> would produce no warnings, but causes a segmentation fault on intel
>> and powerpc architecture.
>
> How will it produce a seg fault on PPC (VMX) targets?  The VMX load
> instructions don't fault but rather just use the "aligned" address.
>
> -- Pinski
>

I may be wrong about PPC, I tested it quite some time ago on mac with
AltiVec, but the point is that gcc inserts an aligned vector move
instruction instead of unaligned (that surely happens on Intel). It is
correct in terms of gcc, but if user made a mistake there is no chance
that the compiler would warn him.


Artem.
Andrew Pinski Aug. 16, 2010, 6:28 p.m. UTC | #3 
On Mon, Aug 16, 2010 at 11:15 AM, Artem Shinkarov
<artyom.shinkaroff@gmail.com> wrote:
> 2010-08-16 Artem Shinkarov <artyom.shinkaroff@gmail.com>
>
>        gcc/
>        * c-typeck.c (build_c_cast): Add warning.
>        * common.opt: New warning type Wmisaligned.

We already have -Wcast-align, why are you adding a new option?  Also
is there a reason why you are adding code to the C front-end without a
corresponding change to the C++ front-end?

Thanks,
Andrew Pinski
Artem Shinkarov Aug. 16, 2010, 6:51 p.m. UTC | #4 
On Mon, Aug 16, 2010 at 7:28 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Mon, Aug 16, 2010 at 11:15 AM, Artem Shinkarov
> <artyom.shinkaroff@gmail.com> wrote:
>> 2010-08-16 Artem Shinkarov <artyom.shinkaroff@gmail.com>
>>
>>        gcc/
>>        * c-typeck.c (build_c_cast): Add warning.
>>        * common.opt: New warning type Wmisaligned.
>
> We already have -Wcast-align, why are you adding a new option?

Probably you are right, I thought that I could come up with something
more than just pointer checks, I was mainly concentrated on vector
operations and possible misalignments there. But for this patch we
could change Wmisalignment to Wcast-align, currently it fits.

>  Also
> is there a reason why you are adding code to the C front-end without a
> corresponding change to the C++ front-end?

Well, mostly lack of time and knowledge.

At this point I would like to gather some ideas about the way we could
produce these warnings at all. My current approach has some
disadvantages, for instance it would produce warnings for the correct
code. But currently I don't know how we can make it better,
considering the fact that we want to warn on -O0 as well.


Thanks,
Artem.
Michael Meissner Aug. 18, 2010, 10:26 p.m. UTC | #5 
On Mon, Aug 16, 2010 at 11:18:04AM -0700, Andrew Pinski wrote:
> On Mon, Aug 16, 2010 at 11:15 AM, Artem Shinkarov
> <artyom.shinkaroff@gmail.com> wrote:
> > would produce no warnings, but causes a segmentation fault on intel
> > and powerpc architecture.
> 
> How will it produce a seg fault on PPC (VMX) targets?  The VMX load
> instructions don't fault but rather just use the "aligned" address.

True for VMX (altivec) target, but the VSX instruction set in power7 no longer
ignores the bottom bits for the vector loads/stores, and so could potentially
have an alignment failure.

However, the VSX instructions will not generate an alignment trap if the pointer
is approprilately aligned for 2 or 4 bytes (depending on the memory reference
instruction used).
Michael Meissner Aug. 18, 2010, 10:45 p.m. UTC | #6 
On Wed, Aug 18, 2010 at 06:26:50PM -0400, Michael Meissner wrote:
> On Mon, Aug 16, 2010 at 11:18:04AM -0700, Andrew Pinski wrote:
> > On Mon, Aug 16, 2010 at 11:15 AM, Artem Shinkarov
> > <artyom.shinkaroff@gmail.com> wrote:
> > > would produce no warnings, but causes a segmentation fault on intel
> > > and powerpc architecture.
> > 
> > How will it produce a seg fault on PPC (VMX) targets?  The VMX load
> > instructions don't fault but rather just use the "aligned" address.
> 
> True for VMX (altivec) target, but the VSX instruction set in power7 no longer
> ignores the bottom bits for the vector loads/stores, and so could potentially
> have an alignment failure.

Hmmm, I probably was unclear here.

The Altivec memory instructions (lvx, stvx) still ignore the bottom bits and
would never generate an alignment trap with an unaligned pointer (but it would
read the wrong data).

The VSX memory instructions (lxvw4x, lxvd2x, lxdx, lxvdsx, stxvw4x, stxvd2x,
stxdx) do not ignore the bottom bits, and can suffer an alignment trap is the
address is not aligned (alignment depends on the instruction used - the
compiler generates lxvw4x/lxvd2x depending on whether the alignment of the base
type is 8 or 4 byte aligned).  So, if you have an array of doubles or floats,
and the compiler generates a VSX memory instruction, you won't see an alignment
trap.  If you hand it a pointer that is not properly aligned for double/float,
then it may trap.

If you select VSX (-mvsx or -mcpu=power7), the compiler will generate the VSX
memory instructions instead of the Altivec memory instructions that access an
entire vector.
diff mbox

Patch

Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 163280)
+++ gcc/doc/invoke.texi	(working copy)
@@ -249,7 +249,7 @@  Objective-C and Objective-C++ Dialects}.
 -Wmain  -Wmissing-braces  -Wmissing-field-initializers @gol
 -Wmissing-format-attribute  -Wmissing-include-dirs @gol
 -Wno-mudflap @gol
--Wno-multichar  -Wnonnull  -Wno-overflow @gol
+-Wno-multichar  -Wnonnull  -Wno-overflow @gol -Wmisaligned
 -Woverlength-strings  -Wpacked  -Wpacked-bitfield-compat  -Wpadded @gol
 -Wparentheses  -Wpedantic-ms-format -Wno-pedantic-ms-format @gol
 -Wpointer-arith  -Wno-pointer-to-int-cast @gol
@@ -4317,6 +4317,25 @@  struct bar @{
 @end group
 @end smallexample
 
+@item -Wmisaligned
+@opindex Wmisaligned
+@opindex Wno-misaligned
+Warn if an expression can cause misalignment. Commonly it is useful
+for vector operations. If you cast a misaligned memory to a vector
+type it is possible to end up with a segmentation fault. Like in
+the following example.
+
+@smallexample
+#define vector(elcount, type)  \
+__attribute__((vector_size((elcount)*sizeof(type)))) type
+
+int *array;
+vector (4, int) vec;
+
+array = (int *) malloc (128 * sizeof (int));
+*((vector (4, int) *) &array[1]) = vec;
+@end smallexample
+
 @item -Wpacked-bitfield-compat
 @opindex Wpacked-bitfield-compat
 @opindex Wno-packed-bitfield-compat
Index: gcc/testsuite/gcc.dg/vector-check-align.c
===================================================================
--- gcc/testsuite/gcc.dg/vector-check-align.c	(revision 0)
+++ gcc/testsuite/gcc.dg/vector-check-align.c	(revision 0)
@@ -0,0 +1,25 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O0 -Wmisaligned" } */
+#define vector(elcount, type)  \
+__attribute__((vector_size((elcount)*sizeof(type)))) type
+
+typedef int unaligned_intv __attribute__((vector_size(16), aligned(4)));
+
+vector (4, int) *
+loadu (int *p) {
+  return (unaligned_intv *)p; /* { dg-warning "" } */
+}
+
+
+int main (int argc, char *argv[]) {
+    int * array;
+    vector (4, int) v = {argc, 1,2,3};
+
+    array = (int *) __builtin_malloc (137 * sizeof (int));
+    *((vector(4, int) *)(1+ array)) = v; /* { dg-warning "" } */
+    *(loadu (&array[0])) = v; 
+
+    return array[argc];
+}
+
+
Index: gcc/c-typeck.c
===================================================================
--- gcc/c-typeck.c	(revision 163280)
+++ gcc/c-typeck.c	(working copy)
@@ -4674,6 +4674,17 @@  build_c_cast (location_t loc, tree type,
 		    OPT_Wint_to_pointer_cast, "cast to pointer from integer "
 		    "of different size");
 
+      if (TREE_CODE (type) == POINTER_TYPE 
+          && TREE_CODE (TREE_TYPE (type)) == VECTOR_TYPE
+          && TREE_CODE (TREE_TYPE (expr)) == POINTER_TYPE)
+        {
+            if (TYPE_ALIGN (TREE_TYPE (type))
+                >  MAX (TYPE_ALIGN (TREE_TYPE (TREE_TYPE (expr))),
+                        get_pointer_alignment (expr, 0)))
+              warning_at (loc, OPT_Wmisaligned, 
+                          "Possible vector-pointer missaligned cast");
+        }
+      
       if (warn_strict_aliasing <= 2)
         strict_aliasing_warning (otype, type, expr);
 
Index: gcc/common.opt
===================================================================
--- gcc/common.opt	(revision 163280)
+++ gcc/common.opt	(working copy)
@@ -176,6 +176,10 @@  Wpacked
 Common Var(warn_packed) Warning
 Warn when the packed attribute has no effect on struct layout
 
+Wmisaligned
+Common Var(warn_misaligned) Warning
+Warn about possible misalignments
+
 Wpadded
 Common Var(warn_padded) Warning
 Warn when padding is required to align structure members
Index: gcc/tree-vect-generic.c
===================================================================
--- gcc/tree-vect-generic.c	(revision 163280)
+++ gcc/tree-vect-generic.c	(working copy)
@@ -30,6 +30,7 @@  along with GCC; see the file COPYING3.  
 #include "tree-pass.h"
 #include "flags.h"
 #include "ggc.h"
+#include "diagnostic.h"
 
 /* Need to include rtl.h, expr.h, etc. for optabs.  */
 #include "expr.h"
@@ -383,6 +384,67 @@  type_for_widest_vector_mode (enum machin
     }
 }
 
+
+/* Cache the location for not giving a warning more than once per line.  */
+static location_t loc_cache = UNKNOWN_LOCATION;
+
+/* Show warning if alignemnt in vector assignment or alignment in 
+   vector-type mem_ref is incorrect. Works only when optimized == 1  */
+static void
+check_alignment (gimple_stmt_iterator *gsi)
+{
+  gimple stmt = gsi_stmt (*gsi);
+  tree lhs, rhs, rtype;
+  location_t loc = gimple_location (stmt);
+
+  if (optimize == 0) 
+    return;
+
+  if (gimple_assign_single_p (stmt))
+    {
+      lhs = gimple_assign_lhs (stmt);
+      rhs = gimple_assign_rhs1 (stmt);
+      rtype = TREE_TYPE (rhs);
+
+      if (TREE_CODE (rhs) == MEM_REF 
+          && TREE_CODE (TREE_TYPE (rhs)) == VECTOR_TYPE)
+        {
+          if (TYPE_ALIGN (TREE_TYPE (rhs))
+              > get_object_alignment (rhs, BIGGEST_ALIGNMENT)
+              && loc_cache != loc)
+            warning_at (loc_cache = loc, OPT_Wmisaligned, 
+                        "Possibly misaligned vector-type memory refernce "
+                        "on the assignment right hand side");
+        }
+    }
+  else
+    return;
+
+  if (TREE_CODE (lhs) == MEM_REF 
+      && TREE_CODE (TREE_TYPE (lhs)) == VECTOR_TYPE)
+    {
+      if (TYPE_ALIGN (TREE_TYPE (lhs)) 
+          > get_object_alignment (lhs, BIGGEST_ALIGNMENT)
+          && loc_cache != loc)
+        warning_at (loc_cache = loc, OPT_Wmisaligned, 
+                    "Possibly misaligned vector-type memory reference");
+    }
+
+  if (POINTER_TYPE_P (TREE_TYPE (lhs))
+      && TREE_CODE (TREE_TYPE (TREE_TYPE (lhs))) == VECTOR_TYPE)
+    {
+      if (TREE_CODE (rhs) == ADDR_EXPR
+          && TREE_CODE (TREE_TYPE (rtype)) == VECTOR_TYPE)
+        {
+          if (TYPE_ALIGN (TREE_TYPE (TREE_TYPE (lhs)))
+              > get_pointer_alignment (rhs, BIGGEST_ALIGNMENT)
+              && loc_cache != loc)
+            warning_at (loc_cache = loc, OPT_Wmisaligned, 
+                        "Possibly misalignment vector-pointer assignment");
+        }   
+    }
+}
+
 /* Process one statement.  If we identify a vector operation, expand it.  */
 
 static void
@@ -396,6 +458,8 @@  expand_vector_operations_1 (gimple_stmt_
   enum gimple_rhs_class rhs_class;
   tree new_rhs;
 
+  check_alignment (gsi);
+
   if (gimple_code (stmt) != GIMPLE_ASSIGN)
     return;
 
Index: gcc/Makefile.in
===================================================================
--- gcc/Makefile.in	(revision 163280)
+++ gcc/Makefile.in	(working copy)
@@ -3156,7 +3156,7 @@  tree-vect-generic.o : tree-vect-generic.
     $(TM_H) $(TREE_FLOW_H) $(GIMPLE_H) tree-iterator.h $(TREE_PASS_H) \
     $(FLAGS_H) $(OPTABS_H) $(MACHMODE_H) $(EXPR_H) \
     langhooks.h $(FLAGS_H) $(DIAGNOSTIC_H) gt-tree-vect-generic.h $(GGC_H) \
-    coretypes.h insn-codes.h
+    coretypes.h insn-codes.h diagnostic.h
 df-core.o : df-core.c $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) $(RTL_H) \
    insn-config.h $(RECOG_H) $(FUNCTION_H) $(REGS_H) alloc-pool.h \
    hard-reg-set.h $(BASIC_BLOCK_H) $(DF_H) $(BITMAP_H) sbitmap.h $(TIMEVAR_H) \
Index: gcc/convert.c
===================================================================
--- gcc/convert.c	(revision 163280)
+++ gcc/convert.c	(working copy)
@@ -60,7 +60,19 @@  convert_to_pointer (tree type, tree expr
 	addr_space_t to_as = TYPE_ADDR_SPACE (TREE_TYPE (type));
 	addr_space_t from_as = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (expr)));
 
-	if (to_as == from_as)
+        if (TREE_CODE (TREE_TYPE (TREE_TYPE (expr))) == VECTOR_TYPE)
+          {
+            /* Check whether implicit vector conversion preserves
+               alignment.  */
+            if (TREE_CODE (type) == POINTER_TYPE
+                && TYPE_ALIGN (TREE_TYPE (type))
+                   > MAX (TYPE_ALIGN (TREE_TYPE (TREE_TYPE (expr))),
+                          get_pointer_alignment (expr, 0)))
+              warning_at (loc, OPT_Wmisaligned, 
+                          "Possible vector-pointer missaligned cast");
+          }
+
+        if (to_as == from_as)
 	  return fold_build1_loc (loc, NOP_EXPR, type, expr);
 	else
 	  return fold_build1_loc (loc, ADDR_SPACE_CONVERT_EXPR, type, expr);