Patchwork LRA: Fix incorrect register spill/reload

login
register
mail settings
Submitter Robert Suchanek
Date Oct. 31, 2013, 1:23 p.m.
Message ID <B5E67142681B53468FAF6B7C313565623D34BDCC@KLMAIL01.kl.imgtec.org>
Download mbox | patch
Permalink /patch/287488/
State New
Headers show

Comments

Robert Suchanek - Oct. 31, 2013, 1:23 p.m.
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.
Vladimir Makarov - Oct. 31, 2013, 2:40 p.m.
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);
>
>

Patch

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);