V5 [PATCH] Optimize vector constructor

On Thu, Mar 7, 2019 at 9:51 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Wed, Mar 6, 2019 at 8:33 PM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Wed, Mar 6, 2019 at 8:46 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > >
> > > On Tue, Mar 5, 2019 at 1:46 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > >
> > > > On Mon, Mar 04, 2019 at 12:55:04PM +0100, Richard Biener wrote:
> > > > > On Sun, Mar 3, 2019 at 10:13 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > > > >
> > > > > > On Sun, Mar 03, 2019 at 06:40:09AM -0800, Andrew Pinski wrote:
> > > > > > > )
> > > > > > > ,On Sun, Mar 3, 2019 at 6:32 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > > > > > >
> > > > > > > > For vector init constructor:
> > > > > > > >
> > > > > > > > ---
> > > > > > > > typedef float __v4sf __attribute__ ((__vector_size__ (16)));
> > > > > > > >
> > > > > > > > __v4sf
> > > > > > > > foo (__v4sf x, float f)
> > > > > > > > {
> > > > > > > >   __v4sf y = { f, x[1], x[2], x[3] };
> > > > > > > >   return y;
> > > > > > > > }
> > > > > > > > ---
> > > > > > > >
> > > > > > > > we can optimize vector init constructor with vector copy or permute
> > > > > > > > followed by a single scalar insert:
> > > >
> > > > > and you want to advance to the _1 = BIT_INSERT_EXPR here.  The easiest way
> > > > > is to emit a new stmt for _2 = copy ...; and do the set_rhs with the
> > > > > BIT_INSERT_EXPR.
> > > >
> > > > Thanks for BIT_INSERT_EXPR suggestion.  I am testing this patch.
> > > >
> > > >
> > > > H.J.
> > > > ---
> > > > We can optimize vector constructor with vector copy or permute followed
> > > > by a single scalar insert:
> > > >
> > > >   __v4sf y;
> > > >   __v4sf D.1930;
> > > >   float _1;
> > > >   float _2;
> > > >   float _3;
> > > >
> > > >   <bb 2> :
> > > >   _1 = BIT_FIELD_REF <x_9(D), 32, 96>;
> > > >   _2 = BIT_FIELD_REF <x_9(D), 32, 64>;
> > > >   _3 = BIT_FIELD_REF <x_9(D), 32, 32>;
> > > >   y_6 = {f_5(D), _3, _2, _1};
> > > >   return y_6;
> > > >
> > > > with
> > > >
> > > >  __v4sf y;
> > > >   __v4sf D.1930;
> > > >   float _1;
> > > >   float _2;
> > > >   float _3;
> > > >   vector(4) float _8;
> > > >
> > > >   <bb 2> :
> > > >   _1 = BIT_FIELD_REF <x_9(D), 32, 96>;
> > > >   _2 = BIT_FIELD_REF <x_9(D), 32, 64>;
> > > >   _3 = BIT_FIELD_REF <x_9(D), 32, 32>;
> > > >   _8 = x_9(D);
> > > >   y_6 = BIT_INSERT_EXPR <x_9(D), f_5(D), 0 (32 bits)>;
> > > >   return y_6;
> > > >
> > > > gcc/
> > > >
> > > >         PR tree-optimization/88828
> > > >         * tree-ssa-forwprop.c (simplify_vector_constructor): Optimize
> > > >         vector init constructor with vector copy or permute followed
> > > >         by a single scalar insert.
> > > >
> > > > gcc/testsuite/
> > > >
> > > >         PR tree-optimization/88828
> > > >         * gcc.target/i386/pr88828-1a.c: New test.
> > > >         * gcc.target/i386/pr88828-2b.c: Likewise.
> > > >         * gcc.target/i386/pr88828-2.c: Likewise.
> > > >         * gcc.target/i386/pr88828-3a.c: Likewise.
> > > >         * gcc.target/i386/pr88828-3b.c: Likewise.
> > > >         * gcc.target/i386/pr88828-3c.c: Likewise.
> > > >         * gcc.target/i386/pr88828-3d.c: Likewise.
> > > >         * gcc.target/i386/pr88828-4a.c: Likewise.
> > > >         * gcc.target/i386/pr88828-4b.c: Likewise.
> > > >         * gcc.target/i386/pr88828-5a.c: Likewise.
> > > >         * gcc.target/i386/pr88828-5b.c: Likewise.
> > > >         * gcc.target/i386/pr88828-6a.c: Likewise.
> > > >         * gcc.target/i386/pr88828-6b.c: Likewise.
> > >
> > > Here is the updated patch with run-time tests.
> >
> > -      if (TREE_CODE (elt->value) != SSA_NAME)
> > +      if (TREE_CODE (ce->value) != SSA_NAME)
> >         return false;
> >
> > hmm, so it doesn't allow { 0, v[1], v[2], v[3] }?  I think the single
> > scalar value can be a constant as well.
>
> Fixed.
>
> >        if (!def_stmt)
> > -       return false;
> > +       {
> > +         if (gimple_nop_p (SSA_NAME_DEF_STMT (ce->value)))
> >
> > if (SSA_NAME_IS_DEFAULT_DEF (ce->value))
> >
> > +           {
> >
> > also you seem to disallow
> >
> >   { i + 1, v[1], v[2], v[3] }
>
> Fixed by
>
>      if (code != BIT_FIELD_REF)
>         {
>           /* Only allow one scalar insert.  */
>           if (nscalars != 0)
>             return false;
>
>           nscalars = 1;
>           insert = true;
>           scalar_idx = i;
>           sel.quick_push (i);
>           scalar_element = ce->value;
>           continue;
>         }
>
> > because get_prop_source_stmt will return the definition computing
> > i + 1 in this case and your code will be skipped?
> >
> > I think you can simplify the code by treating scalar_element != NULL
> > as nscalars == 1 and eliding nscalars.
>
> It works only if
>
> TYPE_VECTOR_SUBPARTS (type).to_constant ()  == (nscalars + nvectors)
>
> We need to check both nscalars and nvectors.  Elide nscalar
> check doesn't help much here.
>
> > -      if (conv_code == ERROR_MARK)
> > +      if (conv_code == ERROR_MARK && !insert)
> >         gimple_assign_set_rhs_with_ops (gsi, VEC_PERM_EXPR, orig[0],
> >                                         orig[1], op2);
> >        else
> > @@ -2148,10 +2198,25 @@ simplify_vector_constructor (gimple_stmt_iterator *gsi)
> >                                    VEC_PERM_EXPR, orig[0], orig[1], op2);
> >           orig[0] = gimple_assign_lhs (perm);
> >           gsi_insert_before (gsi, perm, GSI_SAME_STMT);
> > -         gimple_assign_set_rhs_with_ops (gsi, conv_code, orig[0],
> > +         gimple_assign_set_rhs_with_ops (gsi,
> > +                                         (conv_code != ERROR_MARK
> > +                                          ? conv_code
> > +                                          : NOP_EXPR),
> > +                                         orig[0],
> >                                           NULL_TREE, NULL_TREE);
> >
> > I believe you should elide the last stmt for conv_code == ERROR_MARK,
> > that is, why did you need to add the && !insert check in the guarding condition
>
> When conv_code == ERROR_MARK, we still need
>
>        gimple *perm
>             = gimple_build_assign (make_ssa_name (TREE_TYPE (orig[0])),
>                                    VEC_PERM_EXPR, orig[0], orig[1], op2);
>           orig[0] = gimple_assign_lhs (perm);
>           gsi_insert_before (gsi, perm, GSI_SAME_STMT);
>           gimple_assign_set_rhs_with_ops (gsi,  NOP_EXPR,
>                                           orig[0],
>                                           NULL_TREE, NULL_TREE);
>
> Otherwise, scalar insert won't work.
>
> > (this path should already do the correct thing?).  Note that in all
> > cases it looks
> > that with conv_code != ERROR_MARK you may end up doing a float->int
> > or int->float conversion on a value it wasn't done on before which might
> > raise exceptions?  That is, do we need to make sure we permute a
> > value we already do convert into the place we're going to insert to?
>
> This couldn't happen:
>
>       if (type == TREE_TYPE (ref))
>          {
>            /* The RHS vector has the same type as LHS.  */
>            if (rhs_vector == NULL)
>              rhs_vector = ref;
>            /* Check if all RHS vector elements come fome the same
>               vector.  */
>            if (rhs_vector == ref)
>              nvectors++;
>          }
> ...
>   if (insert
>       && (nvectors == 0
>           || (TYPE_VECTOR_SUBPARTS (type).to_constant ()
>               != (nscalars + nvectors))))
>     return false;
>
> > +  if (insert)
> > +    {
> > +      /* Generate a single scalar insert.  */
> > +      tree var = make_ssa_name (type);
> > +      tree val = gimple_assign_rhs1 (stmt);
> > +      gimple *copy = gimple_build_assign (var, val);
> >
> > I believe this doesn't properly copy the stmt in case it is a permute.
> > You can use (note the use of gsi_stmt - gimple_assign_set_rhs_with_ops
> > can re-allocate the stmt)
> >
> >         gimple *copy = gimple_copy (gsi_stmt (*gsi));
> >         gimple_assign_set_lhs (copy, var);
>
> Fixed.
>
> > +      gsi_insert_before (gsi, copy, GSI_SAME_STMT);
> > +      tree bitpos = bitsize_int (scalar_idx * elem_size);
> > +      gimple_assign_set_rhs_with_ops (gsi, BIT_INSERT_EXPR, var,
> > +                                     scalar_element, bitpos);
> > +    }
> >
> > Otherwise looks OK to me.
> >
> > As separate followup patch it might be interesting to support
> >
> >  { 0, a[1], a[2], 3 }
> >
> > kinds as well, thus combining a VECTOR_CST (which is
> > reasonably cheap to create) with another vector.  That should
> > be maybe done as a first patch given this is just a two-vector
> > permute which the code already handles apart from not
> > recognizing the implicit constant vector participating.
> >
> > Similar
> >
> >  { 0, a[1], b[2], 3 }
> >
> > where the combination of a and b is blended with another
> > constant vector.  I'm not sure if handling an arbitrary number
> > of scalar elements should be done in a similar way, that is,
> > implementing
> >
> >  { s1, a[1], a[2], s2, s3, b[0], b[1], b[2] }
> >
> > as
> >
> >   tem = VEC_PERM <a, b, { ... }>
> >   tem2 = { s1, 0, 0, s2, s3, 0, 0, 0 }
> >   res = VEC_PERM <tem, tem2, { blend-mask }>
> >
> > where constructing tem2 should take at most
> > N-1 inserts (the first element to insert into tem2
> > can use a splat or if element zero a zero-extending move).
> >
> > Doing this effectively lifts the restriction of only
> > handling two vectors - we'd incrementally do
> > two-vector permute plus blend of the rest which has
> > its constructor re-processed.
> >
> > But as said - the code is already a bit awkward so changing
> > this in multiple reivisions is preferred and the single-element
> > case is certainly sth to do via a BIT_INSERT_EXPR.
>
> Agreed.
>
> I am testing this updated patch.
>

This is the fully tested patch.  I used

+
+      if (useless_type_conversion_p (type, TREE_TYPE (ref)))
+ {
+    /* The RHS vector has the same type as LHS.  */
+    if (rhs_vector == NULL)
+      rhs_vector = ref;
+    /* Check if all RHS vector elements come fome the same
+       vector.  */
+    if (rhs_vector == ref)
+      nvectors++;
+ }
+

to support cast between __v4sf and __m128.

Message ID	CAMe9rOpQehpx=sxjvE1wgZYd+NrQUaVn0eEPgDcr5TuPTEjj5w@mail.gmail.com
State	New
Headers	show Return-Path: <gcc-patches-return-497547-incoming=patchwork.ozlabs.org@gcc.gnu.org> DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:references:in-reply-to:from:date:message-id :subject:to:cc:content-type; q=dns; s=default; b=fwPX5D8JX2nCROd 2x0jXNn5rO7oW2Fd30ZLsuXOfGUSz/qI4aQK1ht+0Yif6HH1DqOsBCEMwa/96Prz z4YjMDpQIfRM0rN888SX/+o6+7Yn411LhmfPEvl6lk0YlWBwV7XugcLqf4YUynsS 9bUQvfSTJ3aUkRO6puePqG0+O9pU= Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk Sender: gcc-patches-owner@gcc.gnu.org MIME-Version: 1.0 References: <20190303143230.19742-1-hjl.tools@gmail.com> <CA+=Sn1mpV+MX6u5ERi_4yjG1+pr2EBh8ft-2QJSUBGKWPY6O7w@mail.gmail.com> <20190303211325.GA24057@gmail.com> <CAFiYyc17ECEUCXzV4-rVupBjwWhCuVQfTRGgmGtt1uThh12Prg@mail.gmail.com> <20190304174625.GA25613@gmail.com> <CAMe9rOrCAJv663ontj0sBtn7p1vuH0i-+mmcLyzzFv0xszfLJA@mail.gmail.com> <CAFiYyc2d0zaOMK1nAqvwvGxaiYTHjN56S21rRrsE=_10TG6Dqg@mail.gmail.com> <CAMe9rOrnfZVr=6ZEiNvgCZVhUOcbt0G2Ob_-QyXpj_u8-qzJpA@mail.gmail.com> In-Reply-To: <CAMe9rOrnfZVr=6ZEiNvgCZVhUOcbt0G2Ob_-QyXpj_u8-qzJpA@mail.gmail.com> From: "H.J. Lu" <hjl.tools@gmail.com> Date: Fri, 8 Mar 2019 16:48:43 +0800 Message-ID: <CAMe9rOpQehpx=sxjvE1wgZYd+NrQUaVn0eEPgDcr5TuPTEjj5w@mail.gmail.com> Subject: V5 [PATCH] Optimize vector constructor To: Richard Biener <richard.guenther@gmail.com> Cc: Hongtao Liu <crazylht@gmail.com>, Andrew Pinski <pinskia@gmail.com>, GCC Patches <gcc-patches@gcc.gnu.org> Content-Type: multipart/mixed; boundary="00000000000070cf710583914c42"
Series	V5 [PATCH] Optimize vector constructor \| expand V5 [PATCH] Optimize vector constructor

V5 [PATCH] Optimize vector constructor

Commit Message

Comments

Patch