Fix LRA subreg calculation for big-endian targets

Message ID 87inbopx9c.fsf@linaro.org
State New
Headers show
Series
  • Fix LRA subreg calculation for big-endian targets
Related show

Commit Message

Richard Sandiford Jan. 26, 2018, 1:25 p.m.
LRA was using a subreg offset of 0 whenever constraints matched
two operands with different modes.  That leads to an invalid offset
(and ICE) on big-endian targets if one of the modes is narrower
than a word.  E.g. if a (reg:SI X) is matched to a (reg:QI Y),
the big-endian subreg should be (subreg:QI (reg:SI X) 3) rather
than (subreg:QI (reg:SI X) 0).

But this raises the issue of what the behaviour should be when the
matched operands occupy different numbers of registers.  Should the
register numbers match, or should the locations of the lsbs match?
Although the documentation isn't clear, reload went for the second
interpretation (which seems the most natural to me):

      /* On a REG_WORDS_BIG_ENDIAN machine, point to the last register of a
         multiple hard register group of scalar integer registers, so that
         for example (reg:DI 0) and (reg:SI 1) will be considered the same
         register.  */

So I think this means that we can/must use the lowpart offset
unconditionally, rather than trying to separate out the multi-register
case.  This also matches the LRA handling of constant integers, which
already uses lowpart subregs.

The patch fixes gcc.target/aarch64/sve/extract_[34].c for aarch64_be.

Tested on aarch64_be-none-elf, aarch64-linux-gnu and x86_64-linux-gnu.
OK to install?


2018-01-26  Richard Sandiford  <richard.sandiford@linaro.org>

gcc/
	* lra-constraints.c (match_reload): Use subreg_lowpart_offset
	rather than 0 when creating partial subregs.

Comments

Jeff Law Jan. 30, 2018, 6:07 a.m. | #1
On 01/26/2018 06:25 AM, Richard Sandiford wrote:
> LRA was using a subreg offset of 0 whenever constraints matched

> two operands with different modes.  That leads to an invalid offset

> (and ICE) on big-endian targets if one of the modes is narrower

> than a word.  E.g. if a (reg:SI X) is matched to a (reg:QI Y),

> the big-endian subreg should be (subreg:QI (reg:SI X) 3) rather

> than (subreg:QI (reg:SI X) 0).

Yup.  That can't be right on big endian.

> 

> But this raises the issue of what the behaviour should be when the

> matched operands occupy different numbers of registers.  Should the

> register numbers match, or should the locations of the lsbs match?

> Although the documentation isn't clear, reload went for the second

> interpretation (which seems the most natural to me):

I can even recall seeing that interpretation in local-alloc.c and/or
global.c from eons ago in the context of register tying.  Both
essentially punted doing anything smart in that case anyway leading to
occasionally dreadful code for simple extensions.


> 

>       /* On a REG_WORDS_BIG_ENDIAN machine, point to the last register of a

>          multiple hard register group of scalar integer registers, so that

>          for example (reg:DI 0) and (reg:SI 1) will be considered the same

>          register.  */

> 

> So I think this means that we can/must use the lowpart offset

> unconditionally, rather than trying to separate out the multi-register

> case.  This also matches the LRA handling of constant integers, which

> already uses lowpart subregs.

> 

> The patch fixes gcc.target/aarch64/sve/extract_[34].c for aarch64_be.

> 

> Tested on aarch64_be-none-elf, aarch64-linux-gnu and x86_64-linux-gnu.

> OK to install?

> 

> 

> 2018-01-26  Richard Sandiford  <richard.sandiford@linaro.org>

> 

> gcc/

> 	* lra-constraints.c (match_reload): Use subreg_lowpart_offset

> 	rather than 0 when creating partial subregs.

OK.  Makes me wonder how many big endian LRA targets are getting
significant use.

jeff
Eric Botcazou Jan. 30, 2018, 11:31 p.m. | #2
> OK.  Makes me wonder how many big endian LRA targets are getting significant

> use.


Debian still has an active SPARC64 port now based on GCC 7 with LRA.

-- 
Eric Botcazou
Segher Boessenkool Jan. 31, 2018, 4:28 p.m. | #3
Hi!

