Patchwork Followup for reg_equiv_invariant patch: Fix PR39871

login
register
mail settings
Submitter Bernd Schmidt
Date July 31, 2010, 2:46 p.m.
Message ID <4C54373F.9050400@codesourcery.com>
Download mbox | patch
Permalink /patch/60401/
State New
Headers show

Comments

Bernd Schmidt - July 31, 2010, 2:46 p.m.
On 07/24/2010 03:09 PM, IainS wrote:
> 
> On 17 Jun 2010, at 22:53, Bernd Schmidt wrote:
> 
>>
>> This is what I committed after retesting on ARM (with -O2 -fpic included
>> in TORTURE_OPTIONS).
> 
> this caused:
> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45054

Hmm, there seem to be two latent problems that suddenly show up in one
testcase...

In replace_pseudos_in, we have a series of if statements, which tries to
pick the correct new home from the various possible alternatives.  This
seems to need a check for reg_equiv_invariant.

The other is a crash because when compiling a function with 120 or so
registers, spilled_pseudos has bit 165 set from the previous function -
it's not cleared at the top of reload, and if I had to hazard a guess,
it wasn't cleared in ira_reassign_pseudos due to ALLOCNO_DONT_REASSIGN_P
(conflict_a) or somesuch while compiling the previous function.

Vlad, I think it would be helpful if you could take a look at the
spilled_pseudos mechanism.  It was changed with the introduction of IRA,
and it now has outdated code and comments of the form "if we're using
IRA, we do something else".  Also, I'm not sure I understand why this
code needed to be changed for IRA and how it is supposed to work now, so
anything you can do to explain would be good.


Bernd
Jeff Law - Aug. 2, 2010, 2:59 p.m.
On 07/31/10 08:46, Bernd Schmidt wrote:
> On 07/24/2010 03:09 PM, IainS wrote:
>> On 17 Jun 2010, at 22:53, Bernd Schmidt wrote:
>>
>>> This is what I committed after retesting on ARM (with -O2 -fpic included
>>> in TORTURE_OPTIONS).
>> this caused:
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45054
> Hmm, there seem to be two latent problems that suddenly show up in one
> testcase...
>
> In replace_pseudos_in, we have a series of if statements, which tries to
> pick the correct new home from the various possible alternatives.  This
> seems to need a check for reg_equiv_invariant.
Strange.  Why wasn't this necessary in the past?

> The other is a crash because when compiling a function with 120 or so
> registers, spilled_pseudos has bit 165 set from the previous function -
> it's not cleared at the top of reload, and if I had to hazard a guess,
> it wasn't cleared in ira_reassign_pseudos due to ALLOCNO_DONT_REASSIGN_P
> (conflict_a) or somesuch while compiling the previous function.
What I see is we've disabled wiping spilled_pseudos in the main reload 
loop because its state needs to be carried across iterations, but 
nothing was added to wipe spilled_pseudos at either the very beginning 
or very end of reload, thus the pollution of the bitmap.  I don't see 
how ALLOCNO_DONT_REASSIGN_P plays into this.

One could certainly discuss why we need to accumulate the set of spills 
in spilled_pseudos from one iterations of the main reload loop to the 
next.  It seems wrong, but there's probably some subtle reason for this 
change in behavior that I'm missing.  The comment certainly isn't clear 
enough on this issue.

Jeff
Jeff Law - Aug. 2, 2010, 3 p.m.
On 07/31/10 08:46, Bernd Schmidt wrote:
> On 07/24/2010 03:09 PM, IainS wrote:
>> On 17 Jun 2010, at 22:53, Bernd Schmidt wrote:
>>
>>> This is what I committed after retesting on ARM (with -O2 -fpic included
>>> in TORTURE_OPTIONS).
>> this caused:
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45054
> Hmm, there seem to be two latent problems that suddenly show up in one
> testcase...
>
> In replace_pseudos_in, we have a series of if statements, which tries to
> pick the correct new home from the various possible alternatives.  This
> seems to need a check for reg_equiv_invariant.
>
> The other is a crash because when compiling a function with 120 or so
> registers, spilled_pseudos has bit 165 set from the previous function -
> it's not cleared at the top of reload, and if I had to hazard a guess,
> it wasn't cleared in ira_reassign_pseudos due to ALLOCNO_DONT_REASSIGN_P
> (conflict_a) or somesuch while compiling the previous function.
>
> Vlad, I think it would be helpful if you could take a look at the
> spilled_pseudos mechanism.  It was changed with the introduction of IRA,
> and it now has outdated code and comments of the form "if we're using
> IRA, we do something else".  Also, I'm not sure I understand why this
> code needed to be changed for IRA and how it is supposed to work now, so
> anything you can do to explain would be good.
BTW, the change to wipe spilled_pseudos at the beginning of reload is 
clearly OK.  The replace_pseudos_in probably is OK as well, I'm just 
curious why we haven't seen the need for testing invariants in that code 
prior to now.

