[PATCH] ARM: exclude fixed_regs for stack-alignment save/restore

[PATCH] ARM: exclude fixed_regs for stack-alignment save/restore

[Roland McGrath]

When the ARM compiler needs to ensure the stack pointer stays aligned
and it's already doing a multi-register push/pop in the prologue and
epilogue, it chooses some arbitrary register to add to the register set
in that push and pop just to increase the size of the stack used by 4
bytes.  This is presumed to be harmless, since some register that is
either call-clobbered or not touched by the function at all is just
getting pushed and then the same value popped into it.  

But if e.g. I use -ffixed-r9 then I think it's a reasonable expectation
that no code is generated that touches r9 in any way, shape, or form.
(My actual concern is a variant target port still in progress, where
the ABI specifies that r9 is reserved, and the system enforces that no
instruction may modify r9.)

I haven't managed to come up with an isolated test case to demonstrate
the bug.  Apparently I just don't understand the stack and register
pressure requirements that make the compiler get into the situation
where it wants to add a random register for alignment padding purposes.

I don't have a setup where I can do a proper regression test for ARM.
(My system has a /usr/arm-linux-gnueabi/include/ but configuring with
--target=arm-linux-gnueabi --with-headers=/usr/arm-linux-gnueabi/include
did not succeed in building libgcc.)

But the change seems pretty obviously correct IMHO.


Thanks,
Roland


