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
expansion of vector shifts...
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?
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.
