Rask Ingemann Lambertsen wrote:
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,
[snip]
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. */
It seems to me that the only special thing happening in your testcase is
that reload.in(_reg) is a PLUS rather than a REG or MEM. Does you patch not
prevent reload inheritance in many cases where it would be OK?
See below.
Hmm, the immediately preceeding if() block begins:
/* I is nonneg if this reload used a register.
If rld[r].reg_rtx is 0, this is an optional reload
that we opted to ignore. */
if (i >= 0 && rld[r].reg_rtx != 0)
{
I'd like a similiar comment just before your code. It is the i < 0 in
your case which prevents the preceeding if() block from updating
reg_reloaded_valid and so on.
I'm not as clear as I ought to be about the i < 0 condition; what I
was trying to establish here was "is this reload register a hard
register that was pre-existing in the subexpression being reloaded"?
I believe this corresponds to "is this register not a spill register"
(hence the use of i < 0, following the existing code below) -- but there
doesn't seem anywhere to be a concrete definition of exactly what
counts as a spill register in this context, so I may be mistaken.
Part of the reason for starting this thread was that I was concerned
about invalidating reloads that could be re-used later. However, it
seems to me that in every circumstance where the reload register is a
hard register and the value assigned to that reload register is
different from the value held by the register during the evaluation of
the subexpression being reloaded (as we have here with r9 <- r9 + r10),
we must invalidate all previous reloads involving that hard register.
I may well have encoded that logic incorrectly in the patch, though.
Mark
--
/* If a register gets output-reloaded from a non-spill register,
that invalidates any previous reloaded copy of it.
But forget_old_reloads_1 won't get to see it, because
it thinks only about the original insn. So invalidate it here.
Also do the same thing for RELOAD_OTHER constraints where the
output is discarded. */
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))))
{
rtx out = ((rld[r].out && REG_P (rld[r].out))
? rld[r].out : rld[r].out_reg);
int nregno = REGNO (out);
/* REG_RTX is now set or clobbered by the main instruction.
As the comment above explains, forget_old_reloads_1 only
sees the original instruction, and there is no guarantee
that the original instruction also clobbered REG_RTX.
For example, if find_reloads sees that the input side of
a matched operand pair dies in this instruction, it may
use the input register as the reload register.
Calling forget_old_reloads_1 is a waste of effort if
REG_RTX is also the output register.
If we know that REG_RTX holds the value of a pseudo
register, the code after the call will record that fact. */
if (rld[r].reg_rtx && rld[r].reg_rtx != out)
forget_old_reloads_1 (rld[r].reg_rtx, NULL_RTX, NULL);
