Re: [PATCH] Allow different vector types for stmt groups

[Richard Biener <rguenther@suse.de>]

On Wed, 22 Sep 2021, Hongtao Liu wrote:

> On Tue, Sep 21, 2021 at 10:55 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > On Mon, Sep 20, 2021 at 5:15 AM Richard Biener via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> > >
> > > This allows vectorization (in practice non-loop vectorization) to
> > > have a stmt participate in different vector type vectorizations.
> > > It allows us to remove vect_update_shared_vectype and replace it
> > > by pushing/popping STMT_VINFO_VECTYPE from SLP_TREE_VECTYPE around
> > > vect_analyze_stmt and vect_transform_stmt.
> > >
> > > For data-ref the situation is a bit more complicated since we
> > > analyze alignment info with a specific vector type in mind which
> > > doesn't play well when that changes.
> > >
> > > So the bulk of the change is passing down the actual vector type
> > > used for a vectorized access to the various accessors of alignment
> > > info, first and foremost dr_misalignment but also aligned_access_p,
> > > known_alignment_for_access_p, vect_known_alignment_in_bytes and
> > > vect_supportable_dr_alignment.  I took the liberty to replace
> > > ALL_CAPS macro accessors with the lower-case function invocations.
> > >
> > > The actual changes to the behavior are in dr_misalignment which now
> > > is the place factoring in the negative step adjustment as well as
> > > handling alignment queries for a vector type with bigger alignment
> > > requirements than what we can (or have) analyze(d).
> > >
> > > vect_slp_analyze_node_alignment makes use of this and upon receiving
> > > a vector type with a bigger alingment desire re-analyzes the DR
> > > with respect to it but keeps an older more precise result if possible.
> > > In this context it might be possible to do the analysis just once
> > > but instead of analyzing with respect to a specific desired alignment
> > > look for the biggest alignment we can compute a not unknown alignment.
> > >
> > > The ChangeLog includes the functional changes but not the bulk due
> > > to the alignment accessor API changes - I hope that's something good.
> > >
> > > Bootstrapped and tested on x86_64-unknown-linux-gnu, testing on SPEC
> > > CPU 2017 in progress (for stats and correctness).
> > >
> > > Any comments?
> > >
> > > Thanks,
> > > Richard.
> > >
> > > 2021-09-17  Richard Biener  <rguenther@suse.de>
> > >
> > >         PR tree-optimization/97351
> > >         PR tree-optimization/97352
> > >         PR tree-optimization/82426
> > >         * tree-vectorizer.h (dr_misalignment): Add vector type
> > >         argument.
> > >         (aligned_access_p): Likewise.
> > >         (known_alignment_for_access_p): Likewise.
> > >         (vect_supportable_dr_alignment): Likewise.
> > >         (vect_known_alignment_in_bytes): Likewise.  Refactor.
> > >         (DR_MISALIGNMENT): Remove.
> > >         (vect_update_shared_vectype): Likewise.
> > >         * tree-vect-data-refs.c (dr_misalignment): Refactor, handle
> > >         a vector type with larger alignment requirement and apply
> > >         the negative step adjustment here.
> > >         (vect_calculate_target_alignment): Remove.
> > >         (vect_compute_data_ref_alignment): Get explicit vector type
> > >         argument, do not apply a negative step alignment adjustment
> > >         here.
> > >         (vect_slp_analyze_node_alignment): Re-analyze alignment
> > >         when we re-visit the DR with a bigger desired alignment but
> > >         keep more precise results from smaller alignments.
> > >         * tree-vect-slp.c (vect_update_shared_vectype): Remove.
> > >         (vect_slp_analyze_node_operations_1): Do not update the
> > >         shared vector type on stmts.
> > >         * tree-vect-stmts.c (vect_analyze_stmt): Push/pop the
> > >         vector type of an SLP node to the representative stmt-info.
> > >         (vect_transform_stmt): Likewise.
> > >
> > >         * gcc.target/i386/vect-pr82426.c: New testcase.
> > >         * gcc.target/i386/vect-pr97352.c: Likewise.
> > > ---
> > >  gcc/testsuite/gcc.target/i386/vect-pr82426.c |  32 +++
> > >  gcc/testsuite/gcc.target/i386/vect-pr97352.c |  22 ++
> > >  gcc/tree-vect-data-refs.c                    | 217 ++++++++++---------
> > >  gcc/tree-vect-slp.c                          |  59 -----
> > >  gcc/tree-vect-stmts.c                        |  77 ++++---
> > >  gcc/tree-vectorizer.h                        |  32 ++-
> > >  6 files changed, 231 insertions(+), 208 deletions(-)
> > >  create mode 100644 gcc/testsuite/gcc.target/i386/vect-pr82426.c
> > >  create mode 100644 gcc/testsuite/gcc.target/i386/vect-pr97352.c
> > >
> > > diff --git a/gcc/testsuite/gcc.target/i386/vect-pr82426.c b/gcc/testsuite/gcc.target/i386/vect-pr82426.c
> > > new file mode 100644
> > > index 00000000000..741a1d14d36
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.target/i386/vect-pr82426.c
> > > @@ -0,0 +1,32 @@
> > > +/* i?86 does not have V2SF, x32 does though.  */
> > > +/* { dg-do compile { target { lp64 || x32 } } } */
> >
> > It should be target { ! ia32 }
> >
> > > +/* ???  With AVX512 we only realize one FMA opportunity.  */
> >
> > Hongtao, is AVX512 missing 64-bit vector support??
> >
> (define_insn "fmav2sf4"
>   [(set (match_operand:V2SF 0 "register_operand" "=v,v,x")
> (fma:V2SF
>   (match_operand:V2SF 1 "register_operand" "%0,v,x")
>   (match_operand:V2SF 2 "register_operand" "v,v,x")
>   (match_operand:V2SF 3 "register_operand" "v,0,x")))]
>   "(TARGET_FMA || TARGET_FMA4) && TARGET_MMX_WITH_SSE"
> Need to add TARGET_AVX512VL to the condition.
> I'll post a patch for this.

I have adjusted the testcase and it now passes with AVX512VL.

I plan to commit the change next Monday if there are no further comments.

Richard.