Patchwork expansion of vector shifts...

login
register
mail settings
Submitter David Miller
Date Feb. 12, 2013, 10:31 p.m.
Message ID <20130212.173146.281298392845155966.davem@davemloft.net>
Download mbox | patch
Permalink /patch/220001/
State New
Headers show

Comments

David Miller - Feb. 12, 2013, 10:31 p.m.
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?

Something like this:

gcc/

2013-02-12  David S. Miller  <davem@davemloft.net>

	* expmed.c (expand_shift_1): Only strip scalar integer subregs.
Richard Guenther - Feb. 13, 2013, 11:15 a.m.
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);
>      }
>
>
>
David Miller - Feb. 13, 2013, 6:16 p.m.
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);
>>      }
>>
>>
>>
>

Patch

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);
     }