find_reloads_address_1 question

find_reloads_address_1 question

[Ulrich Weigand]

Hello,

in experimenting with different ways of what to allow as
legitimate addresses I've noticed a somewhat strange behaviour
of find_reloads_address_1:  if an address has a PLUS at the top
level, that routine tries some assumptions on how the components
of the sum ought to look like, but if the sum does *not* fit
any of these assumptions, the routine simply returns without
pushing any reloads; it does not even fall back to its default
behaviour of recurring over subexpressions.

Specifically I've tried to declare addresses of the form
 (plus (plus (reg) (const_int)) (plus (reg) (const_int)))
as legitimate, because I thought it can't hurt as they
can be transformed into a regular base + index + displacement
scheme, and it sometimes helps by allowing a match in combine
which would otherwise fail (because combine apparently doesn't 
try to normalize addresses after subst'ing into them).

But this caused bootstrap failure because reload simply
didn't reload the registers to hard registers and left
pseudos in ...

Am I just trying something stupid here, or would this be
considered a bug in reload?

Bye,
Ulrich

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

Re: find_reloads_address_1 question

[Joern Rennecke]

> Specifically I've tried to declare addresses of the form
>  (plus (plus (reg) (const_int)) (plus (reg) (const_int)))
...
> But this caused bootstrap failure because reload simply
> didn't reload the registers to hard registers and left
> pseudos in ...
> 
> Am I just trying something stupid here, or would this be
> considered a bug in reload?

Yes.  Both.  You are allowing non-canonical RTL, and AFAICT there is
no good reason to do this here.  On the other hand, gcc would be easier
to port if find_reloads_address either had a fallback method to reload
all kinds of addressing modes, or at least aborted for ones that it
doesn't handle.  Note that there is a comment at the start of this function
that hints at this defect:  ...this is not fully machine-customizable;... .

Re: find_reloads_address_1 question

[Ulrich Weigand]

Joern Rennecke wrote:

> > Specifically I've tried to declare addresses of the form
> >  (plus (plus (reg) (const_int)) (plus (reg) (const_int)))
> ...
> > But this caused bootstrap failure because reload simply
> > didn't reload the registers to hard registers and left
> > pseudos in ...
> > 
> > Am I just trying something stupid here, or would this be
> > considered a bug in reload?
> 
> Yes.  Both.  You are allowing non-canonical RTL, and AFAICT there is
> no good reason to do this here. 

Well, I tried this change because a pattern of the type above
was generated by combine through substing into an address.  The
combine attempt then failed because the address didn't pass the
LEGITIMATE_ADDRESS test, and so I thought it was my fault for not
accepting everything that can in fact be translated to a correct
address.

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.

> On the other hand, gcc would be easier
> to port if find_reloads_address either had a fallback method to reload
> all kinds of addressing modes, or at least aborted for ones that it
> doesn't handle.  Note that there is a comment at the start of this function
> that hints at this defect:  ...this is not fully machine-customizable;... .

After a closer look at find_reloads_address_1 I guess I understand
why it doesn't simply recurse over subexpressions: it tries to 
decide which operand of the PLUS to use as base and which as index
for those architectures where the sets of possible base and index
registers are not equal.  Do to so you basically have to analyse
both operands of the PLUS ... (This is not the case on our architecture,
which is probably why I overlooked this problem.)

But I certainly agree that aborting in this case would be preferable
to just leaving an incorrect address in.

Thanks,
Ulrich

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

Re: find_reloads_address_1 question

[Joern Rennecke]

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

Re: find_reloads_address_1 question

[Ulrich Weigand]

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

Re: find_reloads_address_1 question

[Joern Rennecke]

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

This is done assuming that the inner plus is going to be loaded into
a reload register. 
It would be interesting to find out why this has not happened.
Of course, even when this can be fixed, this leaves the issue of the extra
reload.

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

I think changing REG_OK_FOR_INDEX_P would be preferable if you want to work
around this problem in your backend.  You can check reload_in_progress to
avoid an impact on the register (class) preferences in the pre-reload phases.

Alternatively, when you have fixed the problem of not reloading the
inner plus, you can use LEGITIMZE_RELOAD_ADDRESS to tackle the
optimization issue.

A fix in reload at the point that you identified could be to replace the
index by a pseudo of the same mode before trying checking
memory_address_p.  In general, the index might be something that is not
a REG, e.g. a MEM, but then it would make even more sense to use a pseudo
to consider reloading the entire index into a suitable register.
There are two places in find_reloads_address where such a change would have
to be made, so it would make sense to have a helper function to do the
check including modifying and restoring the address.

Re: find_reloads_address_1 question

[Ulrich Weigand]

Joern Rennecke wrote:

>> 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.
>
>This is done assuming that the inner plus is going to be loaded into
>a reload register.
>It would be interesting to find out why this has not happened.

As I said, this is because the address is replaced already on the
*first* call to find_reloads (from within calculate_needs_all_insns)
which has 'replace' set to 0.  However, on the *second* call to
find_reloads (from within reload_as_needed), find_reloads_address
does not go into that special case anymore, because the now modified
address does not fit the pattern the 'if' is testing for anymore ...

Instead the address is passed to find_reloads_address_1 by default,
which then decides to simply reload reg 0.

To fix this, shouldn't the address replacement be done only if
'replace_reloads' is true?

>I think changing REG_OK_FOR_INDEX_P would be preferable if you want to
work
>around this problem in your backend.  You can check reload_in_progress to
>avoid an impact on the register (class) preferences in the pre-reload
phases.

OK, I'll try this.

>A fix in reload at the point that you identified could be to replace the
>index by a pseudo of the same mode before trying checking
>memory_address_p.  In general, the index might be something that is not
>a REG, e.g. a MEM, but then it would make even more sense to use a pseudo
>to consider reloading the entire index into a suitable register.
>There are two places in find_reloads_address where such a change would
have
>to be made, so it would make sense to have a helper function to do the
>check including modifying and restoring the address.

Ah, I see.  Would you consider something like the following patch to be
a proper fix, then?   (Note: I haven't bootstrapped/tested the patch yet,
only verified that it solves my testcase.)

I'm not sure about the replace_reloads parts of the fix.  Maybe the
whole case should not even be entered unless replace_reloads is set?


--- gcc/reload.c    2001/06/12 14:22:49  1.2
+++ gcc/reload.c    2001/10/18 13:27:44
@@ -4581,6 +4581,27 @@
   return tem;
 }

