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

[Ulrich Weigand <weigand at i1 dot informatik dot uni-erlangen dot de>]

Eric Botcazou wrote:

> > The new code now splits the address as
> >    reload reg 1   <-  (plus (reg 0) (const_int))
> >    (plus (reload reg 1) (reg 15))
> >
> > which is broken since the (plus (reg 0) (const_int)) reload cannot
> > be handled with just a single scratch register.
> 
> I presume this latter problem is orthogonal to the problem of not being a 
> base reg, but it is also true that all base regs support "direct" reloading 
> for (plus (reg) (const_int))?

The only way we can reload a PLUS is via a LOAD ADDRESS instruction.
The operand of that instruction must fulfil the same constraints
as a memory operand address; the formats supported are
  CONST
  REG + CONST
  REG + REG + CONST
where any REG must be a base register and any CONST must be in
range (0 .. 4095).

[ The destination of the LOAD ADDRESS can be any general register
(even reg 0, which is not a valid base register), no matter whether
it is the same as one of the source registers or not. ]

> > The problem here appears to be that find_reloads_address_part
> > assumes that its argument is basically directly reloadable into
> > a register; for a PLUS this means it must be a valid load-address
> > instruction argument, i.e. a valid address.
> 
> Yup, find_reloads_address_part assumes that, if the constant is 
> LEGITIMATE_CONSTANT_P, then the PLUS pattern is directly reloadable. This 
> can be false, and I hit this problem recently on SH4 (PR opt/10392).

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.

> See the thread starting at http://gcc.gnu.org/ml/gcc/2003-09/msg00460.html
> for the reasons why I eventually didn't try to patch the function.

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.

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

This is why we catch any PLUS to be reloaded via a secondary reload,
and generate a load-address pattern from reload_insi.  As the PLUS
related code in gen_reload is thus never executed, changes to it 
cannot fix the problem.

The change to push_reload also does not help the problem on s390,
because whether or not the destination of the reload is equal to
one of the source registers is completely irrelevant --
(plus (reg 0) (const_int <out-of-range>)) cannot be reloaded
with a single scratch register (which is all reload_insi has to
work with) no matter whether the destination (reload) register 
is reg 0 or any other register.


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);
}


Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de