| Message ID | 20130212.173146.281298392845155966.davem@davemloft.net |
|---|---|
| State | New |
| Headers | show |
On Tue, Feb 12, 2013 at 11:31 PM, David Miller <davem@davemloft.net> wrote: > From: David Miller <davem@redhat.com> > Date: Fri, 16 Nov 2012 00:33:05 -0500 (EST) > >> From: Richard Sandiford <rdsandiford@googlemail.com> >> Date: Mon, 29 Oct 2012 10:14:53 +0000 >> >>> ...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 <davem@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); > } > > >
From: Richard Biener <richard.guenther@gmail.com> Date: Wed, 13 Feb 2013 12:15:13 +0100 > On Tue, Feb 12, 2013 at 11:31 PM, David Miller <davem@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 <davem@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); >> } >> >> >> >
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); }