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? 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.
fix-pr88828-2
Description: Binary data