On Fri, Jan 26, 2018 at 01:25:51PM +0000, Richard Sandiford wrote:
>  	  if (SCALAR_INT_MODE_P (inmode))

>  	    new_out_reg = gen_lowpart_SUBREG (outmode, reg);

>  	  else

> -	    new_out_reg = gen_rtx_SUBREG (outmode, reg, 0);

> +	    {

> +	      poly_uint64 offset = subreg_lowpart_offset (outmode, inmode);

> +	      new_out_reg = gen_rtx_SUBREG (outmode, reg, offset);

> +	    }


Is this now not exactly the same as the SCALAR_INT_MODE_P case?  The mode
of "reg" is inmode, after all?


Segher
Segher Boessenkool Jan. 31, 2018, 4:31 p.m. | #4
On Mon, Jan 29, 2018 at 11:07:12PM -0700, Jeff Law wrote:
> OK.  Makes me wonder how many big endian LRA targets are getting

> significant use.


powerpc and powerpc64 are BE, but we don't often have QImode and HImode
(all instructions work on 32-bit or 64-bit words).


Segher
Richard Sandiford Jan. 31, 2018, 7:06 p.m. | #5
Segher Boessenkool <segher@kernel.crashing.org> writes:
> Hi!

>

> On Fri, Jan 26, 2018 at 01:25:51PM +0000, Richard Sandiford wrote:

>>  	  if (SCALAR_INT_MODE_P (inmode))

>>  	    new_out_reg = gen_lowpart_SUBREG (outmode, reg);

>>  	  else

>> -	    new_out_reg = gen_rtx_SUBREG (outmode, reg, 0);

>> +	    {

>> +	      poly_uint64 offset = subreg_lowpart_offset (outmode, inmode);

>> +	      new_out_reg = gen_rtx_SUBREG (outmode, reg, offset);

>> +	    }

>

> Is this now not exactly the same as the SCALAR_INT_MODE_P case?  The mode

> of "reg" is inmode, after all?


Bah, yes.  Don't know how I missed that. :-(  I think I must have
been reading it as SCALAR_INT_P, and thinking this was some weird
VOIDmode thing.

Will fix.

Thanks,
Richard
Richard Sandiford Feb. 2, 2018, 2:17 p.m. | #6
Richard Sandiford <richard.sandiford@linaro.org> writes:
> Segher Boessenkool <segher@kernel.crashing.org> writes:

>> Hi!

>>

>> On Fri, Jan 26, 2018 at 01:25:51PM +0000, Richard Sandiford wrote:

>>>  	  if (SCALAR_INT_MODE_P (inmode))

>>>  	    new_out_reg = gen_lowpart_SUBREG (outmode, reg);

>>>  	  else

>>> -	    new_out_reg = gen_rtx_SUBREG (outmode, reg, 0);

>>> +	    {

>>> +	      poly_uint64 offset = subreg_lowpart_offset (outmode, inmode);

>>> +	      new_out_reg = gen_rtx_SUBREG (outmode, reg, offset);

>>> +	    }

>>

>> Is this now not exactly the same as the SCALAR_INT_MODE_P case?  The mode

>> of "reg" is inmode, after all?

>

> Bah, yes.  Don't know how I missed that. :-(  I think I must have

> been reading it as SCALAR_INT_P, and thinking this was some weird

> VOIDmode thing.

>

> Will fix.


Like so.  Tested as before.  OK to install?

Thanks,
Richard


2018-02-02  Richard Sandiford  <richard.sandiford@linaro.org>

gcc/
	* lra-constraints.c (match_reload): Unconditionally use
	gen_lowpart_SUBREG, rather than selecting between that
	and equivalent gen_rtx_SUBREG code.

