On Wed, May 8, 2019 at 2:04 PM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> On Fri, May 3, 2019 at 6:54 PM H.J. Lu <hjl.to...@gmail.com> wrote:
> >
> > On Thu, May 2, 2019 at 10:53 AM H.J. Lu <hjl.to...@gmail.com> wrote:
> > >
> > > On Thu, May 2, 2019 at 7:55 AM Richard Biener
> > > <richard.guent...@gmail.com> wrote:
> > > >
> > > > On Thu, May 2, 2019 at 4:54 PM Richard Biener
> > > > <richard.guent...@gmail.com> wrote:
> > > > >
> > > > > On Mon, Mar 11, 2019 at 8:03 AM H.J. Lu <hjl.to...@gmail.com> wrote:
> > > > > >
> > > > > > On Fri, Mar 8, 2019 at 7:03 PM Richard Biener
> > > > > > <richard.guent...@gmail.com> wrote:
> > > > > > >
> > > > > > > On Fri, Mar 8, 2019 at 9:49 AM H.J. Lu <hjl.to...@gmail.com>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > On Thu, Mar 7, 2019 at 9:51 AM H.J. Lu <hjl.to...@gmail.com>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > On Wed, Mar 6, 2019 at 8:33 PM Richard Biener
> > > > > > > > > <richard.guent...@gmail.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On Wed, Mar 6, 2019 at 8:46 AM H.J. Lu
> > > > > > > > > > <hjl.to...@gmail.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Tue, Mar 5, 2019 at 1:46 AM H.J. Lu
> > > > > > > > > > > <hjl.to...@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.to...@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.to...@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;
> > > > > > >
> > > > > > > I see - that looks like a missed case then?
> > > > > > >
> > > > > > > { 1., (float)v[1], (float)v[2], (float)v[3] }
> > > > > > >
> > > > > > > with integer vector v?
> > > > > >
> > > > > > True.
> > > > > >
> > > > > > > I'll have a look at the full patch next week (it's GCC 10
> > > > > > > material in any case).
> > > > > > >
> > > > >
> > > > > Now looking again. I still don't like the new "structure" of the loop
> > > > > very much.
> > > > > A refactoring like the attached should make it easier to clearly
> > > > > separate the
> > > > > cases where we reach a vector def and where not.
> > > >
> > > > Now attached.
> > > >
> > > > > Do you want me to take over the patch?
> > > > >
> > >
> >
> > Here is the updated patch on top of your patch plus my fix.
>
> Thanks - when doing the constant vector I was thinking of the following patch
> to handle your cases. It doesn't use insertion but eventually leaves that
> to a separate transform. Instead it handles non-constants similar to
> constants
> by permuting from a uniform vector. Thus
>
> __attribute__((noinline, noclone))
> __v4sf
> foo2 (__v4sf x, float f)
> {
> __v4sf y = { f, x[1], x[2], x[3] };
> return y;
> }
>
> becomes
>
> _4 = {f_2(D), f_2(D), f_2(D), f_2(D)};
> y_3 = VEC_PERM_EXPR <x_5(D), _4, { 4, 1, 2, 3 }>;
> return y_3;
>
> this allows us to handle an arbitrary number of inserts of this
> single value. It also ensures we can actually perform the
> permutation while for the insertion we currently do not have
> a convenient way to query whether the target can perform
> it efficiently (IIRC x86 needs AVX to insert to arbitrary lanes
> with a single instruction?). Similarly if the user writes the above
> in source using __builtin_shuffle we'd want to optimize it as well.
>
> The patch as attached only passes some of your testcases,
> the following FAIL:
>
> FAIL: gcc.target/i386/pr88828-2a.c scan-assembler movss
> FAIL: gcc.target/i386/pr88828-2a.c scan-assembler-not movaps
> FAIL: gcc.target/i386/pr88828-2a.c scan-assembler-not movlhps
> FAIL: gcc.target/i386/pr88828-2a.c scan-assembler-not unpcklps
> FAIL: gcc.target/i386/pr88828-2b.c scan-assembler-times vpermilps 1
> FAIL: gcc.target/i386/pr88828-2b.c scan-assembler-times vmovss 1
> FAIL: gcc.target/i386/pr88828-2c.c scan-assembler movss
> FAIL: gcc.target/i386/pr88828-2c.c scan-assembler-not movaps
> FAIL: gcc.target/i386/pr88828-2c.c scan-assembler-not movlhps
> FAIL: gcc.target/i386/pr88828-2c.c scan-assembler-not unpcklps
> FAIL: gcc.target/i386/pr88828-2d.c scan-assembler-times vpermilps 1
> FAIL: gcc.target/i386/pr88828-2d.c scan-assembler-times vmovss 1
> FAIL: gcc.target/i386/pr88828-3a.c scan-assembler movss
> FAIL: gcc.target/i386/pr88828-3a.c scan-assembler-times shufps 2
> FAIL: gcc.target/i386/pr88828-3a.c scan-assembler-times movaps 1
> FAIL: gcc.target/i386/pr88828-3a.c scan-assembler-not movlhps
> FAIL: gcc.target/i386/pr88828-3a.c scan-assembler-not unpcklps
> FAIL: gcc.target/i386/pr88828-3b.c scan-assembler-times vpermilps 1
> FAIL: gcc.target/i386/pr88828-3b.c scan-assembler-times vinsertps 1
> FAIL: gcc.target/i386/pr88828-3b.c scan-assembler-not vshufps
> FAIL: gcc.target/i386/pr88828-3c.c scan-assembler-times vpermilps 1
> FAIL: gcc.target/i386/pr88828-3c.c scan-assembler-times vinsertps 1
> FAIL: gcc.target/i386/pr88828-3c.c scan-assembler-not vshufps
>
> Making the patch emit inserts for single insert locations is of course
> still possible but you get to arrive at heuristics like your choice
> of permuting the original lane into the later overwritten lane which
> might be a choice making the permute impossible or more expensive?
>
> The original purpose of simplify_vector_constructor was to simplify
> the IL, not so much optimal code-generation in the end but I wonder
> if we can rely on RTL expansion or later RTL optimization to do
> the optimal choices here?
>
> I guess simplify_permutation could perform a VEC_PERM
> into an insert if the remaining permutation would be a no-op
> but RTL optimization handles this case well already.
>
> Whether code-generation for a one vector permute plus insert or
> a two-vector permute is better in the end I don't know - at least
> the permute expansion has a chance to see the combined
> instruction.
>
> Do you think the remaining cases above can be handled in the
> backend?
>
> Comments?
I have now applied this after bootstrap / regtest on x86_64-unknown-linux-gnu
together with the part of the testcases that PASS. Note I had to
add -fexcess-precision=standard to the -7.c one as it otherwise fails to
execute both patched and unpatched.
r271153.
I didn't check whether the remaining testcases simply need adjustments
(thus their code-gen is OK) or if there's something to do on the target
or in a GIMPLE transform. That needs to be evaluated still.
There's also still the missed optimization of using VEC_UNPACK/PACK
codes for conversions.
Richard.
> Thanks,
> Richard.
>
> 2019-05-08 Richard Biener <rguent...@suse.de>
>
> PR tree-optimization/88828
> * tree-ssa-forwprop.c (simplify_vector_constructor): Handle
> permuting in a single non-constant element not extracted
> from a vector.
>
>
> > --
> > H.J.
