Re: A reload inheritance bug
Bernd Schmidt wrote: Mark Shinwell wrote: Do you think it should be the case that, at the point below, _any_ reload with reg_rtx corresponding to a hard register should have the relevant bit set in reload_spill_index? I think so. I'm attaching a patch below. It appears to have no effect on all code I've tried - does it fix your test case? As noted before, this does fix the test case; I've now regtested it on arm-none-eabi and there are no regressions. Do you want to take care of applying it? Mark
Re: A reload inheritance bug
Mark Shinwell wrote: Bernd Schmidt wrote: Mark Shinwell wrote: Do you think it should be the case that, at the point below, _any_ reload with reg_rtx corresponding to a hard register should have the relevant bit set in reload_spill_index? I think so. I'm attaching a patch below. It appears to have no effect on all code I've tried - does it fix your test case? As noted before, this does fix the test case; I've now regtested it on arm-none-eabi and there are no regressions. Do you want to take care of applying it? Thanks for testing. I bootstrapped and regression tested it on i686-linux and committed it as 126415. I'll put it on the 4.2 and 4.1 branches in a week or so. Bernd -- This footer brought to you by insane German lawmakers. Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 40368 Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif
Re: A reload inheritance bug
Mark Mitchell wrote: Bernd Schmidt wrote: Mark Shinwell wrote: Do you think it should be the case that, at the point below, _any_ reload with reg_rtx corresponding to a hard register should have the relevant bit set in reload_spill_index? I think so. I'm attaching a patch below. It appears to have no effect on all code I've tried - does it fix your test case? Mark, did you get a chance to try Bernd's patch? This does indeed fix the test case. I'll do full test runs for cross to ARM on mainline with this patch applied. Thanks (and sorry for the delay), Mark
Re: A reload inheritance bug
Bernd Schmidt wrote: Mark Shinwell wrote: Do you think it should be the case that, at the point below, _any_ reload with reg_rtx corresponding to a hard register should have the relevant bit set in reload_spill_index? I think so. I'm attaching a patch below. It appears to have no effect on all code I've tried - does it fix your test case? Mark, did you get a chance to try Bernd's patch? -- Mark Mitchell CodeSourcery [EMAIL PROTECTED] (650) 331-3385 x713
Re: A reload inheritance bug
On Mon, Jun 11, 2007 at 09:22:24AM +0100, Mark Shinwell wrote:
+ if (rld[r].when_needed == RELOAD_FOR_INPUT
+ rld[r].reg_rtx
+ REGNO (rld[r].reg_rtx) FIRST_PSEUDO_REGISTER)
+ {
$ grep -F -e HARD_REGISTER gcc/rtl.h
#define HARD_REGISTER_P(REG) (HARD_REGISTER_NUM_P (REGNO (REG)))
#define HARD_REGISTER_NUM_P(REG_NO) ((REG_NO) FIRST_PSEUDO_REGISTER)
--
Rask Ingemann Lambertsen
Another reload inheritance bug
I've run into a case where reload clobbers a register which it had
decided to use for reload inheritance. Reload is given this:
(insn 623 430 431 35 ../../.././gcc/dp-bit.c:734 (set (reg:HI 162 [+6 ])
(mem/c/i:HI (plus:HI (reg/f:HI 10 bp)
(const_int -2 [0xfffe])) [0 S2 A16])) 9 {*movhi} (nil)
(expr_list:REG_EQUIV (mem/c/i:HI (plus:HI (reg/f:HI 10 bp)
(const_int -2 [0xfffe])) [0 S2 A16])
(nil)))
(insn 431 623 432 35 ../../.././gcc/dp-bit.c:734 (set (mem/s/j:HI (plus:HI
(reg/v/f:HI 37 [ tmp ])
(const_int 10 [0xa])) [0 variable.fraction.ll+6 S2 A16])
(reg:HI 162 [+6 ])) 9 {*movhi} (insn_list:REG_DEP_TRUE 421 (nil))
(expr_list:REG_DEAD (reg:HI 162 [+6 ])
(nil)))
(note 432 431 433 35)
(note 433 432 434 35)
(insn 434 433 435 35 ../../.././gcc/dp-bit.c:735 (parallel [
(set (mem/s/j:HI (plus:HI (reg/v/f:HI 37 [ tmp ])
(const_int 2 [0x2])) [0 variable.normal_exp+0 S2 A16])
(plus:HI (mem/s/j:HI (plus:HI (reg/v/f:HI 37 [ tmp ])
(const_int 2 [0x2])) [0 variable.normal_exp+0 S2
A16])
(const_int 1 [0x1])))
(clobber (reg:CC 13 cc))
]) 44 {*addhi3} (nil)
(expr_list:REG_UNUSED (reg:CC 13 cc)
(nil)))
Reload faces two problems here:
1) Eliminating %bp to %sp where %sp can not be used as a base register.
2) Register 37 didn't get a hard reg, so it is on the stack.
Reload handles insns 623 and 431 fine and correctly records that register
37 has been loaded into %bx (reg:HI 6 b) as a result. Unfortunately, reload
clobbers %bx in insn 897 before using %bx in insn 434.
movw%sp,%bx ;# 895 *movhi/1
movw74(%bx),%ax ;# 623 *movhi/1
movw94(%bx),%bx ;# 896 *movhi/1
movw%ax,10(%bx) ;# 431 *movhi/2
movw%sp,%bx ;# 897 *movhi/1
incw2(%bx) ;# 434 *addhi3/1
Everything looks fine early in choose_reload_regs():
(gdb) call debug_rtx (chain-insn)
(insn 434 433 435 35 ../../.././gcc/dp-bit.c:735 (parallel [
(set (mem/s/j:HI (plus:HI (reg/v/f:HI 37 [ tmp ])
(const_int 2 [0x2])) [0 variable.normal_exp+0 S2 A16])
(plus:HI (mem/s/j:HI (plus:HI (reg/v/f:HI 37 [ tmp ])
(const_int 2 [0x2])) [0 variable.normal_exp+0 S2
A16])
(const_int 1 [0x1])))
(clobber (reg:CC 13 cc))
]) 44 {*addhi3} (nil)
(expr_list:REG_UNUSED (reg:CC 13 cc)
(nil)))
The *addhi3 pattern is nothing unusual. We're using the first
alternative of adding one to a memory operand:
(define_insn *addhi3
[(set (match_operand:HI 0 nonimmediate_operand =rm,rm,qm,qm,r,m,!r,!r)
(plus:HI (match_operand:HI 1 nonimmediate_operand %0,0,0,0,0,0,*w,*B)
(match_operand:HI 2 general_operand P1,M1,Um,Uo,g,ri,*x,i)))
(clobber (reg:CC CC_REG))]
(gdb) call debug_reload()
Reload order: 0 1 2 3 4
Reload 0: reload_in (HI) = (reg/f:HI 12 sp)
BASE_REGS, RELOAD_FOR_OPADDR_ADDR (opnum = 0)
reload_in_reg: (reg/f:HI 12 sp)
Reload 1: reload_in (HI) = (mem/f/c/i:HI (plus:HI (reg/f:HI 12 sp)
(const_int 94 [0x5e]))
[0 tmp+0 S2 A16])
BASE_REGS, RELOAD_FOR_OPERAND_ADDRESS (opnum = 0), can't combine
reload_in_reg: (reg/v/f:HI 37 [ tmp ])
Reload 2: BASE_REGS, RELOAD_FOR_OPERAND_ADDRESS (opnum = 0)
reload_in_reg: (reg/f:HI 12 sp)
Reload 3: BASE_REGS, RELOAD_FOR_OPERAND_ADDRESS (opnum = 0), can't combine
reload_in_reg: (reg/v/f:HI 37 [ tmp ])
Reload 4: reload_in (HI) = (mem/s/j:HI (plus:HI (reg/v/f:HI 37 [ tmp ])
(const_int 2 [0x2])) [0
variable.normal_exp+0 S2 A16])
reload_out (HI) = (mem/s/j:HI (plus:HI (reg/v/f:HI 37 [ tmp ])
(const_int 2 [0x2])) [0
variable.normal_exp+0 S2 A16])
GENERAL_REGS, RELOAD_OTHER (opnum = 0), optional
reload_in_reg: (mem/s/j:HI (plus:HI (reg/v/f:HI 37 [ tmp ])
(const_int 2 [0x2])) [0
variable.normal_exp+0 S2 A16])
reload_out_reg: (mem/s/j:HI (plus:HI (reg/v/f:HI 37 [ tmp ])
(const_int 2 [0x2])) [0
variable.normal_exp+0 S2 A16])
reload 0:
(gdb) print regno
$61 = 12
(gdb) call debug_rtx(reg_last_reload_reg[regno])
(reg:HI 6 b)
(gdb) print /x reg_reloaded_valid
$67 = 0xc0
(gdb) print reg_reloaded_contents[6]
$64 = 37
(gdb) print reg_reloaded_contents[7]
$65 = 37
(gdb) print [EMAIL PROTECTED]
$70 = \000\000\000\000
I.e. reload looks at register 6 for reload inheritance, but the contents
aren't right, so no reload inheritance for reload 0.
reload 1:
(gdb) print regno
$77 = 37
(gdb) print mode
$78 = HImode
(gdb) call
Re: A reload inheritance bug
Mark Shinwell wrote:
As you say, one unusual thing about this situation must be the fact
that the reload register is getting chosen by the code in
push_reload heralded by If this is an input reload and the operand
contains a register that dies in this insn and is used nowhere else,
see if it is the right class to be used for this reload. Use it if so.
Not that unusual - this happens all the time.
etc. I suspect it is the conjunction of this code and the behaviour of
reload in choosing r9 for pseudo 4082 in insn 5314 above that causes the
problem.
I don't entirely follow your example below...
Still, assume a similar sequence
insn 5301: (set (reg/f:SI 4082) (reg/f:SI 3275))
insn 5291 (set (reg:DF 4078])
(unspec:DF (mem/s:DF (plus:SI (reg/f:SI 3275)
(reg:SI 3812)))
(reg:SI 3275)))
REG_DEAD 3275
some other insn where R9 is used for an input reload
insn 5314 (set (reg:DF 4096)
(mem/s:DF (plus:SI (reg/f:SI 4082) (reg:SI 4084
Here, we wouldn't use R9 as reload register in 5291
...since here, as far as I understand it, the clause mentioned above
in push_reload wouldn't select r9 to use as a reload register for
5291.
That, and the subsequent code won't use a reg that's also used in the
insn. However, ...
My gut feeling is that this example will work as a consequence.
... note that I inserted some other insn which could conceivably use
R9 as an input reload, as the hard reg is dead. Where would we
invalidate previous information about R9? I assume it would be the loop
at the end of emit_reload_insns, specifically
/* First, clear out memory of what used to be in this
spill reg.
If consecutive registers are used, clear them all. */
for (k = 0; k nr; k++)
{
CLEAR_HARD_REG_BIT (reg_reloaded_valid, i + k);
CLEAR_HARD_REG_BIT (reg_reloaded_call_part_clobbered,
i + k);
Why isn't this triggering?
(Perhaps you're getting at the fact that the decision procedure leading
us to choose r9 for pseudo 4082 in insn 5314 might be at fault instead?
No, that seems fine.
Bernd
--
This footer brought to you by insane German lawmakers.
Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen
Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 40368
Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif
Re: A reload inheritance bug
Bernd Schmidt wrote:
My gut feeling is that this example will work as a consequence.
... note that I inserted some other insn which could conceivably use
R9 as an input reload, as the hard reg is dead. Where would we
invalidate previous information about R9? I assume it would be the loop
at the end of emit_reload_insns, specifically
/* First, clear out memory of what used to be in this
spill reg.
If consecutive registers are used, clear them all. */
for (k = 0; k nr; k++)
{
CLEAR_HARD_REG_BIT (reg_reloaded_valid, i + k);
CLEAR_HARD_REG_BIT (reg_reloaded_call_part_clobbered,
i + k);
Why isn't this triggering?
This code is guarded by
/* 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)
and in this [my original] case, i is negative (see my original patch).
I don't understand the I is nonneg ... comment above: the surrounding
code seems to say that i is actually non-negative if the reload used a
_spill_ register (rather than just any register). Bernd, could you
clarify the precise meaning of spill register in this context? I've
not yet managed to completely pin this down.
Mark
Re: A reload inheritance bug
Mark Shinwell wrote: This code is guarded by /* 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) and in this [my original] case, i is negative (see my original patch). I don't understand the I is nonneg ... comment above: the surrounding code seems to say that i is actually non-negative if the reload used a _spill_ register (rather than just any register). Bernd, could you clarify the precise meaning of spill register in this context? I've not yet managed to completely pin this down. The term spill register is a bit of a historical artifact, and I think we may be getting close to the root cause here (and it seems to be my fault.) In the early days of gcc, when a register was spilled, it was spilled for the whole function and became a spill register that reload could use freely. That means that none of these spill registers would be selected during find_reloads, as they don't appear in the insn anymore. These days, we are more selective and don't spill hard regs for the entire function. It appears that spill_reg_index is only set up for registers that go through the choose_reload_regs process, not for the ones selected during find_reloads. That's probably a bad thing, as a reg_rtx chosen in find_reloads could be used as a spill reg in a previous insn (as in your example). Bernd -- This footer brought to you by insane German lawmakers. Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 40368 Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif
Re: A reload inheritance bug
Bernd Schmidt wrote: It appears that spill_reg_index is only set up for registers that go through the choose_reload_regs process, not for the ones selected during find_reloads. That's probably a bad thing, as a reg_rtx chosen in find_reloads could be used as a spill reg in a previous insn (as in your example). Yes, I think that is indeed what's going on. I wonder if this issue of spill_reg_index is also manifesting itself in: /* 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. */ /* 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 ... and the code following in emit_reload_insns? Perhaps if spill_reg_index took account of registers selected during find_reloads then this could be simplified too. So what do you think is the best approach to fix all of this? :-) Mark
Re: A reload inheritance bug
Mark Shinwell wrote: and the code following in emit_reload_insns? Perhaps if spill_reg_index took account of registers selected during find_reloads then this could be simplified too. So what do you think is the best approach to fix all of this? :-) Sounds like you gave the answer yourself in the first paragraph. At least that's what I'd try. Bernd -- This footer brought to you by insane German lawmakers. Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 40368 Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif
Re: A reload inheritance bug
Bernd Schmidt wrote: Mark Shinwell wrote: and the code following in emit_reload_insns? Perhaps if spill_reg_index took account of registers selected during find_reloads then this could be simplified too. So what do you think is the best approach to fix all of this? :-) Sounds like you gave the answer yourself in the first paragraph. At least that's what I'd try. OK, I'll have a go -- it seems like a good thing to clean up anyway, even modulo fixing this bug. Mark
Re: A reload inheritance bug
Bernd Schmidt wrote: insn 5301: (set (reg/f:SI 4082) (reg/f:SI 3275)) insn 5291 (set (reg:DF 4078]) (mem/s:DF (plus:SI (reg/f:SI 3275) (reg:SI 3812 REG_DEAD 3275 insn 5314 (set (reg:DF 4096) (mem/s:DF (plus:SI (reg/f:SI 4082) (reg:SI 4084 After reload we end up with the following. I've added dividers to show the correspondence with the insns above. insn 5301 (set (mem/f/c:SI (plus:SI (reg/f:SI 13 sp) (const_int 12))) (reg/f:SI 9 r9 [3275])) --- insn 6675 (set (reg:SI 9 r9) (plus:SI (reg/f:SI 9 r9 [3275]) (reg:SI 10 sl [3812]))) insn 5291 (set (reg:DF 75 s12 [4078]) (mem/s:DF (reg:SI 9 r9))) --- insn 6680 (set (reg:SI 1 r1) (const_int 4928)) insn 6681 (set (reg:SI 1 r1) (plus:SI (reg/f:SI 9 r9 [3275]) (reg:SI 1 r1))) insn 5314 (set (reg:DF 75 s12 [4096]) (mem/s:DF (reg:SI 1 r1))) We see here how pseudo 3275 was allocated to r9 and pseudo 4082 was spilled to the stack. At insn 5291, r9 has been allocated [*] as the reload register since pseudo 3275 dies in that instruction; at insn 5314 we see the then-incorrect use of r9 (in instruction 6681) for the value of pseudo 4082. Note also how the dump shows that the compiler thinks r9 still holds the value of pseudo 3275 at insn 6681. Presumably this is one thing that is mildly unusual - R9 being chosen in find_reloads already. This wouldn't happen later, since it's in reg_used_in_insn and therefore disallowed. Sorry for the delay in replying -- hectic week! As you say, one unusual thing about this situation must be the fact that the reload register is getting chosen by the code in push_reload heralded by If this is an input reload and the operand contains a register that dies in this insn and is used nowhere else, see if it is the right class to be used for this reload. Use it if so. etc. I suspect it is the conjunction of this code and the behaviour of reload in choosing r9 for pseudo 4082 in insn 5314 above that causes the problem. I don't entirely follow your example below... Still, assume a similar sequence insn 5301: (set (reg/f:SI 4082) (reg/f:SI 3275)) insn 5291 (set (reg:DF 4078]) (unspec:DF (mem/s:DF (plus:SI (reg/f:SI 3275) (reg:SI 3812))) (reg:SI 3275))) REG_DEAD 3275 some other insn where R9 is used for an input reload insn 5314 (set (reg:DF 4096) (mem/s:DF (plus:SI (reg/f:SI 4082) (reg:SI 4084 Here, we wouldn't use R9 as reload register in 5291 ...since here, as far as I understand it, the clause mentioned above in push_reload wouldn't select r9 to use as a reload register for 5291. My gut feeling is that this example will work as a consequence. (Perhaps you're getting at the fact that the decision procedure leading us to choose r9 for pseudo 4082 in insn 5314 might be at fault instead? Even if so, I still suspect it's the reuse of a hard reg in an insn with a REG_DEAD note for the corresponding pseudo that is the real cause because it upsets the later code -- that's what my patch was trying to correct.) Mark
Re: A reload inheritance bug
Mark Shinwell wrote: The relevant RTL instructions before reload are as follows. These correspond to points A, B and C respectively in my previous email. I must admit I'm still stumbling in the dark a bit - this would be so much easier to digest with a testcase. The question I'm trying to answer is - why does this happen in this testcase only, what makes it special? insn 5301: (set (reg/f:SI 4082) (reg/f:SI 3275)) insn 5291 (set (reg:DF 4078]) (mem/s:DF (plus:SI (reg/f:SI 3275) (reg:SI 3812 REG_DEAD 3275 insn 5314 (set (reg:DF 4096) (mem/s:DF (plus:SI (reg/f:SI 4082) (reg:SI 4084 After reload we end up with the following. I've added dividers to show the correspondence with the insns above. insn 5301 (set (mem/f/c:SI (plus:SI (reg/f:SI 13 sp) (const_int 12))) (reg/f:SI 9 r9 [3275])) --- insn 6675 (set (reg:SI 9 r9) (plus:SI (reg/f:SI 9 r9 [3275]) (reg:SI 10 sl [3812]))) insn 5291 (set (reg:DF 75 s12 [4078]) (mem/s:DF (reg:SI 9 r9))) --- insn 6680 (set (reg:SI 1 r1) (const_int 4928)) insn 6681 (set (reg:SI 1 r1) (plus:SI (reg/f:SI 9 r9 [3275]) (reg:SI 1 r1))) insn 5314 (set (reg:DF 75 s12 [4096]) (mem/s:DF (reg:SI 1 r1))) We see here how pseudo 3275 was allocated to r9 and pseudo 4082 was spilled to the stack. At insn 5291, r9 has been allocated [*] as the reload register since pseudo 3275 dies in that instruction; at insn 5314 we see the then-incorrect use of r9 (in instruction 6681) for the value of pseudo 4082. Note also how the dump shows that the compiler thinks r9 still holds the value of pseudo 3275 at insn 6681. Presumably this is one thing that is mildly unusual - R9 being chosen in find_reloads already. This wouldn't happen later, since it's in reg_used_in_insn and therefore disallowed. Still, assume a similar sequence insn 5301: (set (reg/f:SI 4082) (reg/f:SI 3275)) insn 5291 (set (reg:DF 4078]) (unspec:DF (mem/s:DF (plus:SI (reg/f:SI 3275) (reg:SI 3812))) (reg:SI 3275))) REG_DEAD 3275 some other insn where R9 is used for an input reload insn 5314 (set (reg:DF 4096) (mem/s:DF (plus:SI (reg/f:SI 4082) (reg:SI 4084 Here, we wouldn't use R9 as reload register in 5291, but could use it subsequently (as it's no longer live holding 3275) for another input reload. Then we'd run into the same problem in insn 5314. Still not sure why either case would be so unusual that we haven't seen this before. Bernd -- This footer brought to you by insane German lawmakers. Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 40368 Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif
Re: A reload inheritance bug
On Tue, May 15, 2007 at 04:27:21PM +0100, Mark Shinwell wrote: 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. I'd say try the usual testing procedure with the addition of saving away libgcc, newlib, libiberty etc. for code size comparison. If you find any, at least we'll be that much wiser. -- Rask Ingemann Lambertsen
Re: A reload inheritance bug
Rask Ingemann Lambertsen wrote:
On Mon, May 14, 2007 at 10:47:13PM +0100, Mark Shinwell wrote:
[snip]
- the last use of reg2 (in B) is inside a matched input operand;
[snip]
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.
I could be missing something here, but aren't matched operands given a
reload type of RELOAD_OTHER rather than RELOAD_FOR_INPUT?
I might be tripping up on the terminology here then. The original
instruction is:
(insn:HI 5291 5282 5295 2 (set (reg:DF 4078 [ Jd+1040 ])
(mem/s:DF (plus:SI (reg/f:SI 3275)
(reg:SI 3812)) [30 Jd+1040 S8 A64])) 578 {*movdf_vfp} (nil)
(expr_list:REG_DEAD (reg/f:SI 3275)
(expr_list:REG_DEAD (reg:SI 3812)
(nil
and after reload we end up with:
(insn 6675 5282 5291 2 (set (reg:SI 9 r9)
(plus:SI (reg/f:SI 9 r9 [3275])
(reg:SI 10 sl [3812]))) 4 {*arm_addsi3} (nil)
(nil))
(insn:HI 5291 6675 5295 2 (set (reg:DF 75 s12 [orig:4078 Jd+1040 ] [4078])
(mem/s:DF (reg:SI 9 r9) [30 Jd+1040 S8 A64])) 578
{*movdf_vfp} (nil)
(nil))
Also, which register is the inc by 8 note for? R9 or sl?
I thought the value of inc wasn't relevant here since we aren't
dealing with an autoincrement instruction (see the comment in
reload.h:struct reload).
Mark
Re: A reload inheritance bug
On Tue, May 15, 2007 at 09:31:10AM +0100, Mark Shinwell wrote:
Rask Ingemann Lambertsen wrote:
On Mon, May 14, 2007 at 10:47:13PM +0100, Mark Shinwell wrote:
[snip]
- the last use of reg2 (in B) is inside a matched input operand;
[snip]
The reload used for the instruction at B looks like this:
[snip]
GENERAL_REGS, RELOAD_FOR_INPUT (opnum = 1), inc by 8
[snip]
I could be missing something here, but aren't matched operands given a
reload type of RELOAD_OTHER rather than RELOAD_FOR_INPUT?
I might be tripping up on the terminology here then. The original
instruction is:
(insn:HI 5291 5282 5295 2 (set (reg:DF 4078 [ Jd+1040 ])
(mem/s:DF (plus:SI (reg/f:SI 3275)
(reg:SI 3812)) [30 Jd+1040 S8 A64])) 578 {*movdf_vfp} (nil)
(expr_list:REG_DEAD (reg/f:SI 3275)
(expr_list:REG_DEAD (reg:SI 3812)
(nil
Where is the matched input operand you're referring to? I don't see any
in the *movdf_vfp pattern.
Also, which register is the inc by 8 note for? R9 or sl?
I thought the value of inc wasn't relevant here since we aren't
dealing with an autoincrement instruction (see the comment in
reload.h:struct reload).
True, there's a bug in the pretty-printer. Quoting reload1.c:
/* AUTO_INC reloads need to be handled even if inherited. We got an
AUTO_INC reload if reload_out is set but reload_out_reg isn't. */
--
Rask Ingemann Lambertsen
756 unexpected failures, and counting...
Re: A reload inheritance bug
Rask Ingemann Lambertsen wrote:
On Tue, May 15, 2007 at 09:31:10AM +0100, Mark Shinwell wrote:
Rask Ingemann Lambertsen wrote:
On Mon, May 14, 2007 at 10:47:13PM +0100, Mark Shinwell wrote:
[snip]
- the last use of reg2 (in B) is inside a matched input operand;
[snip]
The reload used for the instruction at B looks like this:
[snip]
GENERAL_REGS, RELOAD_FOR_INPUT (opnum = 1), inc by 8
[snip]
I could be missing something here, but aren't matched operands given a
reload type of RELOAD_OTHER rather than RELOAD_FOR_INPUT?
I might be tripping up on the terminology here then. The original
instruction is:
(insn:HI 5291 5282 5295 2 (set (reg:DF 4078 [ Jd+1040 ])
(mem/s:DF (plus:SI (reg/f:SI 3275)
(reg:SI 3812)) [30 Jd+1040 S8 A64])) 578 {*movdf_vfp} (nil)
(expr_list:REG_DEAD (reg/f:SI 3275)
(expr_list:REG_DEAD (reg:SI 3812)
(nil
Where is the matched input operand you're referring to? I don't see any
in the *movdf_vfp pattern.
Sorry, I am mistaken in the terminology. Change the bullet point to:
- the last use of reg2 (in B) is inside an input operand.
Mark
Re: A reload inheritance bug
Mark Shinwell wrote: 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: If you can't post the testcase, the best thing you can do to help is to print out all the involved RTL insns before and after, as well as output from debug_reload for all of them (possibly once after returning to reload_as_needed from find_reloads, and again after choose_reload_regs has run). Bernd -- This footer brought to you by insane German lawmakers. Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 40368 Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif
Re: A reload inheritance bug
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
Re: A reload inheritance bug
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, 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.
OK, thanks.
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);
This one won't catch the store to the reload register r9 that causes
the clobber because it's only scanning PATTERN (insn). It's the same
issue as noted in the existing comment in emit_reload_insns:
/* 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. */
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.
This case relies on the presence of a CLOBBER though, which we don't
have.
As per Bernd's request I'll provide some more reload/RTL dumps shortly
which might clarify this.
I'm thinking that maybe we should
include the reload insns themselves in the scan.
I'm not suitably qualified to answer this one, but it sounds more
invasive than my current patch.
Mark
Re: A reload inheritance bug
Bernd Schmidt wrote:
Mark Shinwell wrote:
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:
If you can't post the testcase, the best thing you can do to help is to
print out all the involved RTL insns before and after, as well as output
from debug_reload for all of them (possibly once after returning to
reload_as_needed from find_reloads, and again after choose_reload_regs
has run).
The relevant RTL instructions before reload are as follows. These
correspond to points A, B and C respectively in my previous email.
(insn:HI 5301 3071 3079 2 (set (reg/f:SI 4082)
(reg/f:SI 3275)) 572 {*arm_movsi_vfp} (nil)
(expr_list:REG_EQUIV (symbol_ref:SI (*.LANCHOR0) [flags 0x182])
(nil)))
---
(insn:HI 5291 5282 5295 2 (set (reg:DF 4078 [ Jd+1040 ])
(mem/s:DF (plus:SI (reg/f:SI 3275)
(reg:SI 3812)) [30 Jd+1040 S8 A64])) 578 {*movdf_vfp} (nil)
(expr_list:REG_DEAD (reg/f:SI 3275)
(expr_list:REG_DEAD (reg:SI 3812)
(nil
---
(insn:HI 5314 5308 5318 2 (set (reg:DF 4096 [ Jd+1040 ])
(mem/s:DF (plus:SI (reg/f:SI 4082)
(reg:SI 4084)) [30 Jd+1040 S8 A64])) 578 {*movdf_vfp} (nil)
(expr_list:REG_EQUIV (mem/s:DF (plus:SI (reg/f:SI 4082)
(reg:SI 4084)) [30 Jd+1040 S8 A64])
(nil)))
After reload we end up with the following. I've added dividers to show
the correspondence with the insns above.
(insn:HI 5301 3071 3079 2 (set (mem/f/c:SI (plus:SI (reg/f:SI 13 sp)
(const_int 12 [0xc])) [36 S4 A32])
(reg/f:SI 9 r9 [3275])) 572 {*arm_movsi_vfp} (nil)
(expr_list:REG_EQUIV (symbol_ref:SI (*.LANCHOR0) [flags 0x182])
(nil)))
---
(insn 6675 5282 5291 2 (set (reg:SI 9 r9)
(plus:SI (reg/f:SI 9 r9 [3275])
(reg:SI 10 sl [3812]))) 4 {*arm_addsi3} (nil)
(nil))
(insn:HI 5291 6675 5295 2 (set (reg:DF 75 s12 [orig:4078 Jd+1040 ] [4078])
(mem/s:DF (reg:SI 9 r9) [30 Jd+1040 S8 A64])) 578 {*movdf_vfp} (nil)
(nil))
---
(insn 6680 5308 6681 2 (set (reg:SI 1 r1)
(const_int 4928 [0x1340])) 572 {*arm_movsi_vfp} (nil)
(nil))
(insn 6681 6680 5314 2 (set (reg:SI 1 r1)
(plus:SI (reg/f:SI 9 r9 [3275])
(reg:SI 1 r1))) 4 {*arm_addsi3} (nil)
(nil))
(insn:HI 5314 6681 5318 2 (set (reg:DF 75 s12 [orig:4096 Jd+1040 ] [4096])
(mem/s:DF (reg:SI 1 r1) [30 Jd+1040 S8 A64])) 578 {*movdf_vfp} (nil)
(expr_list:REG_EQUIV (mem/s:DF (reg:SI 1 r1) [30 Jd+1040 S8 A64])
(nil)))
We see here how pseudo 3275 was allocated to r9 and pseudo 4082 was
spilled to the stack. At insn 5291, r9 has been allocated [*] as the
reload register since pseudo 3275 dies in that instruction; at insn
5314 we see the then-incorrect use of r9 (in instruction 6681)
for the value of pseudo 4082. Note also how the dump shows that the
compiler thinks r9 still holds the value of pseudo 3275 at insn 6681.
The reloads and the values of the insn variable in reload_as_needed
are as follows. These have been dumped at the points you suggest
(the output after choose_reload_regs being the same as after
find_reloads).
For instruction 5301:
(insn:HI 5301 3071 3079 2 (set (reg/f:SI 4082)
(reg/f:SI 9 r9 [3275])) 572 {*arm_movsi_vfp} (nil)
(expr_list:REG_EQUIV (symbol_ref:SI (*.LANCHOR0) [flags 0x182])
(nil)))
Reload 0: reload_out (SI) = (reg/f:SI 4082)
NO_REGS, RELOAD_FOR_OUTPUT (opnum = 0), optional
reload_out_reg: (reg/f:SI 4082)
For instruction 5291:
(insn:HI 5291 5282 5295 2 (set (reg:DF 75 s12 [orig:4078 Jd+1040 ] [4078])
(mem/s:DF (plus:SI (reg/f:SI 9 r9 [3275])
(reg:SI 10 sl [3812])) [30 Jd+1040 S8 A64])) 578
{*movdf_vfp} (nil)
(expr_list:REG_DEAD (reg/f:SI 9 r9 [3275])
(expr_list:REG_DEAD (reg:SI 10 sl [3812])
(nil
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)
For instruction 5314:
(insn:HI 5314 5308 5318 2 (set (reg:DF 75 s12 [orig:4096 Jd+1040 ] [4096])
(mem/s:DF (plus:SI (reg/f:SI 4082)
(reg:SI 4084)) [30 Jd+1040 S8 A64])) 578 {*movdf_vfp} (nil)
(expr_list:REG_EQUIV (mem/s:DF (plus:SI (reg/f:SI 4082)
(reg:SI 4084)) [30 Jd+1040 S8 A64])
(nil)))
Reload 0: reload_in (SI) = (reg/f:SI 4082)
GENERAL_REGS, RELOAD_FOR_INPUT_ADDRESS (opnum = 1)
reload_in_reg: (reg/f:SI 4082)
Reload 1: reload_in (SI) = (const_int 4928 [0x1340])
GENERAL_REGS, RELOAD_FOR_INPUT_ADDRESS (opnum = 1), can't combine
Re: A reload inheritance bug
Mark Shinwell wrote: These dumps are of course taken before the application of my patch. Hope that helps, Thanks. I may have missed it in the previous mails, but which piece of code exactly decides to use R9 for reload 0 of insn 5314? Bernd -- This footer brought to you by insane German lawmakers. Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 40368 Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif
Re: A reload inheritance bug
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?
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.
--
Rask Ingemann Lambertsen
Re: A reload inheritance bug
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);
Re: A reload inheritance bug
Bernd Schmidt wrote: Mark Shinwell wrote: These dumps are of course taken before the application of my patch. Hope that helps, Thanks. I may have missed it in the previous mails, but which piece of code exactly decides to use R9 for reload 0 of insn 5314? I'm afraid I'm not sure exactly -- but given that r9 is still deemed to contain a valid reload, isn't that logic correct given the pseudo register numbers in question? Mark
A reload inheritance bug
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. */
Re: A reload inheritance bug
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. If the reload is inherited, and there is no non-input part of the reload that would destroy the value in the register, you should leave previous reloads alone, so that they can be inherited multiple times.
Re: A reload inheritance bug
On Mon, May 14, 2007 at 10:47:13PM +0100, Mark Shinwell wrote: [snip] - the last use of reg2 (in B) is inside a matched input operand; [snip] 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. I could be missing something here, but aren't matched operands given a reload type of RELOAD_OTHER rather than RELOAD_FOR_INPUT? Also, which register is the inc by 8 note for? R9 or sl? Perhaps show the actual insn B before and after reload? -- Rask Ingemann Lambertsen