Index: gcc/lra-constraints.c
===================================================================
--- gcc/lra-constraints.c	2018-01-31 14:14:16.701405568 +0000
+++ gcc/lra-constraints.c	2018-02-02 14:14:50.701951577 +0000
@@ -942,13 +942,7 @@ match_reload (signed char out, signed ch
 	  reg = new_in_reg
 	    = lra_create_new_reg_with_unique_value (inmode, in_rtx,
 						    goal_class, "");
-	  if (SCALAR_INT_MODE_P (inmode))
-	    new_out_reg = gen_lowpart_SUBREG (outmode, reg);
-	  else
-	    {
-	      poly_uint64 offset = subreg_lowpart_offset (outmode, inmode);
-	      new_out_reg = gen_rtx_SUBREG (outmode, reg, offset);
-	    }
+	  new_out_reg = gen_lowpart_SUBREG (outmode, reg);
 	  LRA_SUBREG_P (new_out_reg) = 1;
 	  /* If the input reg is dying here, we can use the same hard
 	     register for REG and IN_RTX.  We do it only for original
@@ -965,13 +959,7 @@ match_reload (signed char out, signed ch
 	  reg = new_out_reg
 	    = lra_create_new_reg_with_unique_value (outmode, out_rtx,
 						    goal_class, "");
-	  if (SCALAR_INT_MODE_P (outmode))
-	    new_in_reg = gen_lowpart_SUBREG (inmode, reg);
-	  else
-	    {
-	      poly_uint64 offset = subreg_lowpart_offset (inmode, outmode);
-	      new_in_reg = gen_rtx_SUBREG (inmode, reg, offset);
-	    }
+	  new_in_reg = gen_lowpart_SUBREG (inmode, reg);
 	  /* NEW_IN_REG is non-paradoxical subreg.  We don't want
 	     NEW_OUT_REG living above.  We add clobber clause for
 	     this.  This is just a temporary clobber.  We can remove
Segher Boessenkool Feb. 2, 2018, 7:23 p.m. | #7
On Fri, Feb 02, 2018 at 02:17:59PM +0000, Richard Sandiford wrote:
> Richard Sandiford <richard.sandiford@linaro.org> writes:

> > Segher Boessenkool <segher@kernel.crashing.org> writes:

> >> On Fri, Jan 26, 2018 at 01:25:51PM +0000, Richard Sandiford wrote:

> >>>  	  if (SCALAR_INT_MODE_P (inmode))

> >>>  	    new_out_reg = gen_lowpart_SUBREG (outmode, reg);

> >>>  	  else

> >>> -	    new_out_reg = gen_rtx_SUBREG (outmode, reg, 0);

> >>> +	    {

> >>> +	      poly_uint64 offset = subreg_lowpart_offset (outmode, inmode);

> >>> +	      new_out_reg = gen_rtx_SUBREG (outmode, reg, offset);

> >>> +	    }

> >>

> >> Is this now not exactly the same as the SCALAR_INT_MODE_P case?  The mode

> >> of "reg" is inmode, after all?

> >

> > Bah, yes.  Don't know how I missed that. :-(  I think I must have

> > been reading it as SCALAR_INT_P, and thinking this was some weird

> > VOIDmode thing.

> >

> > Will fix.

> 

> Like so.  Tested as before.  OK to install?


Looks good to me, fwiw.


Segher
Vladimir Makarov Feb. 6, 2018, 8:37 p.m. | #8
On 02/02/2018 09:17 AM, Richard Sandiford wrote:
> Richard Sandiford <richard.sandiford@linaro.org> writes:

>> Segher Boessenkool <segher@kernel.crashing.org> writes:

>>> Hi!

>>>

>>> On Fri, Jan 26, 2018 at 01:25:51PM +0000, Richard Sandiford wrote:

>>>>   	  if (SCALAR_INT_MODE_P (inmode))

>>>>   	    new_out_reg = gen_lowpart_SUBREG (outmode, reg);

>>>>   	  else

>>>> -	    new_out_reg = gen_rtx_SUBREG (outmode, reg, 0);

>>>> +	    {

>>>> +	      poly_uint64 offset = subreg_lowpart_offset (outmode, inmode);

>>>> +	      new_out_reg = gen_rtx_SUBREG (outmode, reg, offset);

>>>> +	    }

>>> Is this now not exactly the same as the SCALAR_INT_MODE_P case?  The mode

>>> of "reg" is inmode, after all?

>> Bah, yes.  Don't know how I missed that. :-(  I think I must have

>> been reading it as SCALAR_INT_P, and thinking this was some weird

>> VOIDmode thing.

>>

>> Will fix.

> Like so.  Tested as before.  OK to install?

>


Yes.  Thank you, Richard.
> 2018-02-02  Richard Sandiford  <richard.sandiford@linaro.org>

>

> gcc/

> 	* lra-constraints.c (match_reload): Unconditionally use

> 	gen_lowpart_SUBREG, rather than selecting between that

> 	and equivalent gen_rtx_SUBREG code.

>

> Index: gcc/lra-constraints.c

> ===================================================================

> --- gcc/lra-constraints.c	2018-01-31 14:14:16.701405568 +0000

> +++ gcc/lra-constraints.c	2018-02-02 14:14:50.701951577 +0000

> @@ -942,13 +942,7 @@ match_reload (signed char out, signed ch

>   	  reg = new_in_reg

>   	    = lra_create_new_reg_with_unique_value (inmode, in_rtx,

>   						    goal_class, "");

> -	  if (SCALAR_INT_MODE_P (inmode))

> -	    new_out_reg = gen_lowpart_SUBREG (outmode, reg);

> -	  else

> -	    {

> -	      poly_uint64 offset = subreg_lowpart_offset (outmode, inmode);

> -	      new_out_reg = gen_rtx_SUBREG (outmode, reg, offset);

> -	    }

> +	  new_out_reg = gen_lowpart_SUBREG (outmode, reg);

>   	  LRA_SUBREG_P (new_out_reg) = 1;

>   	  /* If the input reg is dying here, we can use the same hard

>   	     register for REG and IN_RTX.  We do it only for original

> @@ -965,13 +959,7 @@ match_reload (signed char out, signed ch

>   	  reg = new_out_reg

>   	    = lra_create_new_reg_with_unique_value (outmode, out_rtx,

>   						    goal_class, "");

> -	  if (SCALAR_INT_MODE_P (outmode))

> -	    new_in_reg = gen_lowpart_SUBREG (inmode, reg);

> -	  else

> -	    {

> -	      poly_uint64 offset = subreg_lowpart_offset (inmode, outmode);

> -	      new_in_reg = gen_rtx_SUBREG (inmode, reg, offset);

> -	    }

> +	  new_in_reg = gen_lowpart_SUBREG (inmode, reg);

>   	  /* NEW_IN_REG is non-paradoxical subreg.  We don't want

>   	     NEW_OUT_REG living above.  We add clobber clause for

>   	     this.  This is just a temporary clobber.  We can remove

Patch

Index: gcc/lra-constraints.c
===================================================================
--- gcc/lra-constraints.c	2018-01-20 13:43:02.060083731 +0000
+++ gcc/lra-constraints.c	2018-01-26 13:22:46.350577506 +0000
@@ -945,7 +945,10 @@  match_reload (signed char out, signed ch
 	  if (SCALAR_INT_MODE_P (inmode))
 	    new_out_reg = gen_lowpart_SUBREG (outmode, reg);
 	  else
-	    new_out_reg = gen_rtx_SUBREG (outmode, reg, 0);
+	    {
+	      poly_uint64 offset = subreg_lowpart_offset (outmode, inmode);
+	      new_out_reg = gen_rtx_SUBREG (outmode, reg, offset);
+	    }
 	  LRA_SUBREG_P (new_out_reg) = 1;
 	  /* If the input reg is dying here, we can use the same hard
 	     register for REG and IN_RTX.  We do it only for original
@@ -965,7 +968,10 @@  match_reload (signed char out, signed ch
 	  if (SCALAR_INT_MODE_P (outmode))
 	    new_in_reg = gen_lowpart_SUBREG (inmode, reg);
 	  else
-	    new_in_reg = gen_rtx_SUBREG (inmode, reg, 0);
+	    {
+	      poly_uint64 offset = subreg_lowpart_offset (inmode, outmode);
+	      new_in_reg = gen_rtx_SUBREG (inmode, reg, offset);
+	    }
 	  /* NEW_IN_REG is non-paradoxical subreg.  We don't want
 	     NEW_OUT_REG living above.  We add clobber clause for
 	     this.  This is just a temporary clobber.  We can remove