A reload inheritance bug

[Mark Shinwell <shinwell at codesourcery dot com>]

I have had the misfortune to discover a reload inheritance bug which,
after a long period of analysis, has turned out to be very similar to
the situation described by Richard Sandiford in
http://gcc.gnu.org/ml/gcc-patches/2007-01/msg01977.html.

This being my first serious foray into reload, I would appreciate some
advice as to whether it looks like my current patch is going in the
right direction.  (Ian, I've copied this to you since you approved
Richard's patch linked above; any help would be appreciated.)

The bug is currently only exhibiting itself on a proprietary testcase
when compiled for an ARM target and is extremely sensitive to the
particular code involved.  It arises as follows, using the same notation
as in Richard's mail:

    A: reg1 <- reg2
       ...
    B: last use of reg2
       ...
    C: use of reg1

where:

    - reg1 is not allocated a hard register, and is spilled to the stack;
    - reg2 is allocated a hard register H;
    - reg2 has a death note in B;
    - the last use of reg2 (in B) is inside a matched input operand;
    - reg1 and reg2 are not modified between A and C;
    - there are no labels between A and C.

The reload used for the instruction at B looks like this:

Reload 0: reload_in (SI) = (plus:SI (reg/f:SI 9 r9 [3275])
                                                    (reg:SI 10 sl [3812]))
        GENERAL_REGS, RELOAD_FOR_INPUT (opnum = 1), inc by 8
        reload_in_reg: (plus:SI (reg/f:SI 9 r9 [3275])
                                                    (reg:SI 10 sl [3812]))
        reload_reg_rtx: (reg:SI 9 r9)

where, in the notation from above, r9 is H and pseudo 3275 is reg2.
Since 3275 dies in the corresponding instruction, reload has chosen
r9 as the reload register.  Unfortunately when we later arrive at
instruction C during inheritance, the current code believes that r9
is still holding the value that it had at A -- which it is not, because
the use of r9 as the reload register at B has clobbered it.

In order to fix this I think that reload1.c:emit_reload_insns should,
at the point of discovery of an input reload whose reload register is a
non-spill hard register H, invalidate all previous reloads involving
that hard register.  That is what the patch below does, or aims to
do.  (The patch is actually against a 4.2-based branch but the code
looks to be the same on the mainline.)  It at least fixes this
particular test case, and appears to be in line with Richard's patch.

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?  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...

Thanks in advance for any help.  Once this is fixed, I shall add the
necessary comments, test it appropriately, and submit it for approval
on the mainline.

Mark

--


Index: reload1.c
===================================================================
--- reload1.c   (revision 170932)
+++ reload1.c   (working copy)
@@ -7506,6 +7506,9 @@ emit_reload_insns (struct insn_chain *ch
            }
        }

+      if (i < 0 && rld[r].in != NULL_RTX && rld[r].reg_rtx != NULL_RTX)
+       forget_old_reloads_1 (rld[r].reg_rtx, NULL_RTX, NULL);
+
       /* The following if-statement was #if 0'd in 1.34 (or before...).
         It's reenabled in 1.35 because supposedly nothing else
         deals with this problem.  */