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

Message ID 8f29097a-3690-5a60-ffb5-09382ed2f92e@codesourcery.com
State New
Headers show
Series
  • emit-rtl.c: Allow vector subreg of float vectors
Related show

Commit Message

Andrew Stubbs Aug. 5, 2020, 1:18 p.m.
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

Comments

Richard Sandiford Aug. 6, 2020, 3:54 a.m. | #1
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.  */
Andrew Stubbs Aug. 7, 2020, 12:52 p.m. | #2
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
Richard Sandiford Aug. 10, 2020, 4:22 p.m. | #3
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

Patch

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))
+    ;
   /* 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.  */