On Mittwoch, 9. Mai 2018 11:08:02 CEST Jakub Jelinek wrote:
> On Tue, May 08, 2018 at 01:25:35PM +0200, Allan Sandfeld Jensen wrote:
> > 2018-05-08 Allan Sandfeld Jensen <allan.jen...@qt.io>
>
> 2 spaces between date and name and two spaces between name and email
> address.
>
> > gcc/
> >
> > PR tree-optimization/85692
> > * tree-ssa-forwprop.c (simplify_vector_constructor): Try two
> > source permute as well.
> >
> > gcc/testsuite
> >
> > * gcc.target/i386/pr85692.c: Test two simply constructions are
> > detected as permute instructions.
>
> Just
> * gcc.target/i386/pr85692.c: New test.
>
> > diff --git a/gcc/testsuite/gcc.target/i386/pr85692.c
> > b/gcc/testsuite/gcc.target/i386/pr85692.c new file mode 100644
> > index 00000000000..322c1050161
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/i386/pr85692.c
> > @@ -0,0 +1,18 @@
> > +/* { dg-do run } */
> > +/* { dg-options "-O2 -msse4.1" } */
> > +/* { dg-final { scan-assembler "unpcklps" } } */
> > +/* { dg-final { scan-assembler "blendps" } } */
> > +/* { dg-final { scan-assembler-not "shufps" } } */
> > +/* { dg-final { scan-assembler-not "unpckhps" } } */
> > +
> > +typedef float v4sf __attribute__ ((vector_size (16)));
> > +
> > +v4sf unpcklps(v4sf a, v4sf b)
> > +{
> > + return v4sf{a[0],b[0],a[1],b[1]};
>
> Though, not really sure if this has been tested at all.
> The above is valid only in C++ (and only C++11 and above), while the
> test is compiled as C and thus has to fail.
>
Yes, I thought it had been tested, but it wasn't. It also needs to change the
first line to be a compile and not run test.
> > @@ -2022,8 +2022,9 @@ simplify_vector_constructor (gimple_stmt_iterator
> > *gsi)>
> > elem_type = TREE_TYPE (type);
> > elem_size = TREE_INT_CST_LOW (TYPE_SIZE (elem_type));
> >
> > - vec_perm_builder sel (nelts, nelts, 1);
> > - orig = NULL;
> > + vec_perm_builder sel (nelts, 2, nelts);
>
> Why this change? I admit the vec_parm_builder arguments are confusing, but
> I think the second times third is the number of how many indices are being
> pushed into the vector, so I think (nelts, nelts, 1) is right.
>
I had the impression it was what was selected from. In any case, I changed it
because without I get crash when vec_perm_indices is created later with a
possible nparms of 2.
> > @@ -2063,10 +2064,26 @@ simplify_vector_constructor (gimple_stmt_iterator
> > *gsi)>
> > return false;
> >
> > op1 = gimple_assign_rhs1 (def_stmt);
> > ref = TREE_OPERAND (op1, 0);
> >
> > - if (orig)
> > + if (orig1)
> >
> > {
> >
> > - if (ref != orig)
> > - return false;
> > + if (ref == orig1 || orig2)
> > + {
> > + if (ref != orig1 && ref != orig2)
> > + return false;
> > + }
> > + else
> > + {
> > + if (TREE_CODE (ref) != SSA_NAME)
> > + return false;
> > + if (! VECTOR_TYPE_P (TREE_TYPE (ref))
> > + || ! useless_type_conversion_p (TREE_TYPE (op1),
> > + TREE_TYPE (TREE_TYPE (ref))))
> > + return false;
> > + if (TREE_TYPE (orig1) != TREE_TYPE (ref))
> > + return false;
>
> I think even different type is acceptable here, as long as its conversion to
> orig1's type is useless.
>
> Furthermore, I think the way you wrote the patch with 2 variables rather
> than an array of 2 elements means too much duplication, this else block
> is a duplication of the else block below. See the patch I've added to the
> PR
It seemed to me it was clearer like this, but I can see your point.
> (and sorry for missing your patch first, the PR wasn't ASSIGNED and there
> was no link to gcc-patches for it).
>
It is okay. You are welcome to take it over. I am not a regular gcc
contributor and thus not well-versed in the details, only the basic logic of
how things work.
'Allan
