diff mbox series

[LRA] Do not use eliminable registers for spilling

Message ID 2ef934ad-ee64-51f6-138b-bb052327304a@codesourcery.com
State New
Headers show
Series [LRA] Do not use eliminable registers for spilling | expand

Commit Message

Kwok Cheung Yeung Nov. 7, 2019, 5:28 p.m. UTC
Hello

On AMD GCN, I encountered the following situation in the following 
testcases using the compilation flags '-O2 -ftracer -fsplit-paths':

libgomp.oacc-fortran/reduction-1.f90
libgomp.oacc-fortran/reduction-2.f90
libgomp.oacc-fortran/reduction-3.f90
gcc.c-torture/execute/ieee/pr50310.c

- LRA decides to spill a register to s14 (which is used for the hard 
frame pointer, but is not in use due to the -fomit-frame-pointer implied 
by -O2). The reload dump has:

   Spill r612 into hr14
...
(insn 597 711 712 2 (set (reg:BI 129 scc [612])
         (ne:BI (reg:SI 2 s2 [684])
             (const_int 0 [0]))) "reduction-1.f90":22:0 23 {cstoresi4}
      (nil))
...
(insn 710 713 598 2 (set (reg:BI 14 s14)
         (reg:BI 160 v0 [685])) "reduction-1.f90":22:0 3 {*movbi}
      (nil))

- Later on, LRA decides to allocate s14 to a pseudo:

	 Assigning to 758 (cl=ALL_REGS, orig=758, freq=388, tfirst=758, 
tfreq=388)...
	   Assign 14 to subreg reload r758 (freq=388)
...
(insn 801 786 787 34 (set (reg:BI 14 s14 [758])
         (reg:BI 163 v3 [758])) 3 {*movbi}
      (nil))

- But then the next BB reloads the value previously spilled into s14, 
which has been clobbered by previous instruction:

(insn 733 144 732 9 (set (reg:BI 163 v3 [706])
         (reg:BI 14 s14)) 3 {*movbi}
      (nil))

A similar issue has been dealt with in the past in PR83327, which was 
fixed in r258093. However, it does not work here - s14 is not marked as 
conflicting with pseudo 758.

This is because s14 is in eliminable_regset - if 
HARD_FRAME_POINTER_IS_FRAME_POINTER is false, 
ira_setup_eliminable_regset puts HARD_FRAME_POINTER_REGNUM into 
eliminable_regset even if the frame pointer is not needed (Why is this? 
It seems to have been that way since IRA was introduced). At the 
beginning of process_bb_lives (in lra-lives.c), eliminable_regset is 
~ANDed out of hard_regs_live, so even if s14 is in the live-outs of the 
BB, it will be removed from consideration when registering conflicts 
with pseudos in the BB.

(As an aside, the liveness of eliminable spill registers would 
previously have been updated by make_hard_regno_live and 
make_hard_regno_dead, but as of r276440 '[LRA] Don't make eliminable 
registers live (PR91957)' this is no longer the case.)

Given that conflicts with registers in eliminable_regset is not tracked, 
I believe the easiest fix is simply to prevent eliminable registers from 
being used as spill registers.

Built and tested on AMD GCN with no regressions.

I've bootstrapped it on x86_64, but there is no point testing on it ATM 
as TARGET_SPILL_CLASS was disabled in r237792.

Okay for trunk?

Kwok Yeung


     [LRA] Do not use eliminable registers for spilling

     2019-11-07  Kwok Cheung Yeung  <kcy@codesourcery.com>

     	gcc/
     	* lra-spills.c (assign_spill_hard_regs): Do not spill into
     	registers in eliminable_regset.

Comments

Vladimir Makarov Nov. 8, 2019, 4:20 p.m. UTC | #1
On 2019-11-07 12:28 p.m., Kwok Cheung Yeung wrote:
> Hello
>
> On AMD GCN, I encountered the following situation in the following 
> testcases using the compilation flags '-O2 -ftracer -fsplit-paths':
>
> libgomp.oacc-fortran/reduction-1.f90
> libgomp.oacc-fortran/reduction-2.f90
> libgomp.oacc-fortran/reduction-3.f90
> gcc.c-torture/execute/ieee/pr50310.c
>
> - LRA decides to spill a register to s14 (which is used for the hard 
> frame pointer, but is not in use due to the -fomit-frame-pointer 
> implied by -O2). The reload dump has:
>
>   Spill r612 into hr14
> ...
> (insn 597 711 712 2 (set (reg:BI 129 scc [612])
>         (ne:BI (reg:SI 2 s2 [684])
>             (const_int 0 [0]))) "reduction-1.f90":22:0 23 {cstoresi4}
>      (nil))
> ...
> (insn 710 713 598 2 (set (reg:BI 14 s14)
>         (reg:BI 160 v0 [685])) "reduction-1.f90":22:0 3 {*movbi}
>      (nil))
>
> - Later on, LRA decides to allocate s14 to a pseudo:
>
>      Assigning to 758 (cl=ALL_REGS, orig=758, freq=388, tfirst=758, 
> tfreq=388)...
>        Assign 14 to subreg reload r758 (freq=388)
> ...
> (insn 801 786 787 34 (set (reg:BI 14 s14 [758])
>         (reg:BI 163 v3 [758])) 3 {*movbi}
>      (nil))
>
> - But then the next BB reloads the value previously spilled into s14, 
> which has been clobbered by previous instruction:
>
> (insn 733 144 732 9 (set (reg:BI 163 v3 [706])
>         (reg:BI 14 s14)) 3 {*movbi}
>      (nil))
>
> A similar issue has been dealt with in the past in PR83327, which was 
> fixed in r258093. However, it does not work here - s14 is not marked 
> as conflicting with pseudo 758.
>
> This is because s14 is in eliminable_regset - if 
> HARD_FRAME_POINTER_IS_FRAME_POINTER is false, 
> ira_setup_eliminable_regset puts HARD_FRAME_POINTER_REGNUM into 
> eliminable_regset even if the frame pointer is not needed (Why is 
> this? It seems to have been that way since IRA was introduced).

   I don't remember exactly why.  It was long time ago (12 years).  I 
suspect it was related to the fact that IRA worked with reload first and 
LRA was added much later and reload communicated differently to IRA than 
to the old global RA.  I guess we could try to change it and see what 
happens.


> At the beginning of process_bb_lives (in lra-lives.c), 
> eliminable_regset is ~ANDed out of hard_regs_live, so even if s14 is 
> in the live-outs of the BB, it will be removed from consideration when 
> registering conflicts with pseudos in the BB.
>
> (As an aside, the liveness of eliminable spill registers would 
> previously have been updated by make_hard_regno_live and 
> make_hard_regno_dead, but as of r276440 '[LRA] Don't make eliminable 
> registers live (PR91957)' this is no longer the case.)
>
> Given that conflicts with registers in eliminable_regset is not 
> tracked, I believe the easiest fix is simply to prevent eliminable 
> registers from being used as spill registers.
>
> Built and tested on AMD GCN with no regressions.
>
> I've bootstrapped it on x86_64, but there is no point testing on it 
> ATM as TARGET_SPILL_CLASS was disabled in r237792.
>
> Okay for trunk?
>
>
Yes, the patch is ok.

Thank you for the patch and good explanation of the problem.

>
>
>     [LRA] Do not use eliminable registers for spilling
>
>     2019-11-07  Kwok Cheung Yeung  <kcy@codesourcery.com>
>
>         gcc/
>         * lra-spills.c (assign_spill_hard_regs): Do not spill into
>         registers in eliminable_regset.
>
> diff --git a/gcc/lra-spills.c b/gcc/lra-spills.c
> index 0068e52..54f76cc 100644
> --- a/gcc/lra-spills.c
> +++ b/gcc/lra-spills.c
> @@ -283,6 +283,8 @@ assign_spill_hard_regs (int *pseudo_regnos, int n)
>        for (k = 0; k < spill_class_size; k++)
>      {
>        hard_regno = ira_class_hard_regs[spill_class][k];
> +      if (TEST_HARD_REG_BIT (eliminable_regset, hard_regno))
> +        continue;
>        if (! overlaps_hard_reg_set_p (conflict_hard_regs, mode, 
> hard_regno))
>          break;
>      }
diff mbox series

Patch

diff --git a/gcc/lra-spills.c b/gcc/lra-spills.c
index 0068e52..54f76cc 100644
--- a/gcc/lra-spills.c
+++ b/gcc/lra-spills.c
@@ -283,6 +283,8 @@  assign_spill_hard_regs (int *pseudo_regnos, int n)
        for (k = 0; k < spill_class_size; k++)
  	{
  	  hard_regno = ira_class_hard_regs[spill_class][k];
+	  if (TEST_HARD_REG_BIT (eliminable_regset, hard_regno))
+	    continue;
  	  if (! overlaps_hard_reg_set_p (conflict_hard_regs, mode, hard_regno))
  	    break;
  	}