jeff
Bernd Schmidt - Aug. 2, 2010, 3:03 p.m.
On 08/02/2010 05:00 PM, Jeff Law wrote:
> BTW, the change to wipe spilled_pseudos at the beginning of reload is
> clearly OK.

Actually, I think some IRA code wants to wipe it in the process of
trying to reallocate them.  So I think a gcc_assert that it's empty
afterwards is probably better once we fix whatever IRA bug we've found.

> The replace_pseudos_in probably is OK as well, I'm just
> curious why we haven't seen the need for testing invariants in that code
> prior to now.

Probably because now we're using costs, and pseudos that are
reg_equiv_invariant end up not allocated more often.  Plus, it was
replacing inside CALL_INSN_FUNCTION_USAGE, which probably doesn't
contain such pseudos very often.


Bernd
Bernd Schmidt - Aug. 2, 2010, 3:11 p.m.
On 08/02/2010 05:03 PM, Bernd Schmidt wrote:
> On 08/02/2010 05:00 PM, Jeff Law wrote:
>> BTW, the change to wipe spilled_pseudos at the beginning of reload is
>> clearly OK.
> 
> Actually, I think some IRA code wants to wipe it in the process of
> trying to reallocate them.  So I think a gcc_assert that it's empty
> afterwards is probably better once we fix whatever IRA bug we've found.

Never mind me.  I looked at ira_reassign_pseudos again, and while it
does clear bits in the bitmap, I no longer think it's supposed to end
with a wiped bitmap.  So I think I'll put in the CLEAR_REG_SET and let
Vlad take a look at whether there's anything else that needs to be done.


Bernd
Jeff Law - Aug. 2, 2010, 3:38 p.m.
On 08/02/10 09:03, Bernd Schmidt wrote:
> On 08/02/2010 05:00 PM, Jeff Law wrote:
>> BTW, the change to wipe spilled_pseudos at the beginning of reload is
>> clearly OK.
> Actually, I think some IRA code wants to wipe it in the process of
> trying to reallocate them.  So I think a gcc_assert that it's empty
> afterwards is probably better once we fix whatever IRA bug we've found.
If IRA is able to find an alternate hard reg for the pseudo, then it'll 
clear that pseudo's entry from spilled_pseudos.  However, a pseudo which 
is spilled for which no alternate hard reg is found will have its bit 
left on in spilled_pseudos.


>> The replace_pseudos_in probably is OK as well, I'm just
>> curious why we haven't seen the need for testing invariants in that code
>> prior to now.
> Probably because now we're using costs, and pseudos that are
> reg_equiv_invariant end up not allocated more often.  Plus, it was
> replacing inside CALL_INSN_FUNCTION_USAGE, which probably doesn't
> contain such pseudos very often.
Ahhh.  OK.

jeff

Patch

Index: reload1.c
===================================================================
--- reload1.c	(revision 162675)
+++ reload1.c	(working copy)
@@ -599,6 +599,8 @@  replace_pseudos_in (rtx *loc, enum machi
 
       if (reg_equiv_constant[regno])
 	*loc = reg_equiv_constant[regno];
+      else if (reg_equiv_invariant[regno])
+	*loc = reg_equiv_invariant[regno];
       else if (reg_equiv_mem[regno])
 	*loc = reg_equiv_mem[regno];
       else if (reg_equiv_address[regno])
@@ -814,6 +816,7 @@  reload (rtx first, int global)
   last_spill_reg = -1;
 
   /* Spill any hard regs that we know we can't eliminate.  */
+  CLEAR_REG_SET (&spilled_pseudos);
   CLEAR_HARD_REG_SET (used_spill_regs);
   /* There can be multiple ways to eliminate a register;
      they should be listed adjacently.