gcc/
2012-06-14  Roland McGrath  <mcgrathr@google.com>

	* config/arm/arm.c (arm_get_frame_offsets): Never use a fixed register
	as the extra register to save/restore for stack-alignment padding.

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 092e202..bcfef3e 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -16752,7 +16752,12 @@ arm_get_frame_offsets (void)
 	  else
 	    for (i = 4; i <= (TARGET_THUMB1 ? LAST_LO_REGNUM : 11); i++)
 	      {
-		if ((offsets->saved_regs_mask & (1 << i)) == 0)
+		/* While the gratuitous register save/restore is ordinarily
+		   harmless, if a register is marked as fixed it may be
+		   entirely forbidden by the system ABI to touch it, so we
+		   should avoid those registers.  */
+		if (!fixed_regs[i]
+		    && (offsets->saved_regs_mask & (1 << i)) == 0)
 		  {
 		    reg = i;
 		    break;

Re: [PATCH] ARM: exclude fixed_regs for stack-alignment save/restore

[Mike Stump]

On Jun 14, 2012, at 10:16 AM, Roland McGrath wrote:
> But if e.g. I use -ffixed-r9 then I think it's a reasonable expectation
> that no code is generated that touches r9 in any way, shape, or form.

Also,  I can't help but wonder if global_regs is respected.  In theory, people are allowed to declare global registers, and nothing should be stopping them, though, this is abi breaking, and one does need to recompile the world as I recall to use it, so, most people don't use it and can't use it, but the bare metal people can.

Your change looks good to me.

I'll note in passing that cse.c does:

/* Determine whether register number N is considered a fixed register for the                             
   purpose of approximating register costs.                                                               
   It is desirable to replace other regs with fixed regs, to reduce need for                              
   non-fixed hard regs.                                                                                   
   A reg wins if it is either the frame pointer or designated as fixed.  */
#define FIXED_REGNO_P(N)  \
  ((N) == FRAME_POINTER_REGNUM || (N) == HARD_FRAME_POINTER_REGNUM \
   || fixed_regs[N] || global_regs[N])

Re: [PATCH] ARM: exclude fixed_regs for stack-alignment save/restore

[Roland McGrath]

On Thu, Jun 14, 2012 at 1:13 PM, Mike Stump <mikestump@comcast.net> wrote:
> On Jun 14, 2012, at 10:16 AM, Roland McGrath wrote:
>> But if e.g. I use -ffixed-r9 then I think it's a reasonable expectation
>> that no code is generated that touches r9 in any way, shape, or form.
>
> Also, I can't help but wonder if global_regs is respected.

It's clearly not.  Making the added condition !fixed_regs[i] && !global_regs[i]
seems sensible to me.


Thanks,
Roland

Re: [PATCH] ARM: exclude fixed_regs for stack-alignment save/restore

[Roland McGrath]

Here's the version of the change that incorporates Mike's suggestion.


Thanks,
Roland


gcc/
2012-06-14  Roland McGrath  <mcgrathr@google.com>

	* config/arm/arm.c (arm_get_frame_offsets): Never use a fixed register
	as the extra register to save/restore for stack-alignment padding.

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 092e202..13771d9 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -16752,7 +16752,12 @@ arm_get_frame_offsets (void)
 	  else
 	    for (i = 4; i <= (TARGET_THUMB1 ? LAST_LO_REGNUM : 11); i++)
 	      {
-		if ((offsets->saved_regs_mask & (1 << i)) == 0)
+		/* While the gratuitous register save/restore is ordinarily
+		   harmless, if a register is marked as fixed or global it
+		   may be entirely forbidden by the system ABI to touch it,
+		   so we should avoid those registers.  */
+		if (!fixed_regs[i] && !global_regs[i]
+		    && (offsets->saved_regs_mask & (1 << i)) == 0)
 		  {
 		    reg = i;
 		    break;

Re: [PATCH] ARM: exclude fixed_regs for stack-alignment save/restore

[Richard Sandiford]

Roland McGrath <mcgrathr@google.com> writes:
> On Thu, Jun 14, 2012 at 1:13 PM, Mike Stump <mikestump@comcast.net> wrote:
>> On Jun 14, 2012, at 10:16 AM, Roland McGrath wrote:
>>> But if e.g. I use -ffixed-r9 then I think it's a reasonable expectation
>>> that no code is generated that touches r9 in any way, shape, or form.
>>
>> Also, I can't help but wonder if global_regs is respected.
>
> It's clearly not.  Making the added condition !fixed_regs[i] &&
> !global_regs[i] seems sensible to me.

All global registers have to be fixed though.  The original seemed
fine to me FWIW.

Richard

Re: [PATCH] ARM: exclude fixed_regs for stack-alignment save/restore

[Richard Earnshaw]

On 16/06/12 13:42, Richard Sandiford wrote:
> Roland McGrath <mcgrathr@google.com> writes:
>> On Thu, Jun 14, 2012 at 1:13 PM, Mike Stump <mikestump@comcast.net> wrote:
>>> On Jun 14, 2012, at 10:16 AM, Roland McGrath wrote:
>>>> But if e.g. I use -ffixed-r9 then I think it's a reasonable expectation
>>>> that no code is generated that touches r9 in any way, shape, or form.
>>>
>>> Also, I can't help but wonder if global_regs is respected.
>>
>> It's clearly not.  Making the added condition !fixed_regs[i] &&
>> !global_regs[i] seems sensible to me.
> 
> All global registers have to be fixed though.  The original seemed
> fine to me FWIW.
> 
> Richard
> 
Indeed, see globalize_reg() in reginfo.c.

R.

Re: [PATCH] ARM: exclude fixed_regs for stack-alignment save/restore

[Roland McGrath]

OK then.  If you like the original patch, would you like to commit it for me?

Thanks,
Roland

Re: [PATCH] ARM: exclude fixed_regs for stack-alignment save/restore

[Roland McGrath]

On Mon, Jun 18, 2012 at 9:34 AM, Roland McGrath <mcgrathr@google.com> wrote:
> OK then.  If you like the original patch, would you like to commit it for me?

ping?

Re: [PATCH] ARM: exclude fixed_regs for stack-alignment save/restore

[Roland McGrath]

Richard, here is the patch against the current trunk, as I promised
last week in Prague.  Please apply.


Thanks,
Roland


gcc/
2012-07-17  Roland McGrath  <mcgrathr@google.com>

	* config/arm/arm.c (arm_get_frame_offsets): Never use a fixed register
	as the extra register to save/restore for stack-alignment padding.

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index e2f625c..189f71e 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -16121,7 +16121,12 @@ arm_get_frame_offsets (void)
 	  else
 	    for (i = 4; i <= (TARGET_THUMB1 ? LAST_LO_REGNUM : 11); i++)
 	      {
-		if ((offsets->saved_regs_mask & (1 << i)) == 0)
+		/* While the gratuitous register save/restore is ordinarily
+		   harmless, if a register is marked as fixed or global it
+		   may be entirely forbidden by the system ABI to touch it,
+		   so we should avoid those registers.  */
+		if (!fixed_regs[i]
+		    && (offsets->saved_regs_mask & (1 << i)) == 0)
 		  {
 		    reg = i;
 		    break;

Re: [PATCH] ARM: exclude fixed_regs for stack-alignment save/restore

[Roland McGrath]

ping?

Re: [PATCH] ARM: exclude fixed_regs for stack-alignment save/restore

[Richard Earnshaw]

On 17/07/12 21:42, Roland McGrath wrote:
> Richard, here is the patch against the current trunk, as I promised
> last week in Prague.  Please apply.
> 

Done.

I've tweaked the comments slightly, but the functional modification is
unchanged.

R.

> 
> Thanks,
> Roland
> 
> 
> gcc/
> 2012-07-17  Roland McGrath  <mcgrathr@google.com>
> 
> 	* config/arm/arm.c (arm_get_frame_offsets): Never use a fixed register
> 	as the extra register to save/restore for stack-alignment padding.
> 
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index e2f625c..189f71e 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -16121,7 +16121,12 @@ arm_get_frame_offsets (void)
>  	  else
>  	    for (i = 4; i <= (TARGET_THUMB1 ? LAST_LO_REGNUM : 11); i++)
>  	      {
> -		if ((offsets->saved_regs_mask & (1 << i)) == 0)
> +		/* While the gratuitous register save/restore is ordinarily
> +		   harmless, if a register is marked as fixed or global it
> +		   may be entirely forbidden by the system ABI to touch it,
> +		   so we should avoid those registers.  */
> +		if (!fixed_regs[i]
> +		    && (offsets->saved_regs_mask & (1 << i)) == 0)
>  		  {
>  		    reg = i;
>  		    break;
> 

Re: [PATCH] ARM: exclude fixed_regs for stack-alignment save/restore

[Roland McGrath]

Thanks muchly!