| Message ID | 4FD602CB.2020908@arm.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jun 11, 2012 at 4:38 PM, Richard Earnshaw <rearnsha@arm.com> wrote: > On 11/06/12 15:17, Richard Guenther wrote: >> On Mon, Jun 11, 2012 at 3:16 PM, Richard Earnshaw <rearnsha@arm.com> wrote: >>> The ARM ABI states that vectors larger than 64 bits in size still have >>> 64-bit alignment; never-the-less, the HW supports alignment hints of up >>> to 128-bits in some cases and will trap in a vector has an alignment >>> that less than the hint. GCC currently hard-codes larger vectors to be >>> aligned by the size of the vector, which means that 128-bit vectors are >>> marked as being 128-bit aligned. >>> >>> The ARM ABI unfortunately does not support generating such alignment for >>> parameters passed by value and this can lead to traps at run time. It >>> seems that the best way to solve this problem is to allow the back-end >>> to set an upper limit on the alignment permitted for a vector. >>> >>> I've implemented this as a separate hook, rather than using the existing >>> hooks because there's a strong likelihood of breaking some existing ABIs >>> if I did it another way. >>> >>> There are a couple of tests that will need some re-working before this >>> can be committed to deal with the fall-out of making this change; I'll >>> prepare those changes if this patch is deemed generally acceptable. >> >> Hm. Where would you use that target hook? >> > > Doh! > > It was supposed to be in the patch set... :-( > > in layout_type(), where the alignment of a vector is forced to the size > of the vector. Ok. + /* Naturally align vectors, but let the target set an upper + limit. This prevents ABI changes depending on whether or + not native vector modes are supported. */ + TYPE_ALIGN (type) + = targetm.vector_alignment (type, tree_low_cst (TYPE_SIZE (type), 0)); The type argument or the size argument looks redundant. Note that we still can have such vector "properly" aligned, thus the vectorizer would need to use build_aligned_type also if it knows the type is aligned, not only when it thinks it is misaligned. You basically change the alignment of the "default" vector type. Richard. > R.
On 11/06/12 15:53, Richard Guenther wrote: > On Mon, Jun 11, 2012 at 4:38 PM, Richard Earnshaw <rearnsha@arm.com> wrote: >> On 11/06/12 15:17, Richard Guenther wrote: >>> On Mon, Jun 11, 2012 at 3:16 PM, Richard Earnshaw <rearnsha@arm.com> wrote: >>>> The ARM ABI states that vectors larger than 64 bits in size still have >>>> 64-bit alignment; never-the-less, the HW supports alignment hints of up >>>> to 128-bits in some cases and will trap in a vector has an alignment >>>> that less than the hint. GCC currently hard-codes larger vectors to be >>>> aligned by the size of the vector, which means that 128-bit vectors are >>>> marked as being 128-bit aligned. >>>> >>>> The ARM ABI unfortunately does not support generating such alignment for >>>> parameters passed by value and this can lead to traps at run time. It >>>> seems that the best way to solve this problem is to allow the back-end >>>> to set an upper limit on the alignment permitted for a vector. >>>> >>>> I've implemented this as a separate hook, rather than using the existing >>>> hooks because there's a strong likelihood of breaking some existing ABIs >>>> if I did it another way. >>>> >>>> There are a couple of tests that will need some re-working before this >>>> can be committed to deal with the fall-out of making this change; I'll >>>> prepare those changes if this patch is deemed generally acceptable. >>> >>> Hm. Where would you use that target hook? >>> >> >> Doh! >> >> It was supposed to be in the patch set... :-( >> >> in layout_type(), where the alignment of a vector is forced to the size >> of the vector. > > Ok. > > + /* Naturally align vectors, but let the target set an upper > + limit. This prevents ABI changes depending on whether or > + not native vector modes are supported. */ > + TYPE_ALIGN (type) > + = targetm.vector_alignment (type, tree_low_cst (TYPE_SIZE (type), 0)); > > The type argument or the size argument looks redundant. Technically, yes, we could get rid of "tree_low_cst (TYPE_SIZE (type)" and calculate it inside the alignment function if it was needed. However, it seemed likely that most targets would need that number one way or another, such that passing it would be helpful. > Note that we still > can have such vector "properly" aligned, thus the vectorizer would need to > use build_aligned_type also if it knows the type is aligned, not only when > it thinks it is misaligned. You basically change the alignment of the "default" > vector type. > I'm not sure I follow... R.
On Mon, Jun 11, 2012 at 5:25 PM, Richard Earnshaw <rearnsha@arm.com> wrote: > On 11/06/12 15:53, Richard Guenther wrote: >> On Mon, Jun 11, 2012 at 4:38 PM, Richard Earnshaw <rearnsha@arm.com> wrote: >>> On 11/06/12 15:17, Richard Guenther wrote: >>>> On Mon, Jun 11, 2012 at 3:16 PM, Richard Earnshaw <rearnsha@arm.com> wrote: >>>>> The ARM ABI states that vectors larger than 64 bits in size still have >>>>> 64-bit alignment; never-the-less, the HW supports alignment hints of up >>>>> to 128-bits in some cases and will trap in a vector has an alignment >>>>> that less than the hint. GCC currently hard-codes larger vectors to be >>>>> aligned by the size of the vector, which means that 128-bit vectors are >>>>> marked as being 128-bit aligned. >>>>> >>>>> The ARM ABI unfortunately does not support generating such alignment for >>>>> parameters passed by value and this can lead to traps at run time. It >>>>> seems that the best way to solve this problem is to allow the back-end >>>>> to set an upper limit on the alignment permitted for a vector. >>>>> >>>>> I've implemented this as a separate hook, rather than using the existing >>>>> hooks because there's a strong likelihood of breaking some existing ABIs >>>>> if I did it another way. >>>>> >>>>> There are a couple of tests that will need some re-working before this >>>>> can be committed to deal with the fall-out of making this change; I'll >>>>> prepare those changes if this patch is deemed generally acceptable. >>>> >>>> Hm. Where would you use that target hook? >>>> >>> >>> Doh! >>> >>> It was supposed to be in the patch set... :-( >>> >>> in layout_type(), where the alignment of a vector is forced to the size >>> of the vector. >> >> Ok. >> >> + /* Naturally align vectors, but let the target set an upper >> + limit. This prevents ABI changes depending on whether or >> + not native vector modes are supported. */ >> + TYPE_ALIGN (type) >> + = targetm.vector_alignment (type, tree_low_cst (TYPE_SIZE (type), 0)); >> >> The type argument or the size argument looks redundant. > > Technically, yes, we could get rid of "tree_low_cst (TYPE_SIZE (type)" > and calculate it inside the alignment function if it was needed. > However, it seemed likely that most targets would need that number one > way or another, such that passing it would be helpful. Well, you don't need it in stor-layout and targets might think the value may be completely unrelated to the type ... >> Note that we still >> can have such vector "properly" aligned, thus the vectorizer would need to >> use build_aligned_type also if it knows the type is aligned, not only when >> it thinks it is misaligned. You basically change the alignment of the "default" >> vector type. >> > > I'm not sure I follow... I say that a large vector may be still aligned, so the vectorizer when creating vector memory references has to use a non-default aligned vector type when the vector is aligned. It won't do that at the moment. Richard. > R. > > >
On Fri, Jul 27, 2012 at 5:24 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote: > Richard Guenther wrote: >> On Mon, Jun 11, 2012 at 5:25 PM, Richard Earnshaw <rearnsha@arm.com> wrote: >> > On 11/06/12 15:53, Richard Guenther wrote: >> >> The type argument or the size argument looks redundant. >> > >> > Technically, yes, we could get rid of "tree_low_cst (TYPE_SIZE (type)" >> > and calculate it inside the alignment function if it was needed. >> > However, it seemed likely that most targets would need that number one >> > way or another, such that passing it would be helpful. >> >> Well, you don't need it in stor-layout and targets might think the value >> may be completely unrelated to the type ... >> >> >> Note that we still can have such vector "properly" aligned, thus the >> >> vectorizer would need to use build_aligned_type also if it knows the >> >> type is aligned, not only when thinks it is misaligned. You basically >> >> change the alignment of the "default" vector type. >> > >> > I'm not sure I follow... >> >> I say that a large vector may be still aligned, so the vectorizer when >> creating vector memory references has to use a non-default aligned vector >> type when the vector is aligned. It won't do that at the moment. > > Richard (Earnshaw) has asked me to take over working on this patch now. > > I've now made the change requested above and removed the size argument. > The target is now simply asked to return the required alignment for the > given vector type. I've also added a check for the case where the > target provides both an alignment and a mode for a vector type, but > the mode actually requires bigger alignment than the type. This is > simply rejected (the target can fix this by reporting a different > type alignment or changing the mode alignment). > > I've not made any attempts to have the vectorizer register larger > alignments than the one returned by the target hook. It's not > clear to me when this would be useful (at least on ARM) ... > > I've also run the testsuite, and this actually uncovered to bugs in > the vectorizer where it made an implicit assumption that vector types > must always be naturally aligned: > > - In vect_update_misalignment_for_peel, the code used the vector size > instead of the required alignment in order to bound misalignment > values -- leading to a misalignment value bigger than the underlying > alignment requirement of the vector type, causing an ICE later on > > - In vect_do_peeling_for_loop_bound, the code divided the vector type > alignment by the number of elements in order to arrive at the element > size ... this returns a wrong value if the alignment is less than the > vector size, causing incorrect code to be generated > > (This routine also had some confusion between size and alignment in > comments and variable names, which I've fixed as well.) > > Finally, two test cases still failed spuriously: > > - gcc.dg/align-2.c actually checked that vector types are naturally > aligned > > - gcc.dg/vect/slp-25.c checked that we needed to perform peeling for > alignment, which we actually don't need any more if vector types > have a lesser alignment requirement in the first place > > I've added a new effective target flag to check whether the target > requires natural alignment for vector types, and disabled those two > tests if it doesn't. > > With those changes, I've completed testing with no regressions on > arm-linux-gnueabi. > > OK for mainline? Ok. Please add to the documentation that the default vector alignment has to be a power-of-two multiple of the default vector element alignment. You probably want to double-check vector_alignment_reachable_p as well which checks whether vector alignment can be reached by peeling off scalar iterations. Thanks, Richard. > Bye, > Ulrich > > > ChangeLog: > > * target.def (vector_alignment): New target hook. > * doc/tm.texi.in (TARGET_VECTOR_ALIGNMENT): Document new hook. > * doc/tm.texi: Regenerate. > * targhooks.c (default_vector_alignment): New function. > * targhooks.h (default_vector_alignment): Add prototype. > * stor-layout.c (layout_type): Use targetm.vector_alignment. > * config/arm/arm.c (arm_vector_alignment): New function. > (TARGET_VECTOR_ALIGNMENT): Define. > > * tree-vect-data-refs.c (vect_update_misalignment_for_peel): Use > vector type alignment instead of size. > * tree-vect-loop-manip.c (vect_do_peeling_for_loop_bound): Use > element type size directly instead of computing it from alignment. > Fix variable naming and comment. > > testsuite/ChangeLog: > > * lib/target-supports.exp > (check_effective_target_vect_natural_alignment): New function. > * gcc.dg/align-2.c: Only run on targets with natural alignment > of vector types. > * gcc.dg/vect/slp-25.c: Adjust tests for targets without natural > alignment of vector types. > > > Index: gcc/target.def > =================================================================== > *** gcc/target.def (revision 189809) > --- gcc/target.def (working copy) > *************** DEFHOOK > *** 1659,1664 **** > --- 1659,1672 ---- > bool, (enum machine_mode mode), > hook_bool_mode_false) > > + DEFHOOK > + (vector_alignment, > + "This hook can be used to define the alignment for a vector of type\n\ > + @var{type}, in order to comply with a platform ABI. The default is to\n\ > + require natural alignment for vector types.", > + HOST_WIDE_INT, (const_tree type), > + default_vector_alignment) > + > /* True if we should try to use a scalar mode to represent an array, > overriding the usual MAX_FIXED_MODE limit. */ > DEFHOOK > Index: gcc/doc/tm.texi > =================================================================== > *** gcc/doc/tm.texi (revision 189809) > --- gcc/doc/tm.texi (working copy) > *************** make it all fit in fewer cache lines. > *** 1107,1112 **** > --- 1107,1118 ---- > If the value of this macro has a type, it should be an unsigned type. > @end defmac > > + @deftypefn {Target Hook} HOST_WIDE_INT TARGET_VECTOR_ALIGNMENT (const_tree @var{type}) > + This hook can be used to define the alignment for a vector of type > + @var{type}, in order to comply with a platform ABI. The default is to > + require natural alignment for vector types. > + @end deftypefn > + > @defmac STACK_SLOT_ALIGNMENT (@var{type}, @var{mode}, @var{basic-align}) > If defined, a C expression to compute the alignment for stack slot. > @var{type} is the data type, @var{mode} is the widest mode available, > Index: gcc/doc/tm.texi.in > =================================================================== > *** gcc/doc/tm.texi.in (revision 189809) > --- gcc/doc/tm.texi.in (working copy) > *************** make it all fit in fewer cache lines. > *** 1091,1096 **** > --- 1091,1098 ---- > If the value of this macro has a type, it should be an unsigned type. > @end defmac > > + @hook TARGET_VECTOR_ALIGNMENT > + > @defmac STACK_SLOT_ALIGNMENT (@var{type}, @var{mode}, @var{basic-align}) > If defined, a C expression to compute the alignment for stack slot. > @var{type} is the data type, @var{mode} is the widest mode available, > Index: gcc/targhooks.c > =================================================================== > *** gcc/targhooks.c (revision 189809) > --- gcc/targhooks.c (working copy) > *************** tree default_mangle_decl_assembler_name > *** 945,950 **** > --- 945,957 ---- > return id; > } > > + /* Default to natural alignment for vector types. */ > + HOST_WIDE_INT > + default_vector_alignment (const_tree type) > + { > + return tree_low_cst (TYPE_SIZE (type), 0); > + } > + > bool > default_builtin_vector_alignment_reachable (const_tree type, bool is_packed) > { > Index: gcc/targhooks.h > =================================================================== > *** gcc/targhooks.h (revision 189809) > --- gcc/targhooks.h (working copy) > *************** extern int default_builtin_vectorization > *** 83,88 **** > --- 83,90 ---- > > extern tree default_builtin_reciprocal (unsigned int, bool, bool); > > + extern HOST_WIDE_INT default_vector_alignment (const_tree); > + > extern bool default_builtin_vector_alignment_reachable (const_tree, bool); > extern bool > default_builtin_support_vector_misalignment (enum machine_mode mode, > Index: gcc/stor-layout.c > =================================================================== > *** gcc/stor-layout.c (revision 189809) > --- gcc/stor-layout.c (working copy) > *************** layout_type (tree type) > *** 2131,2139 **** > TYPE_SIZE (type) = int_const_binop (MULT_EXPR, TYPE_SIZE (innertype), > bitsize_int (nunits)); > > ! /* Always naturally align vectors. This prevents ABI changes > ! depending on whether or not native vector modes are supported. */ > ! TYPE_ALIGN (type) = tree_low_cst (TYPE_SIZE (type), 0); > break; > } > > --- 2131,2147 ---- > TYPE_SIZE (type) = int_const_binop (MULT_EXPR, TYPE_SIZE (innertype), > bitsize_int (nunits)); > > ! /* For vector types, we do not default to the mode's alignment. > ! Instead, query a target hook, defaulting to natural alignment. > ! This prevents ABI changes depending on whether or not native > ! vector modes are supported. */ > ! TYPE_ALIGN (type) = targetm.vector_alignment (type); > ! > ! /* However, if the underlying mode requires a bigger alignment than > ! what the target hook provides, we cannot use the mode. For now, > ! simply reject that case. */ > ! gcc_assert (TYPE_ALIGN (type) > ! >= GET_MODE_ALIGNMENT (TYPE_MODE (type))); > break; > } > > Index: gcc/config/arm/arm.c > =================================================================== > *** gcc/config/arm/arm.c (revision 189809) > --- gcc/config/arm/arm.c (working copy) > *************** static bool arm_array_mode_supported_p ( > *** 254,259 **** > --- 254,260 ---- > unsigned HOST_WIDE_INT); > static enum machine_mode arm_preferred_simd_mode (enum machine_mode); > static bool arm_class_likely_spilled_p (reg_class_t); > + static HOST_WIDE_INT arm_vector_alignment (const_tree type); > static bool arm_vector_alignment_reachable (const_tree type, bool is_packed); > static bool arm_builtin_support_vector_misalignment (enum machine_mode mode, > const_tree type, > *************** static const struct attribute_spec arm_a > *** 602,607 **** > --- 603,611 ---- > #undef TARGET_CLASS_LIKELY_SPILLED_P > #define TARGET_CLASS_LIKELY_SPILLED_P arm_class_likely_spilled_p > > + #undef TARGET_VECTOR_ALIGNMENT > + #define TARGET_VECTOR_ALIGNMENT arm_vector_alignment > + > #undef TARGET_VECTORIZE_VECTOR_ALIGNMENT_REACHABLE > #define TARGET_VECTORIZE_VECTOR_ALIGNMENT_REACHABLE \ > arm_vector_alignment_reachable > *************** arm_have_conditional_execution (void) > *** 25051,25056 **** > --- 25055,25072 ---- > return !TARGET_THUMB1; > } > > + /* The AAPCS sets the maximum alignment of a vector to 64 bits. */ > + static HOST_WIDE_INT > + arm_vector_alignment (const_tree type) > + { > + HOST_WIDE_INT align = tree_low_cst (TYPE_SIZE (type), 0); > + > + if (TARGET_AAPCS_BASED) > + align = MIN (align, 64); > + > + return align; > + } > + > static unsigned int > arm_autovectorize_vector_sizes (void) > { > Index: gcc/tree-vect-data-refs.c > =================================================================== > *** gcc/tree-vect-data-refs.c (revision 189809) > --- gcc/tree-vect-data-refs.c (working copy) > *************** vect_update_misalignment_for_peel (struc > *** 1059,1065 **** > int misal = DR_MISALIGNMENT (dr); > tree vectype = STMT_VINFO_VECTYPE (stmt_info); > misal += negative ? -npeel * dr_size : npeel * dr_size; > ! misal &= GET_MODE_SIZE (TYPE_MODE (vectype)) - 1; > SET_DR_MISALIGNMENT (dr, misal); > return; > } > --- 1059,1065 ---- > int misal = DR_MISALIGNMENT (dr); > tree vectype = STMT_VINFO_VECTYPE (stmt_info); > misal += negative ? -npeel * dr_size : npeel * dr_size; > ! misal &= (TYPE_ALIGN (vectype) / BITS_PER_UNIT) - 1; > SET_DR_MISALIGNMENT (dr, misal); > return; > } > Index: gcc/tree-vect-loop-manip.c > =================================================================== > *** gcc/tree-vect-loop-manip.c (revision 189809) > --- gcc/tree-vect-loop-manip.c (working copy) > *************** vect_do_peeling_for_loop_bound (loop_vec > *** 1937,1943 **** > If the misalignment of DR is known at compile time: > addr_mis = int mis = DR_MISALIGNMENT (dr); > Else, compute address misalignment in bytes: > ! addr_mis = addr & (vectype_size - 1) > > prolog_niters = min (LOOP_NITERS, ((VF - addr_mis/elem_size)&(VF-1))/step) > > --- 1937,1943 ---- > If the misalignment of DR is known at compile time: > addr_mis = int mis = DR_MISALIGNMENT (dr); > Else, compute address misalignment in bytes: > ! addr_mis = addr & (vectype_align - 1) > > prolog_niters = min (LOOP_NITERS, ((VF - addr_mis/elem_size)&(VF-1))/step) > > *************** vect_gen_niters_for_prolog_loop (loop_ve > *** 1991,1999 **** > tree start_addr = vect_create_addr_base_for_vector_ref (dr_stmt, > &new_stmts, offset, loop); > tree type = unsigned_type_for (TREE_TYPE (start_addr)); > ! tree vectype_size_minus_1 = build_int_cst (type, vectype_align - 1); > ! tree elem_size_log = > ! build_int_cst (type, exact_log2 (vectype_align/nelements)); > tree nelements_minus_1 = build_int_cst (type, nelements - 1); > tree nelements_tree = build_int_cst (type, nelements); > tree byte_misalign; > --- 1991,2000 ---- > tree start_addr = vect_create_addr_base_for_vector_ref (dr_stmt, > &new_stmts, offset, loop); > tree type = unsigned_type_for (TREE_TYPE (start_addr)); > ! tree vectype_align_minus_1 = build_int_cst (type, vectype_align - 1); > ! HOST_WIDE_INT elem_size = > ! int_cst_value (TYPE_SIZE_UNIT (TREE_TYPE (vectype))); > ! tree elem_size_log = build_int_cst (type, exact_log2 (elem_size)); > tree nelements_minus_1 = build_int_cst (type, nelements - 1); > tree nelements_tree = build_int_cst (type, nelements); > tree byte_misalign; > *************** vect_gen_niters_for_prolog_loop (loop_ve > *** 2002,2011 **** > new_bb = gsi_insert_seq_on_edge_immediate (pe, new_stmts); > gcc_assert (!new_bb); > > ! /* Create: byte_misalign = addr & (vectype_size - 1) */ > byte_misalign = > fold_build2 (BIT_AND_EXPR, type, fold_convert (type, start_addr), > ! vectype_size_minus_1); > > /* Create: elem_misalign = byte_misalign / element_size */ > elem_misalign = > --- 2003,2012 ---- > new_bb = gsi_insert_seq_on_edge_immediate (pe, new_stmts); > gcc_assert (!new_bb); > > ! /* Create: byte_misalign = addr & (vectype_align - 1) */ > byte_misalign = > fold_build2 (BIT_AND_EXPR, type, fold_convert (type, start_addr), > ! vectype_align_minus_1); > > /* Create: elem_misalign = byte_misalign / element_size */ > elem_misalign = > Index: gcc/testsuite/lib/target-supports.exp > =================================================================== > *** gcc/testsuite/lib/target-supports.exp (revision 189809) > --- gcc/testsuite/lib/target-supports.exp (working copy) > *************** proc check_effective_target_natural_alig > *** 3379,3384 **** > --- 3379,3404 ---- > return $et_natural_alignment_64_saved > } > > + # Return 1 if all vector types are naturally aligned (aligned to their > + # type-size), 0 otherwise. > + # > + # This won't change for different subtargets so cache the result. > + > + proc check_effective_target_vect_natural_alignment { } { > + global et_vect_natural_alignment > + > + if [info exists et_vect_natural_alignment_saved] { > + verbose "check_effective_target_vect_natural_alignment: using cached result" 2 > + } else { > + set et_vect_natural_alignment_saved 1 > + if { [check_effective_target_arm_eabi] } { > + set et_vect_natural_alignment_saved 0 > + } > + } > + verbose "check_effective_target_vect_natural_alignment: returning $et_vect_natural_alignment_saved" 2 > + return $et_vect_natural_alignment_saved > + } > + > # Return 1 if vector alignment (for types of size 32 bit or less) is reachable, 0 otherwise. > # > # This won't change for different subtargets so cache the result. > Index: gcc/testsuite/gcc.dg/align-2.c > =================================================================== > *** gcc/testsuite/gcc.dg/align-2.c (revision 189809) > --- gcc/testsuite/gcc.dg/align-2.c (working copy) > *************** > *** 1,5 **** > /* PR 17962 */ > ! /* { dg-do compile } */ > /* { dg-options "" } */ > > typedef float v4 __attribute__((vector_size(sizeof(float)*4))); > --- 1,5 ---- > /* PR 17962 */ > ! /* { dg-do compile { target vect_natural_alignment } } */ > /* { dg-options "" } */ > > typedef float v4 __attribute__((vector_size(sizeof(float)*4))); > Index: gcc/testsuite/gcc.dg/vect/slp-25.c > =================================================================== > *** gcc/testsuite/gcc.dg/vect/slp-25.c (revision 189809) > --- gcc/testsuite/gcc.dg/vect/slp-25.c (working copy) > *************** int main (void) > *** 56,60 **** > > /* { dg-final { scan-tree-dump-times "vectorized 2 loops" 1 "vect" } } */ > /* { dg-final { scan-tree-dump-times "Vectorizing an unaligned access" 0 "vect" } } */ > ! /* { dg-final { scan-tree-dump-times "Alignment of access forced using peeling" 2 "vect" { xfail { vect_no_align } } } } */ > /* { dg-final { cleanup-tree-dump "vect" } } */ > --- 56,60 ---- > > /* { dg-final { scan-tree-dump-times "vectorized 2 loops" 1 "vect" } } */ > /* { dg-final { scan-tree-dump-times "Vectorizing an unaligned access" 0 "vect" } } */ > ! /* { dg-final { scan-tree-dump-times "Alignment of access forced using peeling" 2 "vect" { xfail { vect_no_align || { ! vect_natural_alignment } } } } } */ > /* { dg-final { cleanup-tree-dump "vect" } } */ > > -- > Dr. Ulrich Weigand > GNU Toolchain for Linux on System z and Cell BE > Ulrich.Weigand@de.ibm.com >
On Tue, Aug 7, 2012 at 4:56 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote: > Richard Guenther wrote: >> On Fri, Jul 27, 2012 at 5:24 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote: >> > ChangeLog: >> > >> > * target.def (vector_alignment): New target hook. >> > * doc/tm.texi.in (TARGET_VECTOR_ALIGNMENT): Document new hook. >> > * doc/tm.texi: Regenerate. >> > * targhooks.c (default_vector_alignment): New function. >> > * targhooks.h (default_vector_alignment): Add prototype. >> > * stor-layout.c (layout_type): Use targetm.vector_alignment. >> > * config/arm/arm.c (arm_vector_alignment): New function. >> > (TARGET_VECTOR_ALIGNMENT): Define. >> > >> > * tree-vect-data-refs.c (vect_update_misalignment_for_peel): Use >> > vector type alignment instead of size. >> > * tree-vect-loop-manip.c (vect_do_peeling_for_loop_bound): Use >> > element type size directly instead of computing it from alignment. >> > Fix variable naming and comment. >> > >> > testsuite/ChangeLog: >> > >> > * lib/target-supports.exp >> > (check_effective_target_vect_natural_alignment): New function. >> > * gcc.dg/align-2.c: Only run on targets with natural alignment >> > of vector types. >> > * gcc.dg/vect/slp-25.c: Adjust tests for targets without natural >> > alignment of vector types. > > >> > OK for mainline? >> >> Ok. > > Would it be OK to backport this to 4.7 and possibly 4.6? > > This patch represents a change in the ABI on ARM (the alignment could > potentially affect struct member offsets, for example), which could > conceivably cause incompatibilities with code compiled with older > versions of GCC. We had originally decided we nevertheless want to > implement this change on ARM, because: > > - GCC is now in compliance with the platform ABI document > - the old code could actually be buggy since GCC *assumed* 16-byte > alignment that wasn't actually guaranteed > - code actually affected by this change ought to be rare (code using > NEON vector types is rare in the first place, code using *structure > members* of such types is even rarer, and code using such structures > in cross-module APIs seems to be extremely rare) > > Nevertheless, given we do want to make this change, it would be best > to roll it out as quickly as possible, to minimize the time period > where people might use old (not yet fixed) compilers to generate > non-ABI-compliant binaries. Thus, we think it would be good for > the change to be applied to all still open release branches as well. > > (Note that while the patch contains changes to common code, those > should be no-ops for all targets that do not implement the new hook.) I'll defer the decision to the target maintainers. But please double-check for any changes in the vectorizer parts when backporting to 4.6. Thanks, Richard. > Bye, > Ulrich > > -- > Dr. Ulrich Weigand > GNU Toolchain for Linux on System z and Cell BE > Ulrich.Weigand@de.ibm.com >
On 07/08/12 16:04, Richard Guenther wrote: > On Tue, Aug 7, 2012 at 4:56 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote: >> Richard Guenther wrote: >>> On Fri, Jul 27, 2012 at 5:24 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote: >>>> ChangeLog: >>>> >>>> * target.def (vector_alignment): New target hook. >>>> * doc/tm.texi.in (TARGET_VECTOR_ALIGNMENT): Document new hook. >>>> * doc/tm.texi: Regenerate. >>>> * targhooks.c (default_vector_alignment): New function. >>>> * targhooks.h (default_vector_alignment): Add prototype. >>>> * stor-layout.c (layout_type): Use targetm.vector_alignment. >>>> * config/arm/arm.c (arm_vector_alignment): New function. >>>> (TARGET_VECTOR_ALIGNMENT): Define. >>>> >>>> * tree-vect-data-refs.c (vect_update_misalignment_for_peel): Use >>>> vector type alignment instead of size. >>>> * tree-vect-loop-manip.c (vect_do_peeling_for_loop_bound): Use >>>> element type size directly instead of computing it from alignment. >>>> Fix variable naming and comment. >>>> >>>> testsuite/ChangeLog: >>>> >>>> * lib/target-supports.exp >>>> (check_effective_target_vect_natural_alignment): New function. >>>> * gcc.dg/align-2.c: Only run on targets with natural alignment >>>> of vector types. >>>> * gcc.dg/vect/slp-25.c: Adjust tests for targets without natural >>>> alignment of vector types. >> >> >>>> OK for mainline? >>> >>> Ok. >> >> Would it be OK to backport this to 4.7 and possibly 4.6? >> >> This patch represents a change in the ABI on ARM (the alignment could >> potentially affect struct member offsets, for example), which could >> conceivably cause incompatibilities with code compiled with older >> versions of GCC. We had originally decided we nevertheless want to >> implement this change on ARM, because: >> >> - GCC is now in compliance with the platform ABI document >> - the old code could actually be buggy since GCC *assumed* 16-byte >> alignment that wasn't actually guaranteed >> - code actually affected by this change ought to be rare (code using >> NEON vector types is rare in the first place, code using *structure >> members* of such types is even rarer, and code using such structures >> in cross-module APIs seems to be extremely rare) >> >> Nevertheless, given we do want to make this change, it would be best >> to roll it out as quickly as possible, to minimize the time period >> where people might use old (not yet fixed) compilers to generate >> non-ABI-compliant binaries. Thus, we think it would be good for >> the change to be applied to all still open release branches as well. >> >> (Note that while the patch contains changes to common code, those >> should be no-ops for all targets that do not implement the new hook.) > > I'll defer the decision to the target maintainers. But please double-check > for any changes in the vectorizer parts when backporting to 4.6. > My inclination is to back-port this to all maintained branches (for consistency). However, it does need to be release-noted. R.
>> (Note that while the patch contains changes to common code, those >> should be no-ops for all targets that do not implement the new hook.) > > I'll defer the decision to the target maintainers. I'd rather have this consistent across all maintained release branches today than to leave this for an ABI break in 4.8 timeframe. In addition I'd like this documented in changes.html for each of the release branches. Thanks, Ramana
--- stor-layout.c (revision 188348) +++ stor-layout.c (local) @@ -2131,9 +2131,11 @@ layout_type (tree type) TYPE_SIZE (type) = int_const_binop (MULT_EXPR, TYPE_SIZE (innertype), bitsize_int (nunits)); - /* Always naturally align vectors. This prevents ABI changes - depending on whether or not native vector modes are supported. */ - TYPE_ALIGN (type) = tree_low_cst (TYPE_SIZE (type), 0); + /* Naturally align vectors, but let the target set an upper + limit. This prevents ABI changes depending on whether or + not native vector modes are supported. */ + TYPE_ALIGN (type) + = targetm.vector_alignment (type, tree_low_cst (TYPE_SIZE (type), 0)); break; }