Re: A reload inheritance bug

[Rask Ingemann Lambertsen]

On Mon, May 14, 2007 at 10:47:13PM +0100, Mark Shinwell wrote:

> I'm fairly certain that this is the correct approach to fix this
> issue, but I'm less certain that I have correctly guarded the call
> to forget_old_reloads_1, and indeed that I've done everything to
> eradicate the previous reloads involving H.  For example, should I be
> wiping the necessary range in reg_last_reload_reg?

   No, any use of reg_last_reload_reg should be guarded by
TEST_HARD_REG_BIT(reg_reloaded_valid, reg). It is just an optimization to
avoid a linear scan of reg_reloaded_contents.

> On the subject of
> the guard, I am unsure because the existing code involves a much
> more complicated check:
> 
>       if (i < 0
>           && ((rld[r].out != 0
>                && (REG_P (rld[r].out)
>                    || (MEM_P (rld[r].out)
>                        && REG_P (rld[r].out_reg))))
>               || (rld[r].out == 0 && rld[r].out_reg
>                   && REG_P (rld[r].out_reg))))
> 
> Should I perhaps be mirroring this check for the input case too?
> I'm quite keen to make the fix for this bug cover all eventualities
> in terms of the various varieties of reload that might arise, if
> possible...

   Reload is 15000+ lines and many if() statements are even more complex
than the one you quote. It is fine to want to cover all eventualities, but
this is the fifth time someone runs into a reload inheritance bug since
about August 2006, so be realistic. I think changes to (except for rewrites
of) reload are best kept small.

   There are two places in reload_as_needed() where note_stores() calls
forget_old_reloads(). The first one at the top before emitting reload insns:

      else if (INSN_P (insn))
        {
          regset_head regs_to_forget;
          INIT_REG_SET (&regs_to_forget);
          note_stores (PATTERN (insn), forget_old_reloads_1, &regs_to_forget);

   The second one after emitting reload insns, just before the big
AUTO_INC_DEC block at the end:

          if (num_eliminable && chain->need_elim)
            update_eliminable_offsets ();

           /* Any previously reloaded spilled pseudo reg, stored in this
           * insn,
             is no longer validly lying around to save a future reload.
             Note that this does not detect pseudos that were reloaded
             for this insn in order to be stored in
             (obeying register constraints).  That is correct; such reload
             registers ARE still valid.  */
          forget_marked_reloads (&regs_to_forget);
          CLEAR_REG_SET (&regs_to_forget);

          /* There may have been CLOBBER insns placed after INSN.  So scan
             between INSN and NEXT and use them to forget old reloads.  */
          for (x = NEXT_INSN (insn); x != old_next; x = NEXT_INSN (x))
            if (NONJUMP_INSN_P (x) && GET_CODE (PATTERN (x)) == CLOBBER)
              note_stores (PATTERN (x), forget_old_reloads_1, NULL);

#ifdef AUTO_INC_DEC
          /* Likewise for regs altered by auto-increment in this insn.

   It might be worth investigating why these calls to
forget_old_reloads_1(), in particular the second one, don't handle your
case, and which changes would fix it. I'm thinking that maybe we should
include the reload insns themselves in the scan.

-- 
Rask Ingemann Lambertsen