On Wed, Oct 01, 2014 at 08:26:27AM +0200, Uros Bizjak wrote: > On Wed, Oct 1, 2014 at 12:13 AM, Evgeny Stupachenko <evstu...@gmail.com> > wrote: > > expand_vselect for some reason ignores the expander. > > Does it work with expanders? > > The comment talks about insn only: > > /* Construct (set target (vec_select op0 (parallel perm))) and > > return true if that's a valid instruction in the active ISA. */ > > It looks to me that the whole approach is wrong from the beginning. > > There is already a function that generates perm/palignr sequence, > conveniently named expand_vec_perm_palignr. This function should be > extended to handle AVX2 sequence. You don't have to add any new > patterns, existing avx2_permv2ti and avx2_palignr2ti should do the > trick.
Yeah, I'd expect something like (so far not even compile tested): --- gcc/config/i386/i386.c 2014-09-26 10:33:18.789645102 +0200 +++ gcc/config/i386/i386.c 2014-10-01 09:27:41.435813522 +0200 @@ -43297,44 +43297,79 @@ expand_vec_perm_palignr (struct expand_v rtx shift, target; struct expand_vec_perm_d dcopy; - /* Even with AVX, palignr only operates on 128-bit vectors. */ - if (!TARGET_SSSE3 || GET_MODE_SIZE (d->vmode) != 16) + /* palignr doesn't help for one_operand_p case. */ + if (d->one_operand_p) return false; - min = nelt, max = 0; + /* Even with AVX, palignr only operates on 128-bit vectors, + in AVX2 palignr operates on both 128-bit lanes. */ + if ((!TARGET_SSSE3 || GET_MODE_SIZE (d->vmode) != 16) + && (!TARGET_AVX2 || GET_MODE_SIZE (d->vmode) != 32)) + return false; + + min = 2 * nelt, max = 0; for (i = 0; i < nelt; ++i) { unsigned e = d->perm[i]; + if (GET_MODE_SIZE (d->vmode) == 32) + e = (e & ((nelt / 2) - 1)) | ((e & nelt) >> 1); if (e < min) min = e; if (e > max) max = e; } - if (min == 0 || max - min >= nelt) + if (min == 0 + || max - min >= (GET_MODE_SIZE (d->vmode) == 32 ? nelt / 2 : nelt)) return false; /* Given that we have SSSE3, we know we'll be able to implement the - single operand permutation after the palignr with pshufb. */ - if (d->testing_p) + single operand permutation after the palignr with pshufb for + 128-bit vectors. */ + if (d->testing_p && GET_MODE_SIZE (d->vmode) == 16) return true; dcopy = *d; - shift = GEN_INT (min * GET_MODE_BITSIZE (GET_MODE_INNER (d->vmode))); - target = gen_reg_rtx (TImode); - emit_insn (gen_ssse3_palignrti (target, gen_lowpart (TImode, d->op1), - gen_lowpart (TImode, d->op0), shift)); - - dcopy.op0 = dcopy.op1 = gen_lowpart (d->vmode, target); - dcopy.one_operand_p = true; in_order = true; for (i = 0; i < nelt; ++i) { - unsigned e = dcopy.perm[i] - min; + unsigned e = dcopy.perm[i]; + if (GET_MODE_SIZE (d->vmode) == 32 + && e >= nelt + && (e & (nelt / 2 - 1)) < min) + e = e - min - (nelt / 2); + else + e = e - min; if (e != i) in_order = false; dcopy.perm[i] = e; } + dcopy.one_operand_p = true; + + /* For AVX2, test whether we can permute the result in one instruction. */ + if (d->testing_p) + { + if (in_order) + return true; + dcopy.op1 = dcopy.op0; + return expand_vec_perm_1 (&dcopy); + } + + shift = GEN_INT (min * GET_MODE_BITSIZE (GET_MODE_INNER (d->vmode))); + if (GET_MODE_SIZE (d->vmode) == 16) + { + target = gen_reg_rtx (TImode); + emit_insn (gen_ssse3_palignrti (target, gen_lowpart (TImode, d->op1), + gen_lowpart (TImode, d->op0), shift)); + } + else + { + target = gen_reg_rtx (V2TImode); + emit_insn (gen_avx2_palignr2vti (target, gen_lowpart (V2TImode, d->op1), + gen_lowpart (V2TImode, d->op0), shift)); + } + + dcopy.op0 = dcopy.op1 = gen_lowpart (d->vmode, target); /* Test for the degenerate case where the alignment by itself produces the desired permutation. */ > And, as said by H.J., please add the testcase from the PR that will > exercise the code path. Without the testcase included, the patch is > unreviewable, and this is the reason why no maintainer (including me) > wants to approve it in its current form. I'd hope my gcc.dg/torture/vshuf* tests catch it, if not in normal mode, at least in the expensive one. OT, note, those tests haven't been extended for AVX-512 modes (for most modes it is trivial, just V64QI will need new vshuf-64.inc). Jakub