+/* Returns true if AD could be turned into a valid memory reference
+   to mode MODE by reloading the part pointed to by PART into a
+   register.  */
+
+static int
+maybe_memory_address_p (mode, ad, part)
+     enum machine_mode mode;
+     rtx ad;
+     rtx *part;
+{
+  int retv;
+  rtx tem = *part;
+  rtx reg = gen_rtx_REG (GET_MODE (tem), FIRST_PSEUDO_REGISTER);
+
+  *part = reg;
+  retv = memory_address_p (mode, ad);
+  *part = tem;
+
+  return retv;
+}
+
 /* Record all reloads needed for handling memory address AD
    which appears in *LOC in a memory reference to mode MODE
    which itself is found in location  *MEMREFLOC.
@@ -4879,12 +4900,14 @@
            || XEXP (XEXP (ad, 0), 0) == arg_pointer_rtx
 #endif
            || XEXP (XEXP (ad, 0), 0) == stack_pointer_rtx)
-       && ! memory_address_p (mode, ad))
+       && ! maybe_memory_address_p (mode, ad, &XEXP (XEXP (ad, 0), 1)))
     {
-      *loc = ad = gen_rtx_PLUS (GET_MODE (ad),
-                   plus_constant (XEXP (XEXP (ad, 0), 0),
-                               INTVAL (XEXP (ad, 1))),
-                 XEXP (XEXP (ad, 0), 1));
+      ad = gen_rtx_PLUS (GET_MODE (ad),
+               plus_constant (XEXP (XEXP (ad, 0), 0),
+                           INTVAL (XEXP (ad, 1))),
+               XEXP (XEXP (ad, 0), 1));
+      if (replace_reloads)
+    *loc = ad;
       find_reloads_address_part (XEXP (ad, 0), &XEXP (ad, 0),
BASE_REG_CLASS,
                     GET_MODE (ad), opnum, type, ind_levels);
       find_reloads_address_1 (mode, XEXP (ad, 1), 1, &XEXP (ad, 1), opnum,
@@ -4903,12 +4926,14 @@
            || XEXP (XEXP (ad, 0), 1) == arg_pointer_rtx
 #endif
            || XEXP (XEXP (ad, 0), 1) == stack_pointer_rtx)
-       && ! memory_address_p (mode, ad))
+       && ! maybe_memory_address_p (mode, ad, &XEXP (XEXP (ad, 0), 0)))
     {
-      *loc = ad = gen_rtx_PLUS (GET_MODE (ad),
-                   XEXP (XEXP (ad, 0), 0),
-                   plus_constant (XEXP (XEXP (ad, 0), 1),
-                               INTVAL (XEXP (ad, 1))));
+      ad = gen_rtx_PLUS (GET_MODE (ad),
+               XEXP (XEXP (ad, 0), 0),
+               plus_constant (XEXP (XEXP (ad, 0), 1),
+                        INTVAL (XEXP (ad, 1))));
+      if (replace_reloads)
+    *loc = ad;
       find_reloads_address_part (XEXP (ad, 1), &XEXP (ad, 1),
BASE_REG_CLASS,
                     GET_MODE (ad), opnum, type, ind_levels);
       find_reloads_address_1 (mode, XEXP (ad, 0), 1, &XEXP (ad, 0), opnum,



Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

--
  Dr. Ulrich Weigand
  Linux for S/390 Design & Development
  IBM Deutschland Entwicklung GmbH, Schoenaicher Str. 220, 71032 Boeblingen
  Phone: +49-7031/16-3727   ---   Email: Ulrich.Weigand@de.ibm.com