This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH]: PR 11271 Handle reloading of R+R+CONST for any reg


> This test can not only have false positives, but also false negatives.
> In fact, I do not understand the reason for that test at all -- the
> documented meaning of LEGITIMATE_CONSTANT_P says nothing about this.

Yes, I think LEGITIMATE_CONSTANT_P is abused here.

> If I understand that thread correctly, the issue there was whether or
> not the target of the reload was the same as the REG inside the PLUS
> or not.  This issue could be fixed -apart from in
> find_reloads_address_part- by changing either push_reload or gen_reload.

Yes, but the root problem is IMHO really that find_reloads_address_part
should push 2 reloads instead of only 1 in this case.  I think that any other 
fix (including my patch) is only papering over the problem.

> The situation on s390 is quite different.  In the first place, all that
> code that tries to reload PLUSes in gen_reload is fundamentally broken
> for s390 because it generates RTL for a regular (arithmetic) addition
> pattern, while we need a load-address pattern -- this is really different
> on s390 because LOAD ADDRESS only performs a *31-bit* addition.

Thanks for clarifying this.  The situation is indeed more intricate than on 
the SPARC.

> What gen_reload tries to generate will not match any insn pattern on
> s390, because we do not have any instruction to perform a 32-bit
> addition *without clobbering the condition code*. On the other hand,
> If gen_reload does finally use gen_add2_insn it will emit an insn
> that does clobber the (possibly live at this point!) condition code.
> [ Actually, just about every use of gen_add2_insn in reload1.c is
> fundamentally broken for that very same reason. ]

I recently came accross the very same problem with gen_add2_insn in 
postreload.c on i386.  And your remark is a bit frightening, because it 
appears that every single use of gen_add2_insn might be unsafe...

> IMO the in the PLUS case in find_reloads_address_part the test for
> LEGITIMATE_CONSTANT_P / PREFERRED_RELOAD_CLASS is completely bogus.
> Instead, I think that a PLUS to be reloaded should be treated as
> an address (which it always is, actually!), and be fixed up via
> find_reloads_address if necessary, possibly like so:
>
> static void
> find_reloads_address_part (rtx x, rtx *loc, enum reg_class class,
>                            enum machine_mode mode, int opnum,
>                            enum reload_type type, int ind_levels)
> {
>   if (CONSTANT_P (x)
>       && (! LEGITIMATE_CONSTANT_P (x)
>
>           || PREFERRED_RELOAD_CLASS (x, class) == NO_REGS))
>
>     {
>       rtx tem;
>
>       tem = x = force_const_mem (mode, x);
>       find_reloads_address (mode, &tem, XEXP (tem, 0), &XEXP (tem, 0),
>                             opnum, type, ind_levels, 0);
>     }
>
>   else if (GET_CODE (x) == PLUS)
>     {
>       find_reloads_address (VOIDmode, (rtx *)0, x, loc,
>                             opnum, type, ind_levels, 0);
>     }
>
>   push_reload (x, NULL_RTX, loc, (rtx*) 0, class,
>                mode, VOIDmode, 0, 0, opnum, type);
> }

I think this would cure the SH4 bug too.

-- 
Eric Botcazou


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]