On Wed, 24 Feb 2021 at 09:38, Richard Biener <rguent...@suse.de> wrote:
>
> On Tue, 23 Feb 2021, Tamar Christina wrote:
>
> > Hi Richi,
> >
> > The attached testcase ICEs due to a couple of issues.
> > In the testcase you have two SLP instances that share the majority of their
> > definition with each other.  One tree defines a COMPLEX_MUL sequence and the
> > other tree a COMPLEX_FMA.
> >
> > The ice happens because:
> >
> > 1. the refcounts are wrong, in particular the FMA case doesn't correctly 
> > count
> > the references for the COMPLEX_MUL that it consumes.
> >
> > 2. when the FMA is created it incorrectly assumes it can just tear apart 
> > the MUL
> > node that it's consuming.  This is wrong and should only be done when there 
> > is
> > no more uses of the node, in which case the vector only pattern is no longer
> > relevant.
> >
> > To fix the last part the SLP only pattern reset code was moved into
> > vect_free_slp_tree which results in cleaner code.  I also think it does 
> > belong
> > there since that function knows when there are no more uses of the node and 
> > so
> > the pattern should be unmarked, so when the the vectorizer is inspecting 
> > the BB
> > it doesn't find the now invalid vector only patterns.
> >
> > The patch also clears the SLP_TREE_REPRESENTATIVE when stores are removed 
> > such
> > that we don't hit an error later trying to free the stmt_vec_info again.
> >
> > Lastly it also tweaks the results of whether a pattern has been detected or 
> > not
> > to return true when another SLP instance has created a pattern that is only 
> > used
> > by a different instance (due to the trees being unshared).
> >
> > Instead of ICEing this code now produces
> >
> >         adrp    x1, .LANCHOR0
> >         add     x2, x1, :lo12:.LANCHOR0
> >         movi    v1.2s, 0
> >         mov     w0, 0
> >         ldr     x4, [x1, #:lo12:.LANCHOR0]
> >         ldrsw   x3, [x2, 16]
> >         ldr     x1, [x2, 8]
> >         ldrsw   x2, [x2, 20]
> >         ldr     d0, [x4]
> >         ldr     d2, [x1, x3, lsl 3]
> >         fcmla   v2.2s, v0.2s, v0.2s, #0
> >         fcmla   v2.2s, v0.2s, v0.2s, #90
> >         str     d2, [x1, x3, lsl 3]
> >         fcmla   v1.2s, v0.2s, v0.2s, #0
> >         fcmla   v1.2s, v0.2s, v0.2s, #90
> >         str     d1, [x1, x2, lsl 3]
> >         ret
> >
> > PS. This testcase actually shows that the codegen we get in these cases is 
> > not
> > optimal. It should generate a MUL + ADD instead MUL + FMA.
> >
> > But that's for GCC 12.
> >
> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.

Hi Tamar,

The new test fails on aarch64_be:
FAIL: g++.dg/vect/pr99149.cc  -std=c++14  scan-tree-dump-times slp2
"stmt.*COMPLEX_FMA" 1
FAIL: g++.dg/vect/pr99149.cc  -std=c++17  scan-tree-dump-times slp2
"stmt.*COMPLEX_FMA" 1
FAIL: g++.dg/vect/pr99149.cc  -std=c++2a  scan-tree-dump-times slp2
"stmt.*COMPLEX_FMA" 1
FAIL: g++.dg/vect/pr99149.cc  -std=c++98  scan-tree-dump-times slp2
"stmt.*COMPLEX_FMA" 1

Is that supposed to work, or should you disable the test on aarch64_be?

Christophe

>
> OK.
>
> Thanks,
> Richard.
>
> > Thanks,
> > Tamar
> >
> > gcc/ChangeLog:
> >
> >       PR tree-optimization/99149
> >       * tree-vect-slp-patterns.c (vect_detect_pair_op): Don't recreate the
> >       buffer.
> >       (vect_slp_reset_pattern): Remove.
> >       (complex_fma_pattern::matches): Remove call to vect_slp_reset_pattern.
> >       (complex_mul_pattern::build, complex_fma_pattern::build,
> >       complex_fms_pattern::build): Fix ref counts.
> >       * tree-vect-slp.c (vect_free_slp_tree): Undo SLP only pattern 
> > relevancy
> >       when node is being deleted.
> >       (vect_match_slp_patterns_2): Correct result of cache hit on patterns.
> >       (vect_schedule_slp): Invalidate SLP_TREE_REPRESENTATIVE of removed
> >       stores.
> >       * tree-vectorizer.c (vec_info::new_stmt_vec_info): Initialize value.
> >
> > gcc/testsuite/ChangeLog:
> >
> >       PR tree-optimization/99149
> >       * g++.dg/vect/pr99149.cc: New test.
> >
> > --- inline copy of patch --
> > diff --git a/gcc/testsuite/g++.dg/vect/pr99149.cc 
> > b/gcc/testsuite/g++.dg/vect/pr99149.cc
> > new file mode 100755
> > index 
> > 0000000000000000000000000000000000000000..902a26f576fcc79d2802bec093668674cca1c84f
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/vect/pr99149.cc
> > @@ -0,0 +1,28 @@
> > +/* { dg-do compile } */
> > +/* { dg-additional-options "-w -O3 -march=armv8.3-a -fdump-tree-slp-all" { 
> > target { aarch64*-*-* } } } */
> > +
> > +class a {
> > +  float b;
> > +  float c;
> > +
> > +public:
> > +  a(float d, float e) : b(d), c(e) {}
> > +  a operator+(a d) { return a(b + d.b, c + d.c); }
> > +  a operator*(a d) { return a(b * b - c * c, b * c + c * d.b); }
> > +};
> > +int f, g;
> > +class {
> > +  a *h;
> > +  a *i;
> > +
> > +public:
> > +  void j() {
> > +    a k = h[0], l = i[g], m = k * i[f];
> > +    i[g] = l + m;
> > +    i[f] = m;
> > +  }
> > +} n;
> > +main() { n.j(); }
> > +
> > +/* { dg-final { scan-tree-dump-times "stmt.*COMPLEX_MUL" 1 "slp2" } } */
> > +/* { dg-final { scan-tree-dump-times "stmt.*COMPLEX_FMA" 1 "slp2" } } */
> > diff --git a/gcc/tree-vect-slp-patterns.c b/gcc/tree-vect-slp-patterns.c
> > index 
> > f0817da9f622d22e3df2e30410d1cf610b4ffa1d..1e2769662a54229ab8e24390f97dfe206f17ab57
> >  100644
> > --- a/gcc/tree-vect-slp-patterns.c
> > +++ b/gcc/tree-vect-slp-patterns.c
> > @@ -407,9 +407,8 @@ vect_detect_pair_op (slp_tree node1, slp_tree node2, 
> > lane_permutation_t &lanes,
> >
> >    if (result != CMPLX_NONE && ops != NULL)
> >      {
> > -      ops->create (2);
> > -      ops->quick_push (node1);
> > -      ops->quick_push (node2);
> > +      ops->safe_push (node1);
> > +      ops->safe_push (node2);
> >      }
> >    return result;
> >  }
> > @@ -1090,15 +1089,17 @@ complex_mul_pattern::build (vec_info *vinfo)
> >  {
> >    slp_tree node;
> >    unsigned i;
> > +  slp_tree newnode
> > +    = vect_build_combine_node (this->m_ops[0], this->m_ops[1], 
> > *this->m_node);
> > +  SLP_TREE_REF_COUNT (this->m_ops[2])++;
> > +
> >    FOR_EACH_VEC_ELT (SLP_TREE_CHILDREN (*this->m_node), i, node)
> >      vect_free_slp_tree (node);
> >
> >    /* First re-arrange the children.  */
> >    SLP_TREE_CHILDREN (*this->m_node).reserve_exact (2);
> >    SLP_TREE_CHILDREN (*this->m_node)[0] = this->m_ops[2];
> > -  SLP_TREE_CHILDREN (*this->m_node)[1] =
> > -    vect_build_combine_node (this->m_ops[0], this->m_ops[1], 
> > *this->m_node);
> > -  SLP_TREE_REF_COUNT (this->m_ops[2])++;
> > +  SLP_TREE_CHILDREN (*this->m_node)[1] = newnode;
> >
> >    /* And then rewrite the node itself.  */
> >    complex_pattern::build (vinfo);
> > @@ -1133,18 +1134,6 @@ class complex_fma_pattern : public complex_pattern
> >      }
> >  };
> >
> > -/* Helper function to "reset" a previously matched node and undo the 
> > changes
> > -   made enough so that the node is treated as an irrelevant node.  */
> > -
> > -static inline void
> > -vect_slp_reset_pattern (slp_tree node)
> > -{
> > -  stmt_vec_info stmt_info = vect_orig_stmt (SLP_TREE_REPRESENTATIVE 
> > (node));
> > -  STMT_VINFO_IN_PATTERN_P (stmt_info) = false;
> > -  STMT_SLP_TYPE (stmt_info) = pure_slp;
> > -  SLP_TREE_REPRESENTATIVE (node) = stmt_info;
> > -}
> > -
> >  /* Pattern matcher for trying to match complex multiply and accumulate
> >     and multiply and subtract patterns in SLP tree.
> >     If the operation matches then IFN is set to the operation it matched and
> > @@ -1208,15 +1197,6 @@ complex_fma_pattern::matches (complex_operation_t op,
> >    if (!vect_pattern_validate_optab (ifn, vnode))
> >      return IFN_LAST;
> >
> > -  /* FMA matched ADD + CMUL.  During the matching of CMUL the
> > -     stmt that starts the pattern is marked as being in a pattern,
> > -     namely the CMUL.  When replacing this with a CFMA we have to
> > -     unmark this statement as being in a pattern.  This is because
> > -     vect_mark_pattern_stmts will only mark the current stmt as being
> > -     in a pattern.  Later on when the scalar stmts are examined the
> > -     old statement which is supposed to be irrelevant will point to
> > -     CMUL unless we undo the pattern relationship here.  */
> > -  vect_slp_reset_pattern (node);
> >    ops->truncate (0);
> >    ops->create (3);
> >
> > @@ -1259,10 +1239,17 @@ complex_fma_pattern::recognize 
> > (slp_tree_to_load_perm_map_t *perm_cache,
> >  void
> >  complex_fma_pattern::build (vec_info *vinfo)
> >  {
> > +  slp_tree node = SLP_TREE_CHILDREN (*this->m_node)[1];
> > +
> >    SLP_TREE_CHILDREN (*this->m_node).release ();
> >    SLP_TREE_CHILDREN (*this->m_node).create (3);
> >    SLP_TREE_CHILDREN (*this->m_node).safe_splice (this->m_ops);
> >
> > +  SLP_TREE_REF_COUNT (this->m_ops[1])++;
> > +  SLP_TREE_REF_COUNT (this->m_ops[2])++;
> > +
> > +  vect_free_slp_tree (node);
> > +
> >    complex_pattern::build (vinfo);
> >  }
> >
> > @@ -1427,6 +1414,11 @@ complex_fms_pattern::build (vec_info *vinfo)
> >  {
> >    slp_tree node;
> >    unsigned i;
> > +  slp_tree newnode =
> > +    vect_build_combine_node (this->m_ops[2], this->m_ops[3], 
> > *this->m_node);
> > +  SLP_TREE_REF_COUNT (this->m_ops[0])++;
> > +  SLP_TREE_REF_COUNT (this->m_ops[1])++;
> > +
> >    FOR_EACH_VEC_ELT (SLP_TREE_CHILDREN (*this->m_node), i, node)
> >      vect_free_slp_tree (node);
> >
> > @@ -1436,10 +1428,7 @@ complex_fms_pattern::build (vec_info *vinfo)
> >    /* First re-arrange the children.  */
> >    SLP_TREE_CHILDREN (*this->m_node).quick_push (this->m_ops[0]);
> >    SLP_TREE_CHILDREN (*this->m_node).quick_push (this->m_ops[1]);
> > -  SLP_TREE_CHILDREN (*this->m_node).quick_push (
> > -    vect_build_combine_node (this->m_ops[2], this->m_ops[3], 
> > *this->m_node));
> > -  SLP_TREE_REF_COUNT (this->m_ops[0])++;
> > -  SLP_TREE_REF_COUNT (this->m_ops[1])++;
> > +  SLP_TREE_CHILDREN (*this->m_node).quick_push (newnode);
> >
> >    /* And then rewrite the node itself.  */
> >    complex_pattern::build (vinfo);
> > diff --git a/gcc/tree-vect-slp.c b/gcc/tree-vect-slp.c
> > index 
> > ea8a97b01c6371791ac66de3e1dabfedee69cb67..605873714a5cafaaf822f61f1f769f96b3876694
> >  100644
> > --- a/gcc/tree-vect-slp.c
> > +++ b/gcc/tree-vect-slp.c
> > @@ -146,6 +146,16 @@ vect_free_slp_tree (slp_tree node)
> >      if (child)
> >        vect_free_slp_tree (child);
> >
> > +  /* If the node defines any SLP only patterns then those patterns are no
> > +     longer valid and should be removed.  */
> > +  stmt_vec_info rep_stmt_info = SLP_TREE_REPRESENTATIVE (node);
> > +  if (rep_stmt_info && STMT_VINFO_SLP_VECT_ONLY_PATTERN (rep_stmt_info))
> > +    {
> > +      stmt_vec_info stmt_info = vect_orig_stmt (rep_stmt_info);
> > +      STMT_VINFO_IN_PATTERN_P (stmt_info) = false;
> > +      STMT_SLP_TYPE (stmt_info) = STMT_SLP_TYPE (rep_stmt_info);
> > +    }
> > +
> >    delete node;
> >  }
> >
> > @@ -2395,7 +2405,9 @@ vect_match_slp_patterns_2 (slp_tree *ref_node, 
> > vec_info *vinfo,
> >    slp_tree node = *ref_node;
> >    bool found_p = false;
> >    if (!node || visited->add (node))
> > -    return false;
> > +    return node
> > +        && SLP_TREE_REPRESENTATIVE (node)
> > +        && STMT_VINFO_SLP_VECT_ONLY_PATTERN (SLP_TREE_REPRESENTATIVE 
> > (node));
> >
> >    slp_tree child;
> >    FOR_EACH_VEC_ELT (SLP_TREE_CHILDREN (node), i, child)
> > @@ -6451,6 +6463,11 @@ vect_schedule_slp (vec_info *vinfo, 
> > vec<slp_instance> slp_instances)
> >         store_info = vect_orig_stmt (store_info);
> >         /* Free the attached stmt_vec_info and remove the stmt.  */
> >         vinfo->remove_stmt (store_info);
> > +
> > +       /* Invalidate SLP_TREE_REPRESENTATIVE in case we released it
> > +          to not crash in vect_free_slp_tree later.  */
> > +       if (SLP_TREE_REPRESENTATIVE (root) == store_info)
> > +         SLP_TREE_REPRESENTATIVE (root) = NULL;
> >          }
> >      }
> >  }
> > diff --git a/gcc/tree-vectorizer.c b/gcc/tree-vectorizer.c
> > index 
> > 5b45df3a4e00266b7530eb4da6985f0d940cb05b..63ba594f2276850a00fc372072d98326891f19e6
> >  100644
> > --- a/gcc/tree-vectorizer.c
> > +++ b/gcc/tree-vectorizer.c
> > @@ -695,6 +695,7 @@ vec_info::new_stmt_vec_info (gimple *stmt)
> >    STMT_VINFO_REDUC_FN (res) = IFN_LAST;
> >    STMT_VINFO_REDUC_IDX (res) = -1;
> >    STMT_VINFO_SLP_VECT_ONLY (res) = false;
> > +  STMT_VINFO_SLP_VECT_ONLY_PATTERN (res) = false;
> >    STMT_VINFO_VEC_STMTS (res) = vNULL;
> >
> >    if (is_a <loop_vec_info> (this)
> >
> >
> >
>
> --
> Richard Biener <rguent...@suse.de>
> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
> Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)

Reply via email to