Re: expansion of vector shifts...
On Tue, Feb 12, 2013 at 11:31 PM, David Miller da...@davemloft.net wrote: From: David Miller da...@redhat.com Date: Fri, 16 Nov 2012 00:33:05 -0500 (EST) From: Richard Sandiford rdsandif...@googlemail.com Date: Mon, 29 Oct 2012 10:14:53 + ...given that the code is like you say written: if (SHIFT_COUNT_TRUNCATED) { if (CONST_INT_P (op1) ... else if (GET_CODE (op1) == SUBREG subreg_lowpart_p (op1) INTEGRAL_MODE_P (GET_MODE (SUBREG_REG (op1 op1 = SUBREG_REG (op1); } INTEGRAL_MODE_P (GET_MODE (op1)) might be better than an explicit VECTOR_MODE_P check. The code really doesn't make sense for anything other than integers. (It amounts to the same thing in practice, of course...) Agreed, I've just committed the following. Thanks! Fix gcc.c-torture/compile/pr53410-2.c on sparc. * expmed.c (expand_shift_1): Don't strip non-integral SUBREGs. This is broken on sparc again, although I'm confused about how this has happened. The suggestion was to use INTEGRAL_MODE_P as the test, so what's there in expand_shift_1() is: else if (GET_CODE (op1) == SUBREG subreg_lowpart_p (op1) INTEGRAL_MODE_P (GET_MODE (SUBREG_REG (op1))) INTEGRAL_MODE_P (GET_MODE (op1))) op1 = SUBREG_REG (op1); but INTEGRAL_MODE_P accepts vectors. This is really confusing because I was absolutely sure I re-ran the test case with the fix I committed and it didn't crash any more. Maybe what we really mean to do here is check both op1 and SUBREG_REG (op1) against SCALAR_INT_MODE_P instead of INTEGRAL_MODE_P? Yes. Something like this: gcc/ 2013-02-12 David S. Miller da...@davemloft.net * expmed.c (expand_shift_1): Only strip scalar integer subregs. diff --git a/gcc/expmed.c b/gcc/expmed.c index 4a6ddb0..954a360 100644 --- a/gcc/expmed.c +++ b/gcc/expmed.c @@ -2116,8 +2116,8 @@ expand_shift_1 (enum tree_code code, enum machine_mode mode, rtx shifted, % GET_MODE_BITSIZE (mode)); else if (GET_CODE (op1) == SUBREG subreg_lowpart_p (op1) - INTEGRAL_MODE_P (GET_MODE (SUBREG_REG (op1))) - INTEGRAL_MODE_P (GET_MODE (op1))) + SCALAR_INT_MODE_P (GET_MODE (SUBREG_REG (op1))) + SCALAR_INT_MODE_P (GET_MODE (op1))) op1 = SUBREG_REG (op1); }
Re: expansion of vector shifts...
From: Richard Biener richard.guent...@gmail.com Date: Wed, 13 Feb 2013 12:15:13 +0100 On Tue, Feb 12, 2013 at 11:31 PM, David Miller da...@davemloft.net wrote: Maybe what we really mean to do here is check both op1 and SUBREG_REG (op1) against SCALAR_INT_MODE_P instead of INTEGRAL_MODE_P? Yes. Ok, I'll commit this after doing some regstraps, thanks. Something like this: gcc/ 2013-02-12 David S. Miller da...@davemloft.net * expmed.c (expand_shift_1): Only strip scalar integer subregs. diff --git a/gcc/expmed.c b/gcc/expmed.c index 4a6ddb0..954a360 100644 --- a/gcc/expmed.c +++ b/gcc/expmed.c @@ -2116,8 +2116,8 @@ expand_shift_1 (enum tree_code code, enum machine_mode mode, rtx shifted, % GET_MODE_BITSIZE (mode)); else if (GET_CODE (op1) == SUBREG subreg_lowpart_p (op1) - INTEGRAL_MODE_P (GET_MODE (SUBREG_REG (op1))) - INTEGRAL_MODE_P (GET_MODE (op1))) + SCALAR_INT_MODE_P (GET_MODE (SUBREG_REG (op1))) + SCALAR_INT_MODE_P (GET_MODE (op1))) op1 = SUBREG_REG (op1); }
Re: expansion of vector shifts...
On Tue, Feb 12, 2013 at 11:31 PM, David Miller da...@davemloft.net wrote: From: David Miller da...@redhat.com Date: Fri, 16 Nov 2012 00:33:05 -0500 (EST) From: Richard Sandiford rdsandif...@googlemail.com Date: Mon, 29 Oct 2012 10:14:53 + ...given that the code is like you say written: if (SHIFT_COUNT_TRUNCATED) { if (CONST_INT_P (op1) ... else if (GET_CODE (op1) == SUBREG subreg_lowpart_p (op1) INTEGRAL_MODE_P (GET_MODE (SUBREG_REG (op1 op1 = SUBREG_REG (op1); } INTEGRAL_MODE_P (GET_MODE (op1)) might be better than an explicit VECTOR_MODE_P check. The code really doesn't make sense for anything other than integers. (It amounts to the same thing in practice, of course...) Agreed, I've just committed the following. Thanks! Fix gcc.c-torture/compile/pr53410-2.c on sparc. * expmed.c (expand_shift_1): Don't strip non-integral SUBREGs. This is broken on sparc again, although I'm confused about how this has happened. The suggestion was to use INTEGRAL_MODE_P as the test, so what's there in expand_shift_1() is: else if (GET_CODE (op1) == SUBREG subreg_lowpart_p (op1) INTEGRAL_MODE_P (GET_MODE (SUBREG_REG (op1))) INTEGRAL_MODE_P (GET_MODE (op1))) op1 = SUBREG_REG (op1); but INTEGRAL_MODE_P accepts vectors. This is really confusing because I was absolutely sure I re-ran the test case with the fix I committed and it didn't crash any more. Maybe what we really mean to do here is check both op1 and SUBREG_REG (op1) against SCALAR_INT_MODE_P instead of INTEGRAL_MODE_P? Yes. Something like this: gcc/ 2013-02-12 David S. Miller da...@davemloft.net * expmed.c (expand_shift_1): Only strip scalar integer subregs. diff --git a/gcc/expmed.c b/gcc/expmed.c index 4a6ddb0..954a360 100644 --- a/gcc/expmed.c +++ b/gcc/expmed.c @@ -2116,8 +2116,8 @@ expand_shift_1 (enum tree_code code, enum machine_mode mode, rtx shifted, % GET_MODE_BITSIZE (mode)); else if (GET_CODE (op1) == SUBREG subreg_lowpart_p (op1) - INTEGRAL_MODE_P (GET_MODE (SUBREG_REG (op1))) - INTEGRAL_MODE_P (GET_MODE (op1))) + SCALAR_INT_MODE_P (GET_MODE (SUBREG_REG (op1))) + SCALAR_INT_MODE_P (GET_MODE (op1))) op1 = SUBREG_REG (op1); }
Re: expansion of vector shifts...
From: Richard Biener richard.guent...@gmail.com Date: Wed, 13 Feb 2013 12:15:13 +0100 On Tue, Feb 12, 2013 at 11:31 PM, David Miller da...@davemloft.net wrote: Maybe what we really mean to do here is check both op1 and SUBREG_REG (op1) against SCALAR_INT_MODE_P instead of INTEGRAL_MODE_P? Yes. Ok, I'll commit this after doing some regstraps, thanks. Something like this: gcc/ 2013-02-12 David S. Miller da...@davemloft.net * expmed.c (expand_shift_1): Only strip scalar integer subregs. diff --git a/gcc/expmed.c b/gcc/expmed.c index 4a6ddb0..954a360 100644 --- a/gcc/expmed.c +++ b/gcc/expmed.c @@ -2116,8 +2116,8 @@ expand_shift_1 (enum tree_code code, enum machine_mode mode, rtx shifted, % GET_MODE_BITSIZE (mode)); else if (GET_CODE (op1) == SUBREG subreg_lowpart_p (op1) - INTEGRAL_MODE_P (GET_MODE (SUBREG_REG (op1))) - INTEGRAL_MODE_P (GET_MODE (op1))) + SCALAR_INT_MODE_P (GET_MODE (SUBREG_REG (op1))) + SCALAR_INT_MODE_P (GET_MODE (op1))) op1 = SUBREG_REG (op1); }
Re: expansion of vector shifts...
From: David Miller da...@redhat.com Date: Fri, 16 Nov 2012 00:33:05 -0500 (EST) From: Richard Sandiford rdsandif...@googlemail.com Date: Mon, 29 Oct 2012 10:14:53 + ...given that the code is like you say written: if (SHIFT_COUNT_TRUNCATED) { if (CONST_INT_P (op1) ... else if (GET_CODE (op1) == SUBREG subreg_lowpart_p (op1) INTEGRAL_MODE_P (GET_MODE (SUBREG_REG (op1 op1 = SUBREG_REG (op1); } INTEGRAL_MODE_P (GET_MODE (op1)) might be better than an explicit VECTOR_MODE_P check. The code really doesn't make sense for anything other than integers. (It amounts to the same thing in practice, of course...) Agreed, I've just committed the following. Thanks! Fix gcc.c-torture/compile/pr53410-2.c on sparc. * expmed.c (expand_shift_1): Don't strip non-integral SUBREGs. This is broken on sparc again, although I'm confused about how this has happened. The suggestion was to use INTEGRAL_MODE_P as the test, so what's there in expand_shift_1() is: else if (GET_CODE (op1) == SUBREG subreg_lowpart_p (op1) INTEGRAL_MODE_P (GET_MODE (SUBREG_REG (op1))) INTEGRAL_MODE_P (GET_MODE (op1))) op1 = SUBREG_REG (op1); but INTEGRAL_MODE_P accepts vectors. This is really confusing because I was absolutely sure I re-ran the test case with the fix I committed and it didn't crash any more. Maybe what we really mean to do here is check both op1 and SUBREG_REG (op1) against SCALAR_INT_MODE_P instead of INTEGRAL_MODE_P? Something like this: gcc/ 2013-02-12 David S. Miller da...@davemloft.net * expmed.c (expand_shift_1): Only strip scalar integer subregs. diff --git a/gcc/expmed.c b/gcc/expmed.c index 4a6ddb0..954a360 100644 --- a/gcc/expmed.c +++ b/gcc/expmed.c @@ -2116,8 +2116,8 @@ expand_shift_1 (enum tree_code code, enum machine_mode mode, rtx shifted, % GET_MODE_BITSIZE (mode)); else if (GET_CODE (op1) == SUBREG subreg_lowpart_p (op1) - INTEGRAL_MODE_P (GET_MODE (SUBREG_REG (op1))) - INTEGRAL_MODE_P (GET_MODE (op1))) + SCALAR_INT_MODE_P (GET_MODE (SUBREG_REG (op1))) + SCALAR_INT_MODE_P (GET_MODE (op1))) op1 = SUBREG_REG (op1); }
Re: expansion of vector shifts...
From: David Miller da...@redhat.com Date: Fri, 16 Nov 2012 00:33:05 -0500 (EST) From: Richard Sandiford rdsandif...@googlemail.com Date: Mon, 29 Oct 2012 10:14:53 + ...given that the code is like you say written: if (SHIFT_COUNT_TRUNCATED) { if (CONST_INT_P (op1) ... else if (GET_CODE (op1) == SUBREG subreg_lowpart_p (op1) INTEGRAL_MODE_P (GET_MODE (SUBREG_REG (op1 op1 = SUBREG_REG (op1); } INTEGRAL_MODE_P (GET_MODE (op1)) might be better than an explicit VECTOR_MODE_P check. The code really doesn't make sense for anything other than integers. (It amounts to the same thing in practice, of course...) Agreed, I've just committed the following. Thanks! Fix gcc.c-torture/compile/pr53410-2.c on sparc. * expmed.c (expand_shift_1): Don't strip non-integral SUBREGs. This is broken on sparc again, although I'm confused about how this has happened. The suggestion was to use INTEGRAL_MODE_P as the test, so what's there in expand_shift_1() is: else if (GET_CODE (op1) == SUBREG subreg_lowpart_p (op1) INTEGRAL_MODE_P (GET_MODE (SUBREG_REG (op1))) INTEGRAL_MODE_P (GET_MODE (op1))) op1 = SUBREG_REG (op1); but INTEGRAL_MODE_P accepts vectors. This is really confusing because I was absolutely sure I re-ran the test case with the fix I committed and it didn't crash any more. Maybe what we really mean to do here is check both op1 and SUBREG_REG (op1) against SCALAR_INT_MODE_P instead of INTEGRAL_MODE_P? Something like this: gcc/ 2013-02-12 David S. Miller da...@davemloft.net * expmed.c (expand_shift_1): Only strip scalar integer subregs. diff --git a/gcc/expmed.c b/gcc/expmed.c index 4a6ddb0..954a360 100644 --- a/gcc/expmed.c +++ b/gcc/expmed.c @@ -2116,8 +2116,8 @@ expand_shift_1 (enum tree_code code, enum machine_mode mode, rtx shifted, % GET_MODE_BITSIZE (mode)); else if (GET_CODE (op1) == SUBREG subreg_lowpart_p (op1) - INTEGRAL_MODE_P (GET_MODE (SUBREG_REG (op1))) - INTEGRAL_MODE_P (GET_MODE (op1))) + SCALAR_INT_MODE_P (GET_MODE (SUBREG_REG (op1))) + SCALAR_INT_MODE_P (GET_MODE (op1))) op1 = SUBREG_REG (op1); }
Re: expansion of vector shifts...
From: Richard Sandiford rdsandif...@googlemail.com Date: Mon, 29 Oct 2012 10:14:53 + ...given that the code is like you say written: if (SHIFT_COUNT_TRUNCATED) { if (CONST_INT_P (op1) ... else if (GET_CODE (op1) == SUBREG subreg_lowpart_p (op1) INTEGRAL_MODE_P (GET_MODE (SUBREG_REG (op1 op1 = SUBREG_REG (op1); } INTEGRAL_MODE_P (GET_MODE (op1)) might be better than an explicit VECTOR_MODE_P check. The code really doesn't make sense for anything other than integers. (It amounts to the same thing in practice, of course...) Agreed, I've just committed the following. Thanks! Fix gcc.c-torture/compile/pr53410-2.c on sparc. * expmed.c (expand_shift_1): Don't strip non-integral SUBREGs. git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@193547 138bc75d-0d04-0410-961f-82ee72b054a4 --- gcc/ChangeLog | 2 ++ gcc/expmed.c | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 9abd396..62bde4e 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,5 +1,7 @@ 2012-11-15 David S. Miller da...@davemloft.net + * expmed.c (expand_shift_1): Don't strip non-integral SUBREGs. + * configure.ac: Add check for assembler SPARC4 instruction support. * configure: Rebuild. diff --git a/gcc/expmed.c b/gcc/expmed.c index 5b697a1..8640427 100644 --- a/gcc/expmed.c +++ b/gcc/expmed.c @@ -2165,7 +2165,8 @@ expand_shift_1 (enum tree_code code, enum machine_mode mode, rtx shifted, % GET_MODE_BITSIZE (mode)); else if (GET_CODE (op1) == SUBREG subreg_lowpart_p (op1) - INTEGRAL_MODE_P (GET_MODE (SUBREG_REG (op1 + INTEGRAL_MODE_P (GET_MODE (SUBREG_REG (op1))) + INTEGRAL_MODE_P (GET_MODE (op1))) op1 = SUBREG_REG (op1); } -- 1.7.12.2.dirty
Re: expansion of vector shifts...
From: Richard Sandiford rdsandif...@googlemail.com Date: Mon, 29 Oct 2012 10:14:53 + ...given that the code is like you say written: if (SHIFT_COUNT_TRUNCATED) { if (CONST_INT_P (op1) ... else if (GET_CODE (op1) == SUBREG subreg_lowpart_p (op1) INTEGRAL_MODE_P (GET_MODE (SUBREG_REG (op1 op1 = SUBREG_REG (op1); } INTEGRAL_MODE_P (GET_MODE (op1)) might be better than an explicit VECTOR_MODE_P check. The code really doesn't make sense for anything other than integers. (It amounts to the same thing in practice, of course...) Agreed, I've just committed the following. Thanks! Fix gcc.c-torture/compile/pr53410-2.c on sparc. * expmed.c (expand_shift_1): Don't strip non-integral SUBREGs. git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@193547 138bc75d-0d04-0410-961f-82ee72b054a4 --- gcc/ChangeLog | 2 ++ gcc/expmed.c | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 9abd396..62bde4e 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,5 +1,7 @@ 2012-11-15 David S. Miller da...@davemloft.net + * expmed.c (expand_shift_1): Don't strip non-integral SUBREGs. + * configure.ac: Add check for assembler SPARC4 instruction support. * configure: Rebuild. diff --git a/gcc/expmed.c b/gcc/expmed.c index 5b697a1..8640427 100644 --- a/gcc/expmed.c +++ b/gcc/expmed.c @@ -2165,7 +2165,8 @@ expand_shift_1 (enum tree_code code, enum machine_mode mode, rtx shifted, % GET_MODE_BITSIZE (mode)); else if (GET_CODE (op1) == SUBREG subreg_lowpart_p (op1) - INTEGRAL_MODE_P (GET_MODE (SUBREG_REG (op1 + INTEGRAL_MODE_P (GET_MODE (SUBREG_REG (op1))) + INTEGRAL_MODE_P (GET_MODE (op1))) op1 = SUBREG_REG (op1); } -- 1.7.12.2.dirty
Re: expansion of vector shifts...
David Miller da...@davemloft.net writes: On sparc a simple test like (from the PR tree-optimization/53410 testcase): typedef int V __attribute__((vector_size (4 * sizeof (int; typedef unsigned int W __attribute__((vector_size (4 * sizeof (int; void f10 (W *p, W *q) { *p = *p (((const W) { 1U, 1U, 1U, 1U }) *q); } aborts in convert_move() because we're trying to move a TImode value into a V2SImode one. How does that happen? On sparc the generic tree vector layer turns the above expression into two V2SImode shifts. The *q parts of each shift are represented as: (subreg:V2SI (reg:TI xxx) 0) (subreg:V2SI (reg:TI xxx) 8) When we get down into expand_shift_1(), that SUBREG is stripped out by the SHIFT_COUNT_TRUNCATED code, and that's how we end up in the crash by the time we reach convert_move() (via expand_binop() -- expand_binop_directly() -- convert_modes() -- convert_move()). Perhaps we should elide the SUBREG stripping if the subreg has a vector mode? Agreed, although... Actually, what seems to confuse this code is that we're passing TImode values around for this vector that the target doesn't have direct support for. The SUBREG stripper explicitly checks for INTEGRAL_MODE_P, and indeed TImode is integral. ...given that the code is like you say written: if (SHIFT_COUNT_TRUNCATED) { if (CONST_INT_P (op1) ... else if (GET_CODE (op1) == SUBREG subreg_lowpart_p (op1) INTEGRAL_MODE_P (GET_MODE (SUBREG_REG (op1 op1 = SUBREG_REG (op1); } INTEGRAL_MODE_P (GET_MODE (op1)) might be better than an explicit VECTOR_MODE_P check. The code really doesn't make sense for anything other than integers. (It amounts to the same thing in practice, of course...) Richard