Re: [RFA:] fix another autoincdec reload

["Richard Guenther" <richard dot guenther at gmail dot com>]

On Dec 4, 2007 3:22 AM, Hans-Peter Nilsson <hans-peter.nilsson@axis.com> wrote:
> Consider <http://gcc.gnu.org/ml/gcc-patches/2000-11/msg00351.html>.
> I've stumbled upon the corresponding case for POST_INC, i.e.:
>
> (set (reg:DI 7) (mem (post_inc (reg X))))
>
> with
>  rld[0] (address of X) and
>  rld[1] (post_inc (mem (address of X)))
> and X = (plus sp (const_int 36)) as offsets are not supported
> for the target, this was reloaded as:
>
> (set R1 36)
> (set R1 (plus R1 sp))
> (set R2 (mem R1))
> (set R2 (plus R2) 8)
> (set (mem R1) R2)
> (set R2 (plus R2) -8)
> (set (reg:DI 7) ...)
>
> ...but with R1 equal to R2 because to reload, it seemed to
> reload1.c:find_reg as if there rld[1] and 2 formed a reusable
> unique reload-chain, with nothing marking there being a conflict
> between the uses.
>
> Judging by Bernd's patch above, because POST_INC needs to be
> written back the same way as POST_MODIFY, it seems that the
> reload type for rld[1] should just also be changed to
> RELOAD_OTHER, rather than the previous RELOAD_FOR_INPUT_ADDRESS
> for both (which are soon changed to RELOAD_FOR_OPERAND_ADDRESS
> at find_reloads:4291).
>
> The same would go for all autoincdec types, not just POST_*; no
> way to write the incdec'd value without conflicting R1 and R2 -
> unless of course the target can incdec memory locations
> directly, and such targets can use LEGITIMIZE_RELOAD_ADDRESS to
> optimize that case.
>
> But... that patch:
>
> Index: reload.c
> ===================================================================
> --- reload.c    (revision 130566)
> +++ reload.c    (working copy)
> @@ -5666,7 +5666,7 @@
>                      write back the value after reading it, hence we actually
>                      need two registers.  */
>                   find_reloads_address (GET_MODE (tem), &tem, XEXP (tem, 0),
> -                                       &XEXP (tem, 0), opnum, type,
> +                                       &XEXP (tem, 0), opnum, RELOAD_OTHER,
>                                         ind_levels, insn);
>                   if (!rtx_equal_p (tem, orig))
>                     push_reg_equiv_alt_mem (regno, tem);
>
>
> while regtest results for the ports below is fine, it causes minor
> code-size (and presumably performance) regressions for at least
> sh-elf.  And it doesn't seem entirely correct to use RELOAD_OTHER,
> when it's just an autoincdec case that needs to be handled, and not
> even the one for the reload above (which is just for the address).
>
> Instead, it seems like the push_reload call for the rld[1] autoincdec
> (the if-branch where it's not a proper address) is incorrect.  It
> should've passed non-NULL for out, to mark the reload properly as an
> autoincdec: (rld[r].out && ! rld[r].out_reg), see reload1.c:7650 and
> (rld[k].out && ! rld[k].out_reg) in reload1.c:8112 and related code,
> for example.  (Yeah, the rule is a bit odd but there you go.)
>
> Regtested native x86_64-linux-gnu for sanity and cross-tested
> for cris, sh and arm -elf for being autoincdec targets.
> For those targets, no change in compiled library code.
>
> :ADDPATCH reload:
> Ok to commit the patch below?

Looks like an obvious typo to me.

Thanks,
Richard.

:REVIEWMAIL:

>         * reload.c (find_reloads_address_1): To properly mark as an
>         autoincdec, pass X for non-NULL OUT in call to push_reload for
>         autoincdec which can't be trivially used as an address.
>
> Index: reload.c
> ===================================================================
> --- reload.c    (revision 130398)
> +++ reload.c    (working copy)
> @@ -5736,7 +5736,7 @@ find_reloads_address_1 (enum machine_mod
>               else
>                 {
>                   reloadnum
> -                   = push_reload (x, NULL_RTX, loc, (rtx*) 0,
> +                   = push_reload (x, x, loc, (rtx*) 0,
>                                    context_reg_class,
>                                    GET_MODE (x), GET_MODE (x), 0, 0,
>                                    opnum, type);
>
> brgds, H-P
>