emit-rtl.c: Allow vector subreg of float vectors

This patch is a prerequisite for some patches I have to add multiple 
vector sizes on amdgcn.

The problem is that validate_subreg rejects things like this:

   (subreg:V32SF (reg:V64SF) 0)

These are commonly generated by the compiler. The integer equivalents 
work fine.

To be honest, I don't know why other targets have not encountered this 
problem before? Perhaps because most other targets require all vectors 
to have the same number of bits, meaning that float vectors have a fixed 
number of lanes? In my case, amdgcn can convert V64SFmode to V32SFmode 
by simply masking off some lanes.

OK to commit? (I have an x86_64 bootstrap and test in progress.)

Andrew

Andrew Stubbs <ams@codesourcery.com> writes:
> This patch is a prerequisite for some patches I have to add multiple 
> vector sizes on amdgcn.
>
> The problem is that validate_subreg rejects things like this:
>
>    (subreg:V32SF (reg:V64SF) 0)
>
> These are commonly generated by the compiler. The integer equivalents 
> work fine.
>
> To be honest, I don't know why other targets have not encountered this 
> problem before? Perhaps because most other targets require all vectors 
> to have the same number of bits, meaning that float vectors have a fixed 
> number of lanes? In my case, amdgcn can convert V64SFmode to V32SFmode 
> by simply masking off some lanes.
>
> OK to commit? (I have an x86_64 bootstrap and test in progress.)
>
> Andrew
>
> Allow vector subreg of float vectors
>
> Integer vector subregs were already permitted.
>
> gcc/ChangeLog:
>
> 	* emit-rtl.c (validate_subreg): Permit sections of float vectors.
>
> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
> index f9b0e9714d9..d7067989ad7 100644
> --- a/gcc/emit-rtl.c
> +++ b/gcc/emit-rtl.c
> @@ -947,6 +947,11 @@ validate_subreg (machine_mode omode, machine_mode imode,
>    else if (VECTOR_MODE_P (omode)
>  	   && GET_MODE_INNER (omode) == GET_MODE_INNER (imode))
>      ;
> +  /* Allow sections of vectors, both smaller and paradoxical.  (This just
> +     works for integers, but needs to be explicitly allowed for floats.)  */
> +  else if (VECTOR_MODE_P (omode) && VECTOR_MODE_P (imode)
> +	   && GET_MODE_INNER (omode) == GET_MODE_INNER (imode))
> +    ;

Might be missing something, but isn't this a subcondition of the
previous “else if”?  It looks like that ought to catch every case
that the new one does.

Thanks,
Richard

>    /* Subregs involving floating point modes are not allowed to
>       change size.  Therefore (subreg:DI (reg:DF) 0) is fine, but
>       (subreg:SI (reg:DF) 0) isn't.  */

On 06/08/2020 04:54, Richard Sandiford wrote:
>> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
>> index f9b0e9714d9..d7067989ad7 100644
>> --- a/gcc/emit-rtl.c
>> +++ b/gcc/emit-rtl.c
>> @@ -947,6 +947,11 @@ validate_subreg (machine_mode omode, machine_mode imode,
>>     else if (VECTOR_MODE_P (omode)
>>   	   && GET_MODE_INNER (omode) == GET_MODE_INNER (imode))
>>       ;
>> +  /* Allow sections of vectors, both smaller and paradoxical.  (This just
>> +     works for integers, but needs to be explicitly allowed for floats.)  */
>> +  else if (VECTOR_MODE_P (omode) && VECTOR_MODE_P (imode)
>> +	   && GET_MODE_INNER (omode) == GET_MODE_INNER (imode))
>> +    ;
> 
> Might be missing something, but isn't this a subcondition of the
> previous “else if”?  It looks like that ought to catch every case
> that the new one does.

Apparently, Hongtao and Uroš fixed my problem while I was working on this.

Yes, my patch does the same (although I would question whether it's safe 
to use "GET_MODE_INNER (imode)" without having first confirmed "imode" 
is a vector mode).

Anyway, I can drop my patch.

Andrew

Andrew Stubbs <ams@codesourcery.com> writes:
> On 06/08/2020 04:54, Richard Sandiford wrote:
>>> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
>>> index f9b0e9714d9..d7067989ad7 100644
>>> --- a/gcc/emit-rtl.c
>>> +++ b/gcc/emit-rtl.c
>>> @@ -947,6 +947,11 @@ validate_subreg (machine_mode omode, machine_mode imode,
>>>     else if (VECTOR_MODE_P (omode)
>>>   	   && GET_MODE_INNER (omode) == GET_MODE_INNER (imode))
>>>       ;
>>> +  /* Allow sections of vectors, both smaller and paradoxical.  (This just
>>> +     works for integers, but needs to be explicitly allowed for floats.)  */
>>> +  else if (VECTOR_MODE_P (omode) && VECTOR_MODE_P (imode)
>>> +	   && GET_MODE_INNER (omode) == GET_MODE_INNER (imode))
>>> +    ;
>> 
>> Might be missing something, but isn't this a subcondition of the
>> previous “else if”?  It looks like that ought to catch every case
>> that the new one does.
>
> Apparently, Hongtao and Uroš fixed my problem while I was working on this.
>
> Yes, my patch does the same (although I would question whether it's safe 
> to use "GET_MODE_INNER (imode)" without having first confirmed "imode" 
> is a vector mode).

FWIW, skipping the VECTOR_MODE_P test means that we also allow vector
paradoxical subregs of scalars, which can be useful if you're trying
to make a vector in which only element 0 is initialised.  (Only works
for element 0 though.)  IIRC this is what some of the x86 patterns do.

Guess it means we also allow vector subregs of complex values,
but that kind-of seems OK/useful too.

Thanks,
Richard