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