[PATCH][GCC] Make DR_TARGET_ALIGNMENT compile time variable

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH][GCC] Make DR_TARGET_ALIGNMENT compile time variable

Andre Simoes Dias Vieira

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  <[hidden email]>

        * 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.

dr_target_alignment.patch (21K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][GCC] Make DR_TARGET_ALIGNMENT compile time variable

Richard Biener-2
On Mon, Nov 5, 2018 at 1:07 PM Andre Vieira (lists)
<[hidden email]> 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  <[hidden email]>
>
>         * 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.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][GCC] Make DR_TARGET_ALIGNMENT compile time variable

Andre Simoes Dias Vieira
On 05/11/18 12:41, Richard Biener wrote:
> On Mon, Nov 5, 2018 at 1:07 PM Andre Vieira (lists)
> <[hidden email]> 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  <[hidden email]>
>>
>>         * 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.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][GCC] Make DR_TARGET_ALIGNMENT compile time variable

Richard Biener-2
On Fri, Nov 9, 2018 at 5:08 PM Andre Vieira (lists)
<[hidden email]> wrote:

>
> On 05/11/18 12:41, Richard Biener wrote:
> > On Mon, Nov 5, 2018 at 1:07 PM Andre Vieira (lists)
> > <[hidden email]> 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  <[hidden email]>
> >>
> >>         * 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.
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][GCC] Make DR_TARGET_ALIGNMENT compile time variable

Dominique d'Humières-2
In reply to this post by Andre Simoes Dias Vieira
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<Ca, bool>::type maybe_lt(const Ca&, const poly_int_pod<N, Cb>&) [with unsigned int N = 1; Ca = int; Cb = long long unsigned int; typename if_nonpoly<Ca, 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

TIA

Dominique
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][GCC] Make DR_TARGET_ALIGNMENT compile time variable

Iain Sandoe-5

> On 13 Nov 2018, at 19:31, Dominique d'Humières <[hidden email]> 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<Ca, bool>::type maybe_lt(const Ca&, const poly_int_pod<N, Cb>&) [with unsigned int N = 1; Ca = int; Cb = long long unsigned int; typename if_nonpoly<Ca, 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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][GCC] Make DR_TARGET_ALIGNMENT compile time variable

Richard Biener-2
On Wed, Nov 14, 2018 at 12:43 PM Iain Sandoe <[hidden email]> wrote:

>
>
> > On 13 Nov 2018, at 19:31, Dominique d'Humières <[hidden email]> 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<Ca, bool>::type maybe_lt(const Ca&, const poly_int_pod<N, Cb>&) [with unsigned int N = 1; Ca = int; Cb = long long unsigned int; typename if_nonpoly<Ca, 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
>