| Message ID | 4C54373F.9050400@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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.