Re: find_reloads_address_1 question

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

Joern Rennecke wrote:
> 
> > However, if there is a canonical form for addresses that reload
> > depends upon (across all architectures), then I guess combine
> > would be at fault here for not properly canonifying addresses
> > after substing into them ...  I'll reexamine my test case and
> > try to find out what's going on here.
> 
> Yes.  Constants should be combined, and come last in a plus.
> So in this case, combine should have generated an address like:
> (plus (plus (reg) (reg)) (const))

Hmm.  Unfortunately I wasn't able to reproduce my original 
problem; either it doesn't occur that way anymore because
something else changed, or I had been misinterpreting things :-/
In any case, thanks for clarifying this.

However, with a different test case I now get an ICE because
of a non-canonical address; it looks like reload itself 
changes a canonical address into a non-canonical one.

What happens is that stupid register allocation decides to 
allocate a pseudo to register %r0 which is invalid as base
or index register on our architecture.  This changes a formerly
valid address  
  (plus (plus (reg 11) (reg 78)) (const_int 124))
into the invalid address
  (plus (plus (reg 11) (reg 0)) (const_int 124))

Note that reg 11 is the frame pointer.

When find_reloads_address is called on this address, one
of the special cases hits, the one starting with this comment:

  /* If we have an indexed stack slot, there are three possible reasons why
     it might be invalid: The index might need to be reloaded, the address
     might have been made by frame pointer elimination and hence have a
     constant out of range, or both reasons might apply.

     We can easily check for an index needing reload, but even if that is the
     case, we might also have an invalid constant.  To avoid making the
     conservative assumption and requiring two reloads, we see if this address
     is valid when not interpreted strictly.  If it is, the only problem is
     that the index needs a reload and find_reloads_address_1 will take care
     of it.

Now, first of all this special case should really not hit here,
because the address in question really needs only a reload of
the index register (%r0).  However, because register 0 is a hard
register that cannot be used as index, this address is not valid
even in a non-strict sense!  Therefore, this code in 
find_reloads_address draws an incorrect conclusion.

Now this alone would just lead to a superfluous reload.  But it is
in fact even worse, because the code *modifies* the originial insn
even if replace_reloads is not set:  
      *loc = ad = gen_rtx_PLUS (GET_MODE (ad),
                                plus_constant (XEXP (XEXP (ad, 0), 0),
                                               INTVAL (XEXP (ad, 1))),
                                XEXP (XEXP (ad, 0), 1));

This means that on the first call of find_reloads, the address
is changed to 
   (plus (plus (reg 11) (const_int 124)) (reg 0))
which is non-canonical.

When find_reloads is later called with replace set to one,
find_reloads_address will no longer consider that special
case to apply, because the format of the address now doesn't
fit anymore.  Therefore it will simply generate a regular 
reload for register 0, and change the address to
   (plus (plus (reg 11) (const_int 124)) (reg 1))

Now this address could be represented by a legal machine
instruction, but the backend rejects it with an ICE in
print_operand due to the stricter checks for canonical 
addresses I've added ...

Is there something in the backend I can do to fix this?
(E.g. accept non-canonical addresses in print_operand,
or allow register 0 in REG_OK_FOR_INDEX_P in the non-strict
case ...)  Or should reload behave differently here?

Note that I can reproduce the problem only on 2.95.3 and not
on 3.0.2 or mainline.  However, this appears to be the case
only because register pressure is much lower on 3.* due to
better register allocation, and hence register 0 is never
even used ...  The reload code in question is unchanged
between 2.95.3 and the CVS head.

Bye,
Ulrich

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