reinit regs upon global reg variable (IRA regression)

reinit regs upon global reg variable (IRA regression)

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 996 bytes --]

QEMU for x86 declares a global variable with register asm("%ebp").  GCC
trunk will miscompile it, using %ebp for normal register allocation
(rather than frame pointer; it compiles with -O2 -fomit-frame-pointer),
which ends up clobbering the variable.

There's a testcase at
https://bugzilla.redhat.com/show_bug.cgi?id=490509

compute_all_subb is one of the functions that displays the problem.

Turns out the new register allocation is using outdated information:
available (rather than fixed) register information in each register
class is computed before compilation starts, but it wasn't updated when
new registers became global, and IRA needs this information to be
updated.

This patch fixes it.

Bootstrapped and regression tested on i686-linux-gnu and
x86_64-linux-gnu.  Ok to install?

Jakub, would you please check this in if it's approved; I'm leaving on a
trip tomorrow morning, and I don't expect to have much
Internet-connected computer time before Monday, at the earliest.
Thanks,


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcc-ira-globalized-regs.patch --]
[-- Type: text/x-patch, Size: 524 bytes --]

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	* reginfo.c (globalize_reg): Recompute derived reg sets.

Index: gcc/reginfo.c
===================================================================
--- gcc/reginfo.c.orig	2009-03-01 02:16:18.000000000 -0300
+++ gcc/reginfo.c	2009-03-17 00:34:49.000000000 -0300
@@ -875,6 +875,8 @@ globalize_reg (int i)
   SET_HARD_REG_BIT (fixed_reg_set, i);
   SET_HARD_REG_BIT (call_used_reg_set, i);
   SET_HARD_REG_BIT (call_fixed_reg_set, i);
+
+  reinit_regs ();
 }
 \f
 

[-- Attachment #3: Type: text/plain, Size: 258 bytes --]



-- 
Alexandre Oliva           http://www.lsd.ic.unicamp.br/~oliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: reinit regs upon global reg variable (IRA regression)

[Richard Guenther]

On Thu, Mar 19, 2009 at 1:46 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> QEMU for x86 declares a global variable with register asm("%ebp").  GCC
> trunk will miscompile it, using %ebp for normal register allocation
> (rather than frame pointer; it compiles with -O2 -fomit-frame-pointer),
> which ends up clobbering the variable.
>
> There's a testcase at
> https://bugzilla.redhat.com/show_bug.cgi?id=490509
>
> compute_all_subb is one of the functions that displays the problem.
>
> Turns out the new register allocation is using outdated information:
> available (rather than fixed) register information in each register
> class is computed before compilation starts, but it wasn't updated when
> new registers became global, and IRA needs this information to be
> updated.
>
> This patch fixes it.
>
> Bootstrapped and regression tested on i686-linux-gnu and
> x86_64-linux-gnu.  Ok to install?

This looks like papering over a different issue.  I see that globalize_reg
is only called from make_decl_rtl, we should call that when we process
globals from the varpool.  So how do we end up in IRA before all
the global regs have been globalized?

So, if at all, we should call init_regs () after expanding the varpool.

Do we have a smaller testcase?

Thanks,
Richard.

> Jakub, would you please check this in if it's approved; I'm leaving on a
> trip tomorrow morning, and I don't expect to have much
> Internet-connected computer time before Monday, at the earliest.
> Thanks,
>
>
>
>
> --
> Alexandre Oliva           http://www.lsd.ic.unicamp.br/~oliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/   FSF Latin America board member
> Free Software Evangelist      Red Hat Brazil Compiler Engineer
>
>

Re: reinit regs upon global reg variable (IRA regression)

[Alexandre Oliva]

On Mar 19, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:

> On Thu, Mar 19, 2009 at 1:46 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=490509

>> Turns out the new register allocation is using outdated information:
>> available (rather than fixed) register information in each register
>> class is computed before compilation starts, but it wasn't updated when
>> new registers became global, and IRA needs this information to be
>> updated.

> This looks like papering over a different issue.

How so?  Maybe the fix is sub-optimal, but I can't get from that to
papering over a different issue.

> I see that globalize_reg is only called from make_decl_rtl

Yup, that's where we realize the decl is bound to a specific global
register and mark it as such, which must in turn recompute register
classes before they're used by anything else.

> we should call that when we process globals from the varpool.

This might very well work.  It would be fragile in other ways, though,
in that it would depend on the current unit-at-a-time compilation mode.
Unless recomputing these sets becomes a performance problem, recomputing
them as soon as we make changes to the base sets works just fine and is
far more robust.

> So how do we end up in IRA before all the global regs have been
> globalized?

We don't.  But they have been globalized after we computed the initial
init_regs(), and we never recomputed it.

> Do we have a smaller testcase?

No, I didn't waste time trying to reduce it.  The problem was obvious
enough upfront, and register allocation testcases are too fragile to be
useful.

-- 
Alexandre Oliva           http://www.lsd.ic.unicamp.br/~oliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: reinit regs upon global reg variable (IRA regression)

[Richard Guenther]

On Thu, Mar 19, 2009 at 2:14 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Mar 19, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>
>> On Thu, Mar 19, 2009 at 1:46 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=490509
>
>>> Turns out the new register allocation is using outdated information:
>>> available (rather than fixed) register information in each register
>>> class is computed before compilation starts, but it wasn't updated when
>>> new registers became global, and IRA needs this information to be
>>> updated.
>
>> This looks like papering over a different issue.
>
> How so?  Maybe the fix is sub-optimal, but I can't get from that to
> papering over a different issue.

Well, the issue would be init_regs called too early.  With
now always being in unit-at-a-time mode it should be possible to
expand global variables upfront.  But looking I see that it still
seems to be a mess, so ...

>> I see that globalize_reg is only called from make_decl_rtl
>
> Yup, that's where we realize the decl is bound to a specific global
> register and mark it as such, which must in turn recompute register
> classes before they're used by anything else.

... sure.  It also seems that ira_init is called very early.  Ugh.

>> we should call that when we process globals from the varpool.
>
> This might very well work.  It would be fragile in other ways, though,
> in that it would depend on the current unit-at-a-time compilation mode.
> Unless recomputing these sets becomes a performance problem, recomputing
> them as soon as we make changes to the base sets works just fine and is
> far more robust.
>
>> So how do we end up in IRA before all the global regs have been
>> globalized?
>
> We don't.  But they have been globalized after we computed the initial
> init_regs(), and we never recomputed it.

Given the mess the patch is ok.

Thanks,
Richard.