[PATCH] LRA: Fix incorrect register spill/reload

[PATCH] LRA: Fix incorrect register spill/reload

[Robert Suchanek]

Hello,

When investigating regression with LRA enabled for mips16 I found incorrect spilling and reload of
registers by callee.  In the case, one register was not saved, although used, and another one never
used but saved/restored. 

The issue appears to be in setting registers ever lived and subsequent passes save/restore the wrong
register(s). I have attached a patch below. I presume that the statement terminator was
typed accidentally as I do not see a justification of the df_set_regs_ever_live() function to be outside
the for loop. Or I am wrong?

Regards,
Robert 

    * lra-spills.c (assign_spill_hard_regs): Removed statement terminator after comment.
    Loop body outside the for loop.

diff --git a/gcc/lra-spills.c b/gcc/lra-spills.c
index 7c0c630..e1cf654 100644
--- a/gcc/lra-spills.c
+++ b/gcc/lra-spills.c
@@ -334,8 +334,8 @@ assign_spill_hard_regs (int *pseudo_regnos, int n)
       for (nr = 0;
           nr < hard_regno_nregs[hard_regno][lra_reg_info[regno].biggest_mode];
           nr++)
-       /* Just loop.  */;
-      df_set_regs_ever_live (hard_regno + nr, true);
+       /* Just loop.  */
+        df_set_regs_ever_live (hard_regno + nr, true);
     }
   bitmap_clear (&ok_insn_bitmap);
   free (reserved_hard_regs);

Re: [PATCH] LRA: Fix incorrect register spill/reload

[Vladimir Makarov]

On 10/31/2013 09:23 AM, Robert Suchanek wrote:
> Hello,
>
> When investigating regression with LRA enabled for mips16 I found incorrect spilling and reload of
> registers by callee.  In the case, one register was not saved, although used, and another one never
> used but saved/restored. 
>
> The issue appears to be in setting registers ever lived and subsequent passes save/restore the wrong
> register(s). I have attached a patch below. I presume that the statement terminator was
> typed accidentally as I do not see a justification of the df_set_regs_ever_live() function to be outside
> the for loop. Or I am wrong?
No,  you are not wrong.  It looks a typo for me.  It was not found
earlier as it is used right now only for x86 general reg spills into sse
regs and sse regs are not saved.

Robert, thanks for finding it and informing.  You can commit the patch
into the trunk.

>  
>
>     * lra-spills.c (assign_spill_hard_regs): Removed statement terminator after comment.
>     Loop body outside the for loop.
>
> diff --git a/gcc/lra-spills.c b/gcc/lra-spills.c
> index 7c0c630..e1cf654 100644
> --- a/gcc/lra-spills.c
> +++ b/gcc/lra-spills.c
> @@ -334,8 +334,8 @@ assign_spill_hard_regs (int *pseudo_regnos, int n)
>        for (nr = 0;
>            nr < hard_regno_nregs[hard_regno][lra_reg_info[regno].biggest_mode];
>            nr++)
> -       /* Just loop.  */;
> -      df_set_regs_ever_live (hard_regno + nr, true);
> +       /* Just loop.  */
> +        df_set_regs_ever_live (hard_regno + nr, true);
>      }
>    bitmap_clear (&ok_insn_bitmap);
>    free (reserved_hard_regs);
>
>

Re: [PATCH] LRA: Fix incorrect register spill/reload

[Jeff Law]

On 11/01/13 19:42, Vladimir Makarov wrote:

> I did not expected from the patch any problems too.  It is so obvious.
> This simple change should not affect x86 (or any other target currently
> using LRA).  The code in question is used only for x86-64 and only for
> modern intel processing tuning.  It is about accuracy of using SSE regs
> (regs_ever_live) which as I know affects only on saving/restoring regs
> in prologue/epilogue.  As all SSE_REGS are only call-clobbered, the
> accuracy of this info does not affect code generation.
Agreed.  It certainly caught me by surprise as well.


>
> I suspect, the reason for your bootstrap failure was in another patch or
> you use bootstrap specific options.
I didn't have any other patches in the trees I was testing.  After a bit 
of searching, this appears to be an ident string or something similar 
with a datestamp.    Certainly not a codegen issue related to this patch.

Concern withdrawn :-)


*** /tmp/J1     2013-11-04 14:05:13.810088971 -0700
--- /tmp/J2     2013-11-04 14:05:17.008088822 -0700
***************
*** 1545,1554 ****
              6665    6e69    6465    2500    2b71    2044    6564    6966
   0030120   n   e   d       b   u   t       n   o   t       u   s   e   d
              656e    2064    7562    2074    6f6e    2074    7375    6465
! 0030140  \0      \0  \0   4   .   9   .   0       2   0   1   3   1   0
!            2000    0000    2e34    2e39    2030    3032    3331    3031
! 0030160   3   0       (   e   x   p   e   r   i   m   e   n   t   a   l
!            3033    2820    7865    6570    6972    656d    746e    6c61
   0030200   )  \0   x   8   6   _   6   4   -   u   n   k   n   o   w   n
              0029    3878    5f36    3436    752d    6b6e    6f6e    6e77
   0030220   -   l   i   n   u   x   -   g   n   u  \0   1   .   0   .   1
--- 1545,1554 ----
              6665    6e69    6465    2500    2b71    2044    6564    6966
   0030120   n   e   d       b   u   t       n   o   t       u   s   e   d
              656e    2064    7562    2074    6f6e    2074    7375    6465
! 0030140  \0      \0  \0   4   .   9   .   0       2   0   1   3   1   1
!            2000    0000    2e34    2e39    2030    3032    3331    3131
! 0030160   0   1       (   e   x   p   e   r   i   m   e   n   t   a   l
!            3130    2820    7865    6570    6972    656d    746e    6c61
   0030200   )  \0   x   8   6   _   6   4   -   u   n   k   n   o   w   n
              0029    3878    5f36    3436    752d    6b6e    6f6e    6e77
   0030220   -   l   i   n   u   x   -   g   n   u  \0   1   .   0   .   1

Re: [PATCH] LRA: Fix incorrect register spill/reload

[Vladimir Makarov]

On 11/1/2013, 3:45 PM, Jeff Law wrote:
> On 10/31/13 14:03, Robert Suchanek wrote:
>> Hi David,
>>
>> No, I do not have read/write SVN access. I know a person who could 
>> commit the patch for me, however, if you can commit it, I'd be grateful.
> Note, I didn't see anywhere in this thread an indication this test had 
> been through a bootstrap and regression testing.  I was running those 
> overnight on Robert's behalf and the bootstrap test failed with a 
> comparison failure between stage2/toplev.o and stage3/toplev.o
>
> Vlad, when approving patches, please make sure they've been through 
> the usual bootstrap and regression testing procedures.  If the 
> contributor hasn't done it themselves, you can either do it for them 
> or ask them to do it.
>
> I trust y'all will address the problem appropriately.
>
I've tried many bootstraps on the current trunk with this patch (x86_64, 
i686, x86_64 with arch corei7, with tune corei7).  I have no problems.

I did not expected from the patch any problems too.  It is so obvious.  
This simple change should not affect x86 (or any other target currently 
using LRA).  The code in question is used only for x86-64 and only for 
modern intel processing tuning.  It is about accuracy of using SSE regs 
(regs_ever_live) which as I know affects only on saving/restoring regs 
in prologue/epilogue.  As all SSE_REGS are only call-clobbered, the 
accuracy of this info does not affect code generation.

I suspect, the reason for your bootstrap failure was in another patch or 
you use bootstrap specific options.

Sorry.

Re: [PATCH] LRA: Fix incorrect register spill/reload

[David Edelsohn]

On Fri, Nov 1, 2013 at 6:00 PM, Jeff Law <law@redhat.com> wrote:

> I'm not in a rush to revert...  I don't plan on doing anything on the trunk
> over the weekend.  I'm comfortable waiting until Monday, both to see if
> anyone else trips over whatever is going wrong and to give Robert or anyone
> else time to debug.

What is the exact configuration command that you are using in the
build that is failing?  If the bootstrap failure is not widespread,
then there must be something unique in your configuration.

Thanks, David

Re: [PATCH] LRA: Fix incorrect register spill/reload

[Mike Stump]

On Nov 1, 2013, at 12:45 PM, Jeff Law <law@redhat.com> wrote:
> Vlad, when approving patches, please make sure they've been through the usual bootstrap and regression testing procedures.

I usually place the blame on the person committing it, as they broke it.  :-)

Re: [PATCH] LRA: Fix incorrect register spill/reload

[Jeff Law]

On 11/01/13 14:39, David Edelsohn wrote:
> On Fri, Nov 1, 2013 at 3:45 PM, Jeff Law <law@redhat.com> wrote:
>> On 10/31/13 14:03, Robert Suchanek wrote:
>>>
>>> Hi David,
>>>
>>> No, I do not have read/write SVN access. I know a person who could commit
>>> the patch for me, however, if you can commit it, I'd be grateful.
>>
>> Note, I didn't see anywhere in this thread an indication this test had been
>> through a bootstrap and regression testing.  I was running those overnight
>> on Robert's behalf and the bootstrap test failed with a comparison failure
>> between stage2/toplev.o and stage3/toplev.o
>>
>> Vlad, when approving patches, please make sure they've been through the
>> usual bootstrap and regression testing procedures.  If the contributor
>> hasn't done it themselves, you can either do it for them or ask them to do
>> it.
>>
>> I trust y'all will address the problem appropriately.
>
> I bootstrapped on powerpc without problem, but Vlad earlier said that
> the code applies to an x86 case.
>
> I have not seen a deluge of complaints about bootstrap being broken.
>
> Do you want me or Vlad to revert the patch? Or you will revert the patch?
I'm not in a rush to revert...  I don't plan on doing anything on the 
trunk over the weekend.  I'm comfortable waiting until Monday, both to 
see if anyone else trips over whatever is going wrong and to give Robert 
or anyone else time to debug.

Jeff

Re: [PATCH] LRA: Fix incorrect register spill/reload

[David Edelsohn]

On Fri, Nov 1, 2013 at 3:45 PM, Jeff Law <law@redhat.com> wrote:
> On 10/31/13 14:03, Robert Suchanek wrote:
>>
>> Hi David,
>>
>> No, I do not have read/write SVN access. I know a person who could commit
>> the patch for me, however, if you can commit it, I'd be grateful.
>
> Note, I didn't see anywhere in this thread an indication this test had been
> through a bootstrap and regression testing.  I was running those overnight
> on Robert's behalf and the bootstrap test failed with a comparison failure
> between stage2/toplev.o and stage3/toplev.o
>
> Vlad, when approving patches, please make sure they've been through the
> usual bootstrap and regression testing procedures.  If the contributor
> hasn't done it themselves, you can either do it for them or ask them to do
> it.
>
> I trust y'all will address the problem appropriately.

I bootstrapped on powerpc without problem, but Vlad earlier said that
the code applies to an x86 case.

I have not seen a deluge of complaints about bootstrap being broken.

Do you want me or Vlad to revert the patch? Or you will revert the patch?

Thanks, David

Re: [PATCH] LRA: Fix incorrect register spill/reload

[Jeff Law]

On 10/31/13 14:03, Robert Suchanek wrote:
> Hi David,
>
> No, I do not have read/write SVN access. I know a person who could commit the patch for me, however, if you can commit it, I'd be grateful.
Note, I didn't see anywhere in this thread an indication this test had 
been through a bootstrap and regression testing.  I was running those 
overnight on Robert's behalf and the bootstrap test failed with a 
comparison failure between stage2/toplev.o and stage3/toplev.o

Vlad, when approving patches, please make sure they've been through the 
usual bootstrap and regression testing procedures.  If the contributor 
hasn't done it themselves, you can either do it for them or ask them to 
do it.

I trust y'all will address the problem appropriately.

Jeff

RE: [PATCH] LRA: Fix incorrect register spill/reload

[Robert Suchanek]

Hi David,

No, I do not have read/write SVN access. I know a person who could commit the patch for me, however, if you can commit it, I'd be grateful. 

Regards,
Robert

>>>>> Vladimir Makarov wrote:

> Robert, thanks for finding it and informing.  You can commit the patch
> into the trunk.

Robert,

Do you have GCC SVN access? If not, please ask one of us to commit
your patch for you.

Thanks, David

Re: [PATCH] LRA: Fix incorrect register spill/reload

[David Edelsohn]

>>>>> Vladimir Makarov wrote:

> Robert, thanks for finding it and informing.  You can commit the patch
> into the trunk.

Robert,

Do you have GCC SVN access? If not, please ask one of us to commit
your patch for you.

Thanks, David