| Message ID | 1398111582.659.3.camel@gnopaine |
|---|---|
| State | New |
| Headers | show |
On 04/21/2014 01:19 PM, Bill Schmidt wrote: > + if (GET_CODE (trueop0) == VEC_SELECT > + && GET_MODE (XEXP (trueop0, 0)) == mode) > + { > + rtx op0_subop1 = XEXP (trueop0, 1); > + gcc_assert (GET_CODE (op0_subop1) == PARALLEL); > + gcc_assert (XVECLEN (trueop1, 0) == GET_MODE_NUNITS (mode)); > + > + /* Apply the outer ordering vector to the inner one. (The inner > + ordering vector is expressly permitted to be of a different > + length than the outer one.) If the result is { 0, 1, ..., n-1 } > + then the two VEC_SELECTs cancel. */ > + for (int i = 0; i < XVECLEN (trueop1, 0); ++i) > + { > + rtx x = XVECEXP (trueop1, 0, i); > + gcc_assert (CONST_INT_P (x)); > + rtx y = XVECEXP (op0_subop1, 0, INTVAL (x)); > + gcc_assert (CONST_INT_P (y)); In two places you're asserting that you've got a constant permutation. Surely there should be a non-assertion check and graceful exit for either select to be a variable permutation. r~
On Mon, 2014-04-21 at 13:48 -0700, Richard Henderson wrote: > On 04/21/2014 01:19 PM, Bill Schmidt wrote: > > + if (GET_CODE (trueop0) == VEC_SELECT > > + && GET_MODE (XEXP (trueop0, 0)) == mode) > > + { > > + rtx op0_subop1 = XEXP (trueop0, 1); > > + gcc_assert (GET_CODE (op0_subop1) == PARALLEL); > > + gcc_assert (XVECLEN (trueop1, 0) == GET_MODE_NUNITS (mode)); > > + > > + /* Apply the outer ordering vector to the inner one. (The inner > > + ordering vector is expressly permitted to be of a different > > + length than the outer one.) If the result is { 0, 1, ..., n-1 } > > + then the two VEC_SELECTs cancel. */ > > + for (int i = 0; i < XVECLEN (trueop1, 0); ++i) > > + { > > + rtx x = XVECEXP (trueop1, 0, i); > > + gcc_assert (CONST_INT_P (x)); > > + rtx y = XVECEXP (op0_subop1, 0, INTVAL (x)); > > + gcc_assert (CONST_INT_P (y)); > > In two places you're asserting that you've got a constant permutation. Surely > there should be a non-assertion check and graceful exit for either select to be > a variable permutation. Ah. I was not aware this was even a possibility. From rtl.def: /* Describes an operation that selects parts of a vector. Operands 0 is the source vector, operand 1 is a PARALLEL that contains a CONST_INT for each of the subparts of the result vector, giving the number of the source subpart that should be stored into it. */ DEF_RTL_EXPR(VEC_SELECT, "vec_select", "ee", RTX_BIN_ARITH) If variable permutations are possible with VEC_SELECT, then I suppose this commentary should be updated. The GCC internals document contains similar text in section 13.12. I'll rebuild the patch per your comments tomorrow. Thanks for letting me know about this! Regards, Bill > > > r~ >
On Mon, 21 Apr 2014, Richard Henderson wrote: > On 04/21/2014 01:19 PM, Bill Schmidt wrote: >> + if (GET_CODE (trueop0) == VEC_SELECT >> + && GET_MODE (XEXP (trueop0, 0)) == mode) >> + { >> + rtx op0_subop1 = XEXP (trueop0, 1); >> + gcc_assert (GET_CODE (op0_subop1) == PARALLEL); >> + gcc_assert (XVECLEN (trueop1, 0) == GET_MODE_NUNITS (mode)); >> + >> + /* Apply the outer ordering vector to the inner one. (The inner >> + ordering vector is expressly permitted to be of a different >> + length than the outer one.) If the result is { 0, 1, ..., n-1 } >> + then the two VEC_SELECTs cancel. */ >> + for (int i = 0; i < XVECLEN (trueop1, 0); ++i) >> + { >> + rtx x = XVECEXP (trueop1, 0, i); >> + gcc_assert (CONST_INT_P (x)); >> + rtx y = XVECEXP (op0_subop1, 0, INTVAL (x)); >> + gcc_assert (CONST_INT_P (y)); > > In two places you're asserting that you've got a constant permutation. Surely > there should be a non-assertion check and graceful exit for either select to be > a variable permutation. Note that in the case where trueop0 is a CONST_VECTOR, we already check each element of trueop1: gcc_assert (CONST_INT_P (x)); In the case where the result is a scalar, we also have: gcc_assert (CONST_INT_P (XVECEXP (trueop1, 0, 0))); so we will have other issues if something ever creates a variable vec_select. Not that a graceful exit will hurt of course.
Marc Glisse <marc.glisse@inria.fr> writes: > On Mon, 21 Apr 2014, Richard Henderson wrote: > >> On 04/21/2014 01:19 PM, Bill Schmidt wrote: >>> + if (GET_CODE (trueop0) == VEC_SELECT >>> + && GET_MODE (XEXP (trueop0, 0)) == mode) >>> + { >>> + rtx op0_subop1 = XEXP (trueop0, 1); >>> + gcc_assert (GET_CODE (op0_subop1) == PARALLEL); >>> + gcc_assert (XVECLEN (trueop1, 0) == GET_MODE_NUNITS (mode)); >>> + >>> + /* Apply the outer ordering vector to the inner one. (The inner >>> + ordering vector is expressly permitted to be of a different >>> + length than the outer one.) If the result is { 0, 1, ..., n-1 } >>> + then the two VEC_SELECTs cancel. */ >>> + for (int i = 0; i < XVECLEN (trueop1, 0); ++i) >>> + { >>> + rtx x = XVECEXP (trueop1, 0, i); >>> + gcc_assert (CONST_INT_P (x)); >>> + rtx y = XVECEXP (op0_subop1, 0, INTVAL (x)); >>> + gcc_assert (CONST_INT_P (y)); >> >> In two places you're asserting that you've got a constant permutation. Surely >> there should be a non-assertion check and graceful exit for either >> select to be >> a variable permutation. > > Note that in the case where trueop0 is a CONST_VECTOR, we already check > each element of trueop1: > > gcc_assert (CONST_INT_P (x)); > > In the case where the result is a scalar, we also have: > > gcc_assert (CONST_INT_P (XVECEXP (trueop1, 0, 0))); > > so we will have other issues if something ever creates a variable > vec_select. Not that a graceful exit will hurt of course. I realise this isn't the point, but maybe we should go easy on this kind of gcc_assert. Using INTVAL is itself an assertion that you have a CONST_INT. Adding gcc_asserts on top (and so forcing the assert even in release compilers) kind-of subverts the --enable-checking option. Thanks, Richard
Index: gcc/simplify-rtx.c =================================================================== --- gcc/simplify-rtx.c (revision 209516) +++ gcc/simplify-rtx.c (working copy) @@ -3673,6 +3673,31 @@ simplify_binary_operation_1 (enum rtx_code code, e } } + /* If we have two nested selects that are inverses of each + other, replace them with the source operand. */ + if (GET_CODE (trueop0) == VEC_SELECT + && GET_MODE (XEXP (trueop0, 0)) == mode) + { + rtx op0_subop1 = XEXP (trueop0, 1); + gcc_assert (GET_CODE (op0_subop1) == PARALLEL); + gcc_assert (XVECLEN (trueop1, 0) == GET_MODE_NUNITS (mode)); + + /* Apply the outer ordering vector to the inner one. (The inner + ordering vector is expressly permitted to be of a different + length than the outer one.) If the result is { 0, 1, ..., n-1 } + then the two VEC_SELECTs cancel. */ + for (int i = 0; i < XVECLEN (trueop1, 0); ++i) + { + rtx x = XVECEXP (trueop1, 0, i); + gcc_assert (CONST_INT_P (x)); + rtx y = XVECEXP (op0_subop1, 0, INTVAL (x)); + gcc_assert (CONST_INT_P (y)); + if (i != INTVAL (y)) + return 0; + } + return XEXP (trueop0, 0); + } + return 0; case VEC_CONCAT: { Index: gcc/testsuite/gcc.target/powerpc/vsxcopy.c =================================================================== --- gcc/testsuite/gcc.target/powerpc/vsxcopy.c (revision 0) +++ gcc/testsuite/gcc.target/powerpc/vsxcopy.c (working copy) @@ -0,0 +1,15 @@ +/* { dg-do compile { target { powerpc64*-*-* } } } */ +/* { dg-require-effective-target powerpc_vsx_ok } */ +/* { dg-options "-O1" } */ +/* { dg-final { scan-assembler "lxvd2x" } } */ +/* { dg-final { scan-assembler "stxvd2x" } } */ +/* { dg-final { scan-assembler-not "xxpermdi" } } */ + +typedef float vecf __attribute__ ((vector_size (16))); +extern vecf j, k; + +void fun (void) +{ + j = k; +} +