Re: post-inc & duplicate reloads

[Geoff Keating <geoffk at geoffk dot org>]

> Date: Mon, 27 Aug 2001 19:03:09 +0200
> From: Hans-Peter Nilsson <hans-peter.nilsson@axis.com>
> CC: gcc-patches@gcc.gnu.org, hans-peter.nilsson@axis.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.

The memory _address_ is HI, and it is the address that is being reloaded.

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

Yes.  Your patch added an extra reload to try to fix this, with the
effect that suddenly the post-increment was done twice.  Part of the
problem is that such operands should always be reloaded, so I don't
really know how your patch could do the right thing---it would seem
that this should happen every time this code is used.

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

If you want to try again, I'll look for it in gcc-patches.

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

I couldn't make it fail.  

-- 
- Geoffrey Keating <geoffk@geoffk.org>