Re: [PATCH][GCC] Make DR_TARGET_ALIGNMENT compile time variable
On Wed, Nov 14, 2018 at 12:43 PM Iain Sandoe wrote: > > > > On 13 Nov 2018, at 19:31, Dominique d'Humières wrote: > > > > Revision r266072 breaks bootstrap on darwin: > > > > In file included from ../../work/gcc/coretypes.h:430, > > from ../../work/gcc/tree-vect-data-refs.c:24: > > ../../work/gcc/poly-int.h: In instantiation of 'typename if_nonpoly > bool>::type maybe_lt(const Ca&, const poly_int_pod&) [with unsigned > > int N = 1; Ca = int; Cb = long long unsigned int; typename if_nonpoly > bool>::type = bool]': > > ../../work/gcc/tree-vect-data-refs.c:6338:13: required from here > > ../../work/gcc/poly-int.h:1384:12: error: comparison of integer expressions > > of different signedness: 'const int' and 'const long long unsigned int' > > [-Werror=sign-compare] > > 1384 | return a < b.coeffs[0]; > > | ~~^~~ > > cc1plus: all warnings being treated as errors > > It’s not immediately obvious why there aren’t more folks with bootstrap fails > - grep says that MAX_OFILE_ALIGNMENT is defined in a similar way for many > targets. > > I’m testing this (almost obvious) fix: > (nit: the else case line also needs wrapping [81chars]) : > > diff --git a/gcc/tree-vect-data-refs.c b/gcc/tree-vect-data-refs.c > index 1cc0320..deb7121 100644 > --- a/gcc/tree-vect-data-refs.c > +++ b/gcc/tree-vect-data-refs.c > @@ -6335,7 +6335,8 @@ vect_can_force_dr_alignment_p (const_tree decl, > poly_uint64 alignment) > return false; > >if (TREE_STATIC (decl)) > -return (known_le (alignment, MAX_OFILE_ALIGNMENT)); > +return (known_le (alignment, > + (unsigned HOST_WIDE_INT) MAX_OFILE_ALIGNMENT)); >else > return (known_le (alignment, (unsigned HOST_WIDE_INT) > MAX_STACK_ALIGNMENT)); > } > > OK? OK. > Iain >
Re: [PATCH][GCC] Make DR_TARGET_ALIGNMENT compile time variable
> On 13 Nov 2018, at 19:31, Dominique d'Humières wrote: > > Revision r266072 breaks bootstrap on darwin: > > In file included from ../../work/gcc/coretypes.h:430, > from ../../work/gcc/tree-vect-data-refs.c:24: > ../../work/gcc/poly-int.h: In instantiation of 'typename if_nonpoly bool>::type maybe_lt(const Ca&, const poly_int_pod&) [with unsigned > int N = 1; Ca = int; Cb = long long unsigned int; typename if_nonpoly bool>::type = bool]': > ../../work/gcc/tree-vect-data-refs.c:6338:13: required from here > ../../work/gcc/poly-int.h:1384:12: error: comparison of integer expressions > of different signedness: 'const int' and 'const long long unsigned int' > [-Werror=sign-compare] > 1384 | return a < b.coeffs[0]; > | ~~^~~ > cc1plus: all warnings being treated as errors It’s not immediately obvious why there aren’t more folks with bootstrap fails - grep says that MAX_OFILE_ALIGNMENT is defined in a similar way for many targets. I’m testing this (almost obvious) fix: (nit: the else case line also needs wrapping [81chars]) : diff --git a/gcc/tree-vect-data-refs.c b/gcc/tree-vect-data-refs.c index 1cc0320..deb7121 100644 --- a/gcc/tree-vect-data-refs.c +++ b/gcc/tree-vect-data-refs.c @@ -6335,7 +6335,8 @@ vect_can_force_dr_alignment_p (const_tree decl, poly_uint64 alignment) return false; if (TREE_STATIC (decl)) -return (known_le (alignment, MAX_OFILE_ALIGNMENT)); +return (known_le (alignment, + (unsigned HOST_WIDE_INT) MAX_OFILE_ALIGNMENT)); else return (known_le (alignment, (unsigned HOST_WIDE_INT) MAX_STACK_ALIGNMENT)); } OK? Iain
Re: [PATCH][GCC] Make DR_TARGET_ALIGNMENT compile time variable
Revision r266072 breaks bootstrap on darwin: In file included from ../../work/gcc/coretypes.h:430, from ../../work/gcc/tree-vect-data-refs.c:24: ../../work/gcc/poly-int.h: In instantiation of 'typename if_nonpoly::type maybe_lt(const Ca&, const poly_int_pod&) [with unsigned int N = 1; Ca = int; Cb = long long unsigned int; typename if_nonpoly::type = bool]': ../../work/gcc/tree-vect-data-refs.c:6338:13: required from here ../../work/gcc/poly-int.h:1384:12: error: comparison of integer expressions of different signedness: 'const int' and 'const long long unsigned int' [-Werror=sign-compare] 1384 | return a < b.coeffs[0]; | ~~^~~ cc1plus: all warnings being treated as errors TIA Dominique
Re: [PATCH][GCC] Make DR_TARGET_ALIGNMENT compile time variable
On Fri, Nov 9, 2018 at 5:08 PM Andre Vieira (lists) wrote: > > On 05/11/18 12:41, Richard Biener wrote: > > On Mon, Nov 5, 2018 at 1:07 PM Andre Vieira (lists) > > wrote: > >> > >> > >> Hi, > >> > Hi, > > Thank you for the quick response! See inline responses below. > > >> This patch enables targets to describe DR_TARGET_ALIGNMENT as a > >> compile-time variable. It does so by turning the variable into a > >> 'poly_uint64'. This should not affect the current code-generation for > >> any target. > >> > >> We hope to use this in the near future for SVE using the > >> current_vector_size as the preferred target alignment for vectors. In > >> fact I have a patch to do just this, but I am still trying to figure out > >> whether and when it is beneficial to peel for alignment with a runtime > >> misalignment. > > > > In fact in most cases I have seen the issue is that it's not visible whether > > peeling will be able to align _all_ references and doing peeling only to > > align some is hardly beneficial. To improve things the vectorizer would > > have to version the loop for the case where peeling can reach alignment > > for a group of DRs and then vectorize one copy with peeling for alignment > > and one copy with unaligned accesses. > > > So I have seen code being peeled for alignment even when it only knows > how to align one of a group (only checked 2 or 3) and I think this may > still be beneficial in some cases. I am more worried about cases where > the number of iterations isn't enough to justify the initial peeling > cost or when the loop isn't memory bound, i.e. very arithmetic heavy > loops. This is a bigger vectorization problem though, that would > require some kind of cost-model. > > > > >> The patch I am working on will change the behavior of > >> auto-vectorization for SVE when building vector-length agnostic code for > >> targets that benefit from aligned vector loads/stores. The patch will > >> result in the generation of a runtime computation of misalignment and > >> the construction of a corresponding mask for the first iteration of the > >> loop. > >> > >> I have decided to not offer support for prolog/epilog peeling when the > >> target alignment is not compile-time constant, as this didn't seem > >> useful, this is why 'vect_do_peeling' returns early if > >> DR_TARGET_ALIGNMENT is not constant. > >> > >> I bootstrapped and tested this on aarch64 and x86 basically > >> bootstrapping one target that uses this hook and one that doesn't. > >> > >> Is this OK for trunk? > > > > The patch looks good but I wonder wheter it is really necessary at this > > point. > > The goal of this patch is really to enable future work, on it's own it > does nothing. I am working on a small target-specific patch to enable > this for SVE, but I need to do a bit more analysis and benchmarking to > be able to determine whether its beneficial which I will not be able to > finish before end of stage 1. That is why I split them up and sent this > one upstream to see if I could get the middle-end change in. OK, fine with me then. Thanks, Richard. > > > > Thanks, > > Richard. > > > >> Cheers, > >> Andre > >> > >> 2018-11-05 Andre Vieira > >> > >> * config/aarch64/aarch64.c > >> (aarch64_vectorize_preferred_vector_alignment): > >> Change return type to poly_uint64. > >> (aarch64_simd_vector_alignment_reachable): Adapt to preferred > >> vector > >> alignment being a poly int. > >> * doc/tm.texi (TARGET_VECTORIZE_PREFERRED_VECTOR_ALIGNMENT): > >> Change return > >> type to poly_uint64. > >> * target.def (default_preferred_vector_alignment): Likewise. > >> * targhooks.c (default_preferred_vector_alignment): Likewise. > >> * targhooks.h (default_preferred_vector_alignment): Likewise. > >> * tree-vect-data-refs.c > >> (vect_calculate_target_alignment): Likewise. > >> (vect_compute_data_ref_alignment): Adapt to vector alignment > >> being a poly int. > >> (vect_update_misalignment_for_peel): Likewise. > >> (vect_enhance_data_refs_alignment): Likewise. > >> (vect_find_same_alignment_drs): Likewise. > >> (vect_duplicate_ssa_name_ptr_info): Likewise. > >> (vect_setup_realignment): Likewise. > >> (vect_can_force_dr_alignment_p): Change alignment parameter type to > >> poly_uint64. > >> * tree-vect-loop-manip.c (get_misalign_in_elems): Learn to > >> construct a mask > >> with a compile time variable vector alignment. > >> (vect_gen_prolog_loop_niters): Adapt to vector alignment being a > >> poly int. > >> (vect_do_peeling): Exit early if vector alignment is not constant. > >> * tree-vect-stmts.c (ensure_base_align): Adapt to vector alignment > >> being a > >> poly int. > >> (vectorizable_store): Likewise. > >> (vectorizable_load): Likweise. > >> * tree-vectorizer.h (struct dr_vec_info): Make
Re: [PATCH][GCC] Make DR_TARGET_ALIGNMENT compile time variable
On 05/11/18 12:41, Richard Biener wrote: > On Mon, Nov 5, 2018 at 1:07 PM Andre Vieira (lists) > wrote: >> >> >> Hi, >> Hi, Thank you for the quick response! See inline responses below. >> This patch enables targets to describe DR_TARGET_ALIGNMENT as a >> compile-time variable. It does so by turning the variable into a >> 'poly_uint64'. This should not affect the current code-generation for >> any target. >> >> We hope to use this in the near future for SVE using the >> current_vector_size as the preferred target alignment for vectors. In >> fact I have a patch to do just this, but I am still trying to figure out >> whether and when it is beneficial to peel for alignment with a runtime >> misalignment. > > In fact in most cases I have seen the issue is that it's not visible whether > peeling will be able to align _all_ references and doing peeling only to > align some is hardly beneficial. To improve things the vectorizer would > have to version the loop for the case where peeling can reach alignment > for a group of DRs and then vectorize one copy with peeling for alignment > and one copy with unaligned accesses. So I have seen code being peeled for alignment even when it only knows how to align one of a group (only checked 2 or 3) and I think this may still be beneficial in some cases. I am more worried about cases where the number of iterations isn't enough to justify the initial peeling cost or when the loop isn't memory bound, i.e. very arithmetic heavy loops. This is a bigger vectorization problem though, that would require some kind of cost-model. > >> The patch I am working on will change the behavior of >> auto-vectorization for SVE when building vector-length agnostic code for >> targets that benefit from aligned vector loads/stores. The patch will >> result in the generation of a runtime computation of misalignment and >> the construction of a corresponding mask for the first iteration of the >> loop. >> >> I have decided to not offer support for prolog/epilog peeling when the >> target alignment is not compile-time constant, as this didn't seem >> useful, this is why 'vect_do_peeling' returns early if >> DR_TARGET_ALIGNMENT is not constant. >> >> I bootstrapped and tested this on aarch64 and x86 basically >> bootstrapping one target that uses this hook and one that doesn't. >> >> Is this OK for trunk? > > The patch looks good but I wonder wheter it is really necessary at this > point. The goal of this patch is really to enable future work, on it's own it does nothing. I am working on a small target-specific patch to enable this for SVE, but I need to do a bit more analysis and benchmarking to be able to determine whether its beneficial which I will not be able to finish before end of stage 1. That is why I split them up and sent this one upstream to see if I could get the middle-end change in. > > Thanks, > Richard. > >> Cheers, >> Andre >> >> 2018-11-05 Andre Vieira >> >> * config/aarch64/aarch64.c >> (aarch64_vectorize_preferred_vector_alignment): >> Change return type to poly_uint64. >> (aarch64_simd_vector_alignment_reachable): Adapt to preferred vector >> alignment being a poly int. >> * doc/tm.texi (TARGET_VECTORIZE_PREFERRED_VECTOR_ALIGNMENT): Change >> return >> type to poly_uint64. >> * target.def (default_preferred_vector_alignment): Likewise. >> * targhooks.c (default_preferred_vector_alignment): Likewise. >> * targhooks.h (default_preferred_vector_alignment): Likewise. >> * tree-vect-data-refs.c >> (vect_calculate_target_alignment): Likewise. >> (vect_compute_data_ref_alignment): Adapt to vector alignment >> being a poly int. >> (vect_update_misalignment_for_peel): Likewise. >> (vect_enhance_data_refs_alignment): Likewise. >> (vect_find_same_alignment_drs): Likewise. >> (vect_duplicate_ssa_name_ptr_info): Likewise. >> (vect_setup_realignment): Likewise. >> (vect_can_force_dr_alignment_p): Change alignment parameter type to >> poly_uint64. >> * tree-vect-loop-manip.c (get_misalign_in_elems): Learn to construct >> a mask >> with a compile time variable vector alignment. >> (vect_gen_prolog_loop_niters): Adapt to vector alignment being a >> poly int. >> (vect_do_peeling): Exit early if vector alignment is not constant. >> * tree-vect-stmts.c (ensure_base_align): Adapt to vector alignment >> being a >> poly int. >> (vectorizable_store): Likewise. >> (vectorizable_load): Likweise. >> * tree-vectorizer.h (struct dr_vec_info): Make target_alignment >> field a >> poly_uint64. >> (vect_known_alignment_in_bytes): Adapt to vector alignment being a >> poly >> int. >> (vect_can_force_dr_alignment_p): Change alignment parameter type to >> poly_uint64.
Re: [PATCH][GCC] Make DR_TARGET_ALIGNMENT compile time variable
On Mon, Nov 5, 2018 at 1:07 PM Andre Vieira (lists) wrote: > > > Hi, > > This patch enables targets to describe DR_TARGET_ALIGNMENT as a > compile-time variable. It does so by turning the variable into a > 'poly_uint64'. This should not affect the current code-generation for > any target. > > We hope to use this in the near future for SVE using the > current_vector_size as the preferred target alignment for vectors. In > fact I have a patch to do just this, but I am still trying to figure out > whether and when it is beneficial to peel for alignment with a runtime > misalignment. In fact in most cases I have seen the issue is that it's not visible whether peeling will be able to align _all_ references and doing peeling only to align some is hardly beneficial. To improve things the vectorizer would have to version the loop for the case where peeling can reach alignment for a group of DRs and then vectorize one copy with peeling for alignment and one copy with unaligned accesses. > The patch I am working on will change the behavior of > auto-vectorization for SVE when building vector-length agnostic code for > targets that benefit from aligned vector loads/stores. The patch will > result in the generation of a runtime computation of misalignment and > the construction of a corresponding mask for the first iteration of the > loop. > > I have decided to not offer support for prolog/epilog peeling when the > target alignment is not compile-time constant, as this didn't seem > useful, this is why 'vect_do_peeling' returns early if > DR_TARGET_ALIGNMENT is not constant. > > I bootstrapped and tested this on aarch64 and x86 basically > bootstrapping one target that uses this hook and one that doesn't. > > Is this OK for trunk? The patch looks good but I wonder wheter it is really necessary at this point. Thanks, Richard. > Cheers, > Andre > > 2018-11-05 Andre Vieira > > * config/aarch64/aarch64.c > (aarch64_vectorize_preferred_vector_alignment): > Change return type to poly_uint64. > (aarch64_simd_vector_alignment_reachable): Adapt to preferred vector > alignment being a poly int. > * doc/tm.texi (TARGET_VECTORIZE_PREFERRED_VECTOR_ALIGNMENT): Change > return > type to poly_uint64. > * target.def (default_preferred_vector_alignment): Likewise. > * targhooks.c (default_preferred_vector_alignment): Likewise. > * targhooks.h (default_preferred_vector_alignment): Likewise. > * tree-vect-data-refs.c > (vect_calculate_target_alignment): Likewise. > (vect_compute_data_ref_alignment): Adapt to vector alignment > being a poly int. > (vect_update_misalignment_for_peel): Likewise. > (vect_enhance_data_refs_alignment): Likewise. > (vect_find_same_alignment_drs): Likewise. > (vect_duplicate_ssa_name_ptr_info): Likewise. > (vect_setup_realignment): Likewise. > (vect_can_force_dr_alignment_p): Change alignment parameter type to > poly_uint64. > * tree-vect-loop-manip.c (get_misalign_in_elems): Learn to construct > a mask > with a compile time variable vector alignment. > (vect_gen_prolog_loop_niters): Adapt to vector alignment being a poly > int. > (vect_do_peeling): Exit early if vector alignment is not constant. > * tree-vect-stmts.c (ensure_base_align): Adapt to vector alignment > being a > poly int. > (vectorizable_store): Likewise. > (vectorizable_load): Likweise. > * tree-vectorizer.h (struct dr_vec_info): Make target_alignment field > a > poly_uint64. > (vect_known_alignment_in_bytes): Adapt to vector alignment being a > poly > int. > (vect_can_force_dr_alignment_p): Change alignment parameter type to > poly_uint64.
[PATCH][GCC] Make DR_TARGET_ALIGNMENT compile time variable
Hi,
This patch enables targets to describe DR_TARGET_ALIGNMENT as a
compile-time variable. It does so by turning the variable into a
'poly_uint64'. This should not affect the current code-generation for
any target.
We hope to use this in the near future for SVE using the
current_vector_size as the preferred target alignment for vectors. In
fact I have a patch to do just this, but I am still trying to figure out
whether and when it is beneficial to peel for alignment with a runtime
misalignment. The patch I am working on will change the behavior of
auto-vectorization for SVE when building vector-length agnostic code for
targets that benefit from aligned vector loads/stores. The patch will
result in the generation of a runtime computation of misalignment and
the construction of a corresponding mask for the first iteration of the
loop.
I have decided to not offer support for prolog/epilog peeling when the
target alignment is not compile-time constant, as this didn't seem
useful, this is why 'vect_do_peeling' returns early if
DR_TARGET_ALIGNMENT is not constant.
I bootstrapped and tested this on aarch64 and x86 basically
bootstrapping one target that uses this hook and one that doesn't.
Is this OK for trunk?
Cheers,
Andre
2018-11-05 Andre Vieira
* config/aarch64/aarch64.c
(aarch64_vectorize_preferred_vector_alignment):
Change return type to poly_uint64.
(aarch64_simd_vector_alignment_reachable): Adapt to preferred vector
alignment being a poly int.
* doc/tm.texi (TARGET_VECTORIZE_PREFERRED_VECTOR_ALIGNMENT): Change
return
type to poly_uint64.
* target.def (default_preferred_vector_alignment): Likewise.
* targhooks.c (default_preferred_vector_alignment): Likewise.
* targhooks.h (default_preferred_vector_alignment): Likewise.
* tree-vect-data-refs.c
(vect_calculate_target_alignment): Likewise.
(vect_compute_data_ref_alignment): Adapt to vector alignment
being a poly int.
(vect_update_misalignment_for_peel): Likewise.
(vect_enhance_data_refs_alignment): Likewise.
(vect_find_same_alignment_drs): Likewise.
(vect_duplicate_ssa_name_ptr_info): Likewise.
(vect_setup_realignment): Likewise.
(vect_can_force_dr_alignment_p): Change alignment parameter type to
poly_uint64.
* tree-vect-loop-manip.c (get_misalign_in_elems): Learn to construct a
mask
with a compile time variable vector alignment.
(vect_gen_prolog_loop_niters): Adapt to vector alignment being a poly
int.
(vect_do_peeling): Exit early if vector alignment is not constant.
* tree-vect-stmts.c (ensure_base_align): Adapt to vector alignment
being a
poly int.
(vectorizable_store): Likewise.
(vectorizable_load): Likweise.
* tree-vectorizer.h (struct dr_vec_info): Make target_alignment field a
poly_uint64.
(vect_known_alignment_in_bytes): Adapt to vector alignment being a poly
int.
(vect_can_force_dr_alignment_p): Change alignment parameter type to
poly_uint64.
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 4c7790826658539f71f2fd9eb9ea0329081938be..19f084abefd76255d1a4a0b726e51c7025b9cea6 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -14120,7 +14120,7 @@ aarch64_simd_vector_alignment (const_tree type)
}
/* Implement target hook TARGET_VECTORIZE_PREFERRED_VECTOR_ALIGNMENT. */
-static HOST_WIDE_INT
+static poly_uint64
aarch64_vectorize_preferred_vector_alignment (const_tree type)
{
if (aarch64_sve_data_mode_p (TYPE_MODE (type)))
@@ -14145,9 +14145,11 @@ aarch64_simd_vector_alignment_reachable (const_tree type, bool is_packed)
/* For fixed-length vectors, check that the vectorizer will aim for
full-vector alignment. This isn't true for generic GCC vectors
that are wider than the ABI maximum of 128 bits. */
+ poly_uint64 preferred_alignment =
+aarch64_vectorize_preferred_vector_alignment (type);
if (TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST
- && (wi::to_widest (TYPE_SIZE (type))
- != aarch64_vectorize_preferred_vector_alignment (type)))
+ && maybe_ne (wi::to_widest (TYPE_SIZE (type)),
+ preferred_alignment))
return false;
/* Vectors whose size is <= BIGGEST_ALIGNMENT are naturally aligned. */
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index 0fcf8069b8cc948fcaf5604a1235fe269de7e8f3..328eb43ca2495dd889bc47cf136761381a594cdf 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -5889,7 +5889,7 @@ For vector memory operations the cost may depend on type (@var{vectype}) and
misalignment value (@var{misalign}).
@end deftypefn
-@deftypefn {Target Hook} HOST_WIDE_INT TARGET_VECTORIZE_PREFERRED_VECTOR_ALIGNMENT (const_tree @var{type})
+@deftypefn {Target Hook} poly_uint64 TARGET_VECTORIZE_PREFERRED_VECTOR_ALIGNMENT (const_tree @var{type})
This hook returns the preferred alignment in
