Re: post-inc & duplicate reloads

[Hans-Peter Nilsson <hans-peter dot nilsson at axis dot com>]

> Date: Sat, 25 Aug 2001 12:54:12 -0700
> From: Geoff Keating <geoffk@geoffk.org>

(Oh no.  Not only is it hard to get patches in, now people are
starting to revert the few patches that I get in.  1/2 :-)

> Reloads for insn # 16
> Reload 0: reload_in (QI) = (mem:QI (post_inc:HI (reg/v/f:HI 2 r2 [25])) 0)
>         EIGHT_REGS, RELOAD_FOR_OTHER_ADDRESS (opnum = 0)
>         reload_in_reg: (mem:QI (post_inc:HI (reg/v/f:HI 2 r2 [25])) 0)
>         reload_reg_rtx: (reg:QI 5 r5)
> Reload 1: reload_in (QI) = (mem:QI (post_inc:HI (reg/v/f:HI 2 r2 [25])) 0)
>         reload_out (HI) = (reg:HI 9 r9 [30])
>         EIGHT_REGS, RELOAD_OTHER (opnum = 0)
>         reload_in_reg: (mem:QI (post_inc:HI (reg/v/f:HI 2 r2 [25])) 0)
>         reload_out_reg: (reg:HI 9 r9 [30])
>         reload_reg_rtx: (reg:HI 5 r5)
> Reload 2: reload_out (BI) = (scratch:BI)
>         CARRY_REGS, RELOAD_FOR_OUTPUT (opnum = 3)
>         reload_out_reg: (scratch:BI)
>         reload_reg_rtx: (reg:BI 16 carry)
> 
> which is wrong.  Reload 0 and reload 1 both increment r2, with the
> result that r2 gets incremented by 4 instead of by 2.  Reload 0 is the
> incorrect one.

For me without much insight into reload, it looks like the wrong
reload is "Reload 1", being a reload to HI when the memory
access (and thus the post-increment) is QI.

> There is already code in push_reloads which detects in-out operands
> with side-effects and ensures that the side-effects happen only once,
> under the comment
> 
>   /* If we have a read-write operand with an address side-effect,
>      change either IN or OUT so the side-effect happens only once.  */

Not sure if you're referring to the original bug or the one you
see, but the original bug wasn't that side-effects happened
twice, but instead that the post-increment was "stretched out"
from HImode to SImode.  So if reload widens the mode of the
memory access for the side-effect, you see the bug.

> I believe that the patch is wrong.  I'd like to try to work out what
> the right patch is, but the CRIS port never got contributed, so I'm
> kind of stuck.

Well, more correctly, it *was* contributed (first part in
<URL:http://gcc.gnu.org/ml/gcc-patches/2000-11/msg01181.html>).
There were no comments and no approval for the port, hence not
checked in.  Come to think of it, there were some issues with a
prerequisite patch about compiling libgcc1 functions, that
weren't resolved.  Anyway, that patch is now outdated, not only
due to recent target rearrangements in gcc.

> Reverting this patch fixed 21 testsuite failures on stormy16-elf, and
> introduced no new ones (plus the bug causes atoi() to be miscompiled).
> This is slightly unfortunate, because if it had introduced new
> failures, that'd probably be the same bug that Hans saw.
> 
> Anyway, this seems to be the best I can do.  If anyone can reproduce
> the problem that this patch originally fixed on a contributed port,
> please contact me!  I will commit this in a day or two, to give Hans a
> chance to respond.

I don't mind that patch being reverted (in current form, after
subreg-byte changes), but wish to have your attention when it
comes to fixing the bug again.

I'm updating the CRIS port to current CVS, but somehow labels
are deleted so I can't right now get to the point of running
gcc.c-torture/execute/20000726-1.c, which is the name of the
committed test-case.  You might want to try and tweak that
testcase for stormy16 to see if you can tickle the original bug.
At a glance it seems s/short/char/ is necessary to make it trig,
at least in theory.

brgds, H-P