Help with reload and naked constant sum causing ICE

Help with reload and naked constant sum causing ICE

[Andy H]

Hi,

I am am tracking down ICE bug in reload during global register allocation.

I have managed to find root of problem but I am not sure how it should 
work correctly. I would appreciate anyones advise on this.

ICE is from assert  in push_reload  to ensure that pseudos  have been 
replaced with hard register or constants. (reload.c  line 940 or so)
It fails because pseudo equivalent of constant is not (yet)  replaced 
for IN.

The sequence of event that causes this to occur is something like this.

1) Psuedo p68 is marked as equivalent to constant
reg_equiv_constant[68]=(symbol_ref:HI ("chk_fail_buf") [flags 0x40] 
<var_decl 0x7fe00000 chk_fail_buf>)

2) p68+2 is also needed
(plus:HI (reg/f:HI 68)
    (const_int 2 [0x2]))

3) find_reloads_address  is given p68+2 by find_reloads()

4)  p68+2 is  STRICTLY checked as memory address - and target rejects it 
as it has no outer constant and contains psuedo.
  if (strict_memory_address_p (mode, ad))

5) find_reloads_address then uses LEGITIMIZE_RELOAD_ADDRESS to fix problem

6) LEGITIMIZE_RELOAD_ADDRESS calls push_reload  to reload p68 using  
BASE_POINTER

7)ICE


I think there is something wrong at step 4  but according to manual  
address p68+2 should be rejected since it does not have outer constant 
wrapper.

<QUOTE>
Normally, constant addresses which are the sum of a |symbol_ref| and an 
integer are stored inside a |const| RTX to mark them as constant. 
Therefore, there is no need to recognize such sums specifically as 
legitimate addresses. Normally you would simply recognize any |const| as 
legitimate.

Usually |PRINT_OPERAND_ADDRESS| is not prepared to handle constant sums 
that are not marked with |const|. It assumes that a naked |plus| 
indicates indexing. If so, then you /mus/reject such naked constant sums 
as illegitimate addresses, so that none of them will be given to 
|PRINT_OPERAND_ADDRESS|.
// <QUOTE>

But I cannot find any const RTX wrappers being  applied. The rest of 
reload does not seem to expect a wrapper.

Perhaps LEGITIMIZE_RELOAD should fix problem - though that seems odd.

Can someone point in right direction? 

best regards

Andy

Re: Help with reload and naked constant sum causing ICE

[Richard Sandiford]

Andy H <hutchinsonandy@aim.com> writes:
> I am am tracking down ICE bug in reload during global register allocation.
>
> I have managed to find root of problem but I am not sure how it should 
> work correctly. I would appreciate anyones advise on this.
>
> ICE is from assert  in push_reload  to ensure that pseudos  have been 
> replaced with hard register or constants. (reload.c  line 940 or so)
> It fails because pseudo equivalent of constant is not (yet)  replaced 
> for IN.
>
> The sequence of event that causes this to occur is something like this.
>
> 1) Psuedo p68 is marked as equivalent to constant
> reg_equiv_constant[68]=(symbol_ref:HI ("chk_fail_buf") [flags 0x40] 
> <var_decl 0x7fe00000 chk_fail_buf>)
>
> 2) p68+2 is also needed
> (plus:HI (reg/f:HI 68)
>     (const_int 2 [0x2]))
>
> 3) find_reloads_address  is given p68+2 by find_reloads()
>
> 4)  p68+2 is  STRICTLY checked as memory address - and target rejects it 
> as it has no outer constant and contains psuedo.
>   if (strict_memory_address_p (mode, ad))
>
> 5) find_reloads_address then uses LEGITIMIZE_RELOAD_ADDRESS to fix problem
>
> 6) LEGITIMIZE_RELOAD_ADDRESS calls push_reload  to reload p68 using  
> BASE_POINTER
>
> 7)ICE
> [...]
> Perhaps LEGITIMIZE_RELOAD should fix problem - though that seems odd.

TBH, it sounds like the opposite: LEGITIMIZE_RELOAD_ADDRESS should
not be handling this address at all.  If L_R_A does nothing with it,
the normal reload handling will first try:

  (const:HI (plus:HI (symbol_ref:HI ("chk_fail_buf") (const_int 2))))

If that's legitimate, that's the address you'll get.  If it isn't,
the normal reload handling will reload the symbol_ref into a
base register of class:

  MODE_CODE_BASE_REG_CLASS (mem_mode, PLUS, CONST_INT)

And it sounds from your message like that's exactly what you want.

Richard

Re: Help with reload and naked constant sum causing ICE

[Andy H]

Thank you very much for reply. reload is such a lonely place!
> TBH, it sounds like the opposite: LEGITIMIZE_RELOAD_ADDRESS should
> not be handling this address at all. 
Yes but reload will not do anything before call to L_R_A. So in practice 
that would mean L_R_A has
to check reg_equiv_constant[regno] to see if register were a constant 
and do nothing if it is.

>  If L_R_A does nothing with it,
> the normal reload handling will first try:
>
>   (const:HI (plus:HI (symbol_ref:HI ("chk_fail_buf") (const_int 2))))
>   

Are you sure? I think it will try

(plus:HI (symbol_ref:HI ("chk_fail_buf") (const_int 2)))

I could be wrong so I will rerun to check this. If const is missing it 
will  be  rejected as valid address.
Though I note parts of reload do not impose this check.

> If that's legitimate, that's the address you'll get.  If it isn't,
> the normal reload handling will reload the symbol_ref into a
> base register of class:
>
>   MODE_CODE_BASE_REG_CLASS (mem_mode, PLUS, CONST_INT)
>
> And it sounds from your message like that's exactly what you want.
>   
Not really. - though that might be what happens if cons wrapper is missing.
We can take any relocatable expressions. So first choice is a good one.

> Richard
>   

Re: Help with reload and naked constant sum causing ICE

[Andy H]

> If L_R_A does nothing with it,
> the normal reload handling will first try:
>
>   (const:HI (plus:HI (symbol_ref:HI ("chk_fail_buf") (const_int 2))))
>   

This worked just as your described after I added test of 
reg_equiv_constant[] inside L_R_A .

So I guess that looks like  the fix for bug I posted.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=34641

To summarize

LEGITIMIZE_RELOAD_ADDRESS should now always check reg_equiv_constant
before it trying to do any push_reload of register.

Thanks for help!

Re: Help with reload and naked constant sum causing ICE

[Richard Sandiford]

Andy H <hutchinsonandy@aim.com> writes:
>> If L_R_A does nothing with it,
>> the normal reload handling will first try:
>>
>>   (const:HI (plus:HI (symbol_ref:HI ("chk_fail_buf") (const_int 2))))
>>   
>
> This worked just as your described after I added test of 
> reg_equiv_constant[] inside L_R_A .
>
> So I guess that looks like  the fix for bug I posted.
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=34641
>
> To summarize
>
> LEGITIMIZE_RELOAD_ADDRESS should now always check reg_equiv_constant
> before it trying to do any push_reload of register.

TBH, I still think AVR is doing far too much in L_R_A.  To quote
the current version:

#define LEGITIMIZE_RELOAD_ADDRESS(X, MODE, OPNUM, TYPE, IND_LEVELS, WIN)    \
do {									    \
  if (1&&(GET_CODE (X) == POST_INC || GET_CODE (X) == PRE_DEC))	    \
    {									    \
      push_reload (XEXP (X,0), XEXP (X,0), &XEXP (X,0), &XEXP (X,0),	    \
	           POINTER_REGS, GET_MODE (X),GET_MODE (X) , 0, 0,	    \
		   OPNUM, RELOAD_OTHER);				    \
      goto WIN;								    \
    }									    \

Why does AVR need different POST_INC and PRE_DEC handling from
other auto-inc targets?  This at least deserves a comment.

But really, you should just let reload handle this case.
Does reload currently lack some support you need?

  if (GET_CODE (X) == PLUS						    \
      && REG_P (XEXP (X, 0))						    \
      && GET_CODE (XEXP (X, 1)) == CONST_INT				    \
      && INTVAL (XEXP (X, 1)) >= 1)					    \
    {									    \
      int fit = INTVAL (XEXP (X, 1)) <= (64 - GET_MODE_SIZE (MODE));	    \
      if (fit)								    \
	{								    \
          if (reg_equiv_address[REGNO (XEXP (X, 0))] != 0)		    \
	    {								    \
	      int regno = REGNO (XEXP (X, 0));				    \
	      rtx mem = make_memloc (X, regno);				    \
	      push_reload (XEXP (mem,0), NULL, &XEXP (mem,0), NULL,         \
		           POINTER_REGS, Pmode, VOIDmode, 0, 0,		    \
		           1, ADDR_TYPE (TYPE));			    \
	      push_reload (mem, NULL_RTX, &XEXP (X, 0), NULL,		    \
		           BASE_POINTER_REGS, GET_MODE (X), VOIDmode, 0, 0, \
		           OPNUM, TYPE);				    \
	      goto WIN;							    \
	    }								    \
	  push_reload (XEXP (X, 0), NULL_RTX, &XEXP (X, 0), NULL,	    \
		       BASE_POINTER_REGS, GET_MODE (X), VOIDmode, 0, 0,	    \
		       OPNUM, TYPE);					    \
          goto WIN;							    \
	}								    \
      else if (! (frame_pointer_needed && XEXP (X,0) == frame_pointer_rtx)) \
	{								    \
	  push_reload (X, NULL_RTX, &X, NULL,				    \
		       POINTER_REGS, GET_MODE (X), VOIDmode, 0, 0,	    \
		       OPNUM, TYPE);					    \
          goto WIN;							    \
	}								    \
    }									    \
} while(0)

These too don't make much immediate sense to me.  What are they trying
to do that reload wouldn't do otherwise?

Rather than duplicating more of reload's checks in this macro, I think
it would be better to strip it down as far as possible.

There's a bit of self-interest here.  It makes it very hard to work on
reload if ports try to duplicate so much of the logic in target-specific
files.

Richard

Re: Help with reload and naked constant sum causing ICE

[hutchinsonandy]

Richard,

I appreciate the extra input.

I agree with what you say. The  target should not be  doing middle-end 
stuff .

The inc/dec and (Rxx) != (frame pointer)  parts just reload using 
pointer class
which is a one extra register than base pointers but the extra reg 
cannot take offset.

However, I dont know what reload can - or cannot do - (but I'm learning 
that every day)
I do not know  fully what was intended by all of AVR  L_R_A and how 
this might now be redundant, useful or wrong.

I already have patch with maintainer that takes out a chunk of this to 
fix another (spill) bug.

I have copied Anatoly for comment, and I promise to revisit this again 
after reviewing reload capabilities.


Andy


-----Original Message-----
From: Richard Sandiford <rdsandiford@googlemail.com>
To: Andy H <hutchinsonandy@aim.com>
Cc: GCC Development <gcc@gcc.gnu.org>; rdsandiford@googlemail.com
Sent: Wed, 28 May 2008 3:00 pm
Subject: Re: Help with reload and naked constant sum causing ICE



Andy H <hutchinsonandy@aim.com> writes:
>> If L_R_A does nothing with it,
>> the normal reload handling will first try:
>>
>>   (const:HI (plus:HI (symbol_ref:HI ("chk_fail_buf") (const_int 2))))
>>
>
> This worked just as your described after I added test of
> reg_equiv_constant[] inside L_R_A .
>
> So I guess that looks like  the fix for bug I posted.
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=34641
>
> To summarize
>
> LEGITIMIZE_RELOAD_ADDRESS should now always check reg_equiv_constant
> before it trying to do any push_reload of register.

TBH, I still think AVR is doing far too much in L_R_A.  To quote
the current version:

#define LEGITIMIZE_RELOAD_ADDRESS(X, MODE, OPNUM, TYPE, IND_LEVELS, 
WIN)    \
do {                                        \
  if (1&&(GET_CODE (X) == POST_INC || GET_CODE (X) == PRE_DEC))     \
    {                                       \
       push_reload (XEXP (X,0), XEXP (X,0), &XEXP (X,0), &XEXP (X,0),    
    \
               POINTER_REGS, GET_MODE (X),GET_MODE (X) , 0, 0,      \
           OPNUM, RELOAD_OTHER);                    \
      goto WIN;                                 \
    }                                       \

Why does AVR need different POST_INC and PRE_DEC handling from
other auto-inc targets?  This at least deserves a comment.

But really, you should just let reload handle this case.
Does reload currently lack some support you need?

  if (GET_CODE (X) == PLUS                          \
      && REG_P (XEXP (X, 0))                            \
      && GET_CODE (XEXP (X, 1)) == CONST_INT                    \
      && INTVAL (XEXP (X, 1)) >= 1)                     \
    {                                       \
       int fit = INTVAL (XEXP (X, 1)) <= (64 - GET_MODE_SIZE (MODE));    
    \
      if (fit)                                  \
    {                                   \
          if (reg_equiv_address[REGNO (XEXP (X, 0))] != 0)          \
        {                                   \
          int regno = REGNO (XEXP (X, 0));                  \
          rtx mem = make_memloc (X, regno);                 \
           push_reload (XEXP (mem,0), NULL, &XEXP (mem,0), NULL,         
\
                   POINTER_REGS, Pmode, VOIDmode, 0, 0,         \
                   1, ADDR_TYPE (TYPE));                \
          push_reload (mem, NULL_RTX, &XEXP (X, 0), NULL,           \
                   BASE_POINTER_REGS, GET_MODE (X), VOIDmode, 0, 0, \
                   OPNUM, TYPE);                    \
          goto WIN;                             \
        }                                   \
      push_reload (XEXP (X, 0), NULL_RTX, &XEXP (X, 0), NULL,       \
               BASE_POINTER_REGS, GET_MODE (X), VOIDmode, 0, 0,     \
               OPNUM, TYPE);                        \
          goto WIN;                             \
    }                                   \
       else if (! (frame_pointer_needed && XEXP (X,0) == 
frame_pointer_rtx)) \
    {                                   \
      push_reload (X, NULL_RTX, &X, NULL,                   \
               POINTER_REGS, GET_MODE (X), VOIDmode, 0, 0,      \
               OPNUM, TYPE);                        \
          goto WIN;                             \
    }                                   \
    }                                       \
} while(0)

These too don't make much immediate sense to me.  What are they trying
to do that reload wouldn't do otherwise?

Rather than duplicating more of reload's checks in this macro, I think
it would be better to strip it down as far as possible.

There's a bit of self-interest here.  It makes it very hard to work on
reload if ports try to duplicate so much of the logic in target-specific
files.

Richard

Re: Help with reload and naked constant sum causing ICE

[Richard Sandiford]

hutchinsonandy@aim.com writes:
> I have copied Anatoly for comment, and I promise to revisit this again 
> after reviewing reload capabilities.

Thanks.

Looking back, my message sounded like I was holding you personally
responsible for the current AVR macro.  Didn't mean to do that ;)

Richard

Re: Help with reload and naked constant sum causing ICE

[Jeff Law]

Richard Sandiford wrote:
> Andy H <hutchinsonandy@aim.com> writes:
>>> If L_R_A does nothing with it,
>>> the normal reload handling will first try:
>>>
>>>   (const:HI (plus:HI (symbol_ref:HI ("chk_fail_buf") (const_int 2))))
>>>   
>> This worked just as your described after I added test of 
>> reg_equiv_constant[] inside L_R_A .
>>
>> So I guess that looks like  the fix for bug I posted.
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=34641
>>
>> To summarize
>>
>> LEGITIMIZE_RELOAD_ADDRESS should now always check reg_equiv_constant
>> before it trying to do any push_reload of register.
> 
> TBH, I still think AVR is doing far too much in L_R_A.  To quote
> the current version:
[...]
Not only should there be some comments, those comments should clearly 
explain how L_R_A is improving the generated code.  That is LRA's job, 
to implement target specific reload strategies which improve the 
generated code.  If the AVR port is using L_R_A for *correctness*, then 
the AVR port is broken.

Interestingly enough, we've seen a fair amount of commonality in L_R_A 
implementations (particularly in dealing with out-of-range offsets in 
reg+d addressing modes).  It probably wouldn't be terribly difficult to 
factor that code into reload itself and do away with a significant hunk 
of the existing L_R_A implementations.


Jeff

Re: Help with reload and naked constant sum causing ICE

[Denis Chertykov]

2008/5/29 Jeff Law <law@redhat.com>:
> Richard Sandiford wrote:
>>
>> Andy H <hutchinsonandy@aim.com> writes:
>>>>
>>>> If L_R_A does nothing with it,
>>>> the normal reload handling will first try:
>>>>
>>>>  (const:HI (plus:HI (symbol_ref:HI ("chk_fail_buf") (const_int 2))))
>>>>
>>>
>>> This worked just as your described after I added test of
>>> reg_equiv_constant[] inside L_R_A .
>>>
>>> So I guess that looks like  the fix for bug I posted.
>>>
>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=34641
>>>
>>> To summarize
>>>
>>> LEGITIMIZE_RELOAD_ADDRESS should now always check reg_equiv_constant
>>> before it trying to do any push_reload of register.
>>
>> TBH, I still think AVR is doing far too much in L_R_A.  To quote
>> the current version:
>
> [...]
> Not only should there be some comments, those comments should clearly
> explain how L_R_A is improving the generated code.  That is LRA's job, to
> implement target specific reload strategies which improve the generated
> code.  If the AVR port is using L_R_A for *correctness*, then the AVR port
> is broken.

I wrote L_R_A for *correctness*, so in your terms AVR port is broken.
It's because AVR have only 3 pointer registers (X,Y,Z) and only 2 of
them Y and Z are base pointers.
Y is a frame pointer, so only one general base pointer - Z.
Offset also very limited -63 to +63.
In L_R_A I play with POINTER_REGS (X,Y,Z) and BASE_POINTER_REGS (Y,Z).
My experiments with reload show me that reload can't handle all
difficult situations.
Also, Jeff I know that you think that reload can.
GCC havn't something like (define_address ...) and reload can't handle
POINTER_REGS and
BASE_POINTER_REGS differently. In my version of L_R_A I tried to do it.

Denis.

Re: Help with reload and naked constant sum causing ICE

[hutchinsonandy]

Again thank you and Denis for your comment.

Here is what I deduce from code and Denis comments - I am sure he (and 
others) will correct me if wrong :-)


The main issue is that we have one pointer register that cannot take 
offset and two base pointers  with limited offset (0-63).

  Reload provides no means to pick the right pointer register. To do so 
would require a "preffered base pointer" mechanism.

AVR  L_R_A sole purpose is  substituting POINTER class where there are 
oppertunities to do so.

BUT as  L_R_A is in  middle of find_reloads_address, we have a problem 
in that some normal reload solutions have already been applied and some 
have not.

This may mean  that 1) We never get here - so miss POINTER oppertunity 
or 2) Miss even better solutions that are applied latter.
3)Have code vulnerable to reload changes.


I have added comment below to explain the parts.
I'll probably need to add some debug hooks to check what parts still 
contribute.

best regards

Andy



#define LEGITIMIZE_RELOAD_ADDRESS(X, MODE, OPNUM, TYPE, IND_LEVELS, 
WIN)    \
do {                                        \
  if (1&&(GET_CODE (X) == POST_INC || GET_CODE (X) == PRE_DEC))     \
    {                                       \
       push_reload (XEXP (X,0), XEXP (X,0), &XEXP (X,0), &XEXP (X,0),    
    \
               POINTER_REGS, GET_MODE (X),GET_MODE (X) , 0, 0,      \
           OPNUM, RELOAD_OTHER);                    \
      goto WIN;                                 \
    }                                       \


The intent is to use POINTER class registers for INC/DEC so we have one 
more register available than BASE_POINTER

Has  INC/DEC been handled already making this part redundant?
Are there any other solutions that this code skips?



  if (GET_CODE (X) == PLUS                          \
      && REG_P (XEXP (X, 0))                            \
      && GET_CODE (XEXP (X, 1)) == CONST_INT                    \
      && INTVAL (XEXP (X, 1)) >= 1)                     \
    {                                       \
       int fit = INTVAL (XEXP (X, 1)) <= (64 - GET_MODE_SIZE (MODE));    
    \
      if (fit)                                  \
    {                                   \
          if (reg_equiv_address[REGNO (XEXP (X, 0))] != 0)          \
        {                                   \
          int regno = REGNO (XEXP (X, 0));                  \
          rtx mem = make_memloc (X, regno);                 \
           push_reload (XEXP (mem,0), NULL, &XEXP (mem,0), NULL,         
\
                   POINTER_REGS, Pmode, VOIDmode, 0, 0,         \
                   1, ADDR_TYPE (TYPE));                \
          push_reload (mem, NULL_RTX, &XEXP (X, 0), NULL,           \
                   BASE_POINTER_REGS, GET_MODE (X), VOIDmode, 0, 0, \
                   OPNUM, TYPE);                    \
          goto WIN;                             \
        }

Im struggling with this part. however, the POINTER class is an 
advantageous in the first reloading the address from memory
- where we would not want to waste BASE POINTER and also create overlap 
problem.

The implication of using this code is that without it we cannot  catch 
the inner reload POINTER oppertunity  if we leave reload to decompose  
BASE+offset. I think this is correct :-(


                               \
      push_reload (XEXP (X, 0), NULL_RTX, &XEXP (X, 0), NULL,       \
               BASE_POINTER_REGS, GET_MODE (X), VOIDmode, 0, 0,     \
               OPNUM, TYPE);                        \
          goto WIN;

This part is duplicating reload and should not be here (I have patch 
pending with maintainer that removes this - for different bug)

                      \
    }                                   \
       else if (! (frame_pointer_needed && XEXP (X,0) == 
frame_pointer_rtx)) \
    {                                   \
      push_reload (X, NULL_RTX, &X, NULL,                   \
               POINTER_REGS, GET_MODE (X), VOIDmode, 0, 0,      \
               OPNUM, TYPE);                        \
          goto WIN;                             \
    }                                   \
    }                                       \

This case is where offset is out of range - so we allow POINTER class 
on the basis that this is no worse than BASE POINTER. However, we leave 
frame_pointer to reload.




} while(0)








-----Original Message-----
From: Denis Chertykov <chertykov@gmail.com>
To: Jeff Law <law@redhat.com>
Cc: Andy H <hutchinsonandy@aim.com>; GCC Development <gcc@gcc.gnu.org>; 
rdsandiford@googlemail.com
Sent: Thu, 29 May 2008 3:23 am
Subject: Re: Help with reload and naked constant sum causing ICE



2008/5/29 Jeff Law <law@redhat.com>:
> Richard Sandiford wrote:
>>
>> Andy H <hutchinsonandy@aim.com> writes:
>>>>
>>>> If L_R_A does nothing with it,
>>>> the normal reload handling will first try:
>>>>
>>>>  (const:HI (plus:HI (symbol_ref:HI ("chk_fail_buf") (const_int 
2))))
>>>>
>>>
>>> This worked just as your described after I added test of
>>> reg_equiv_constant[] inside L_R_A .
>>>
>>> So I guess that looks like  the fix for bug I posted.
>>>
>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=34641
>>>
>>> To summarize
>>>
>>> LEGITIMIZE_RELOAD_ADDRESS should now always check reg_equiv_constant
>>> before it trying to do any push_reload of register.
>>
>> TBH, I still think AVR is doing far too much in L_R_A.  To quote
>> the current version:
>
> [...]
> Not only should there be some comments, those comments should clearly
> explain how L_R_A is improving the generated code.  That is LRA's 
job, to
> implement target specific reload strategies which improve the 
generated
> code.  If the AVR port is using L_R_A for *correctness*, then the AVR 
port
> is broken.

I wrote L_R_A for *correctness*, so in your terms AVR port is broken.
It's because AVR have only 3 pointer registers (X,Y,Z) and only 2 of
them Y and Z are base pointers.
Y is a frame pointer, so only one general base pointer - Z.
Offset also very limited -63 to +63.
In L_R_A I play with POINTER_REGS (X,Y,Z) and BASE_POINTER_REGS (Y,Z).
My experiments with reload show me that reload can't handle all
difficult situations.
Also, Jeff I know that you think that reload can.
GCC havn't something like (define_address ...) and reload can't handle
POINTER_REGS and
BASE_POINTER_REGS differently. In my version of L_R_A I tried to do it.

Denis.

Re: Help with reload and naked constant sum causing ICE

[Denis Chertykov]

2008/5/29  <hutchinsonandy@aim.com>:
> Again thank you and Denis for your comment.
>
> Here is what I deduce from code and Denis comments - I am sure he (and
> others) will correct me if wrong :-)
>
>
> The main issue is that we have one pointer register that cannot take offset
> and two base pointers  with limited offset (0-63).

Yes.

Oops I'm sorry. I wrote -63 to +63 in my last mail it's wrong.
The right offset 0 to 63.

>
>  Reload provides no means to pick the right pointer register. To do so would
> require a "preffered base pointer" mechanism.

Yes.

>
> AVR  L_R_A sole purpose is  substituting POINTER class where there are
> oppertunities to do so.
>
> BUT as  L_R_A is in  middle of find_reloads_address, we have a problem in
> that some normal reload solutions have already been applied and some have
> not.

Yes.

> This may mean  that 1) We never get here - so miss POINTER oppertunity or 2)
> Miss even better solutions that are applied latter.
> 3)Have code vulnerable to reload changes.

Yes.

> I have added comment below to explain the parts.
> I'll probably need to add some debug hooks to check what parts still
> contribute.
>
> best regards
>
> Andy
>
>
>
> #define LEGITIMIZE_RELOAD_ADDRESS(X, MODE, OPNUM, TYPE, IND_LEVELS, WIN)
>  \
> do {                                        \
>  if (1&&(GET_CODE (X) == POST_INC || GET_CODE (X) == PRE_DEC))     \
>   {                                       \
>      push_reload (XEXP (X,0), XEXP (X,0), &XEXP (X,0), &XEXP (X,0),       \
>              POINTER_REGS, GET_MODE (X),GET_MODE (X) , 0, 0,      \
>          OPNUM, RELOAD_OTHER);                    \
>     goto WIN;                                 \
>   }                                       \
>
>
> The intent is to use POINTER class registers for INC/DEC so we have one more
> register available than BASE_POINTER
>
> Has  INC/DEC been handled already making this part redundant?
> Are there any other solutions that this code skips?

Really, I forgot why this part of L_R_A exists.
Look at  strange 'if (1&&'.
I remember that once I have disabled it (add 0&&), but after some
testing I have
enablet it again.

>  if (GET_CODE (X) == PLUS                          \
>     && REG_P (XEXP (X, 0))                            \
>     && GET_CODE (XEXP (X, 1)) == CONST_INT                    \
>     && INTVAL (XEXP (X, 1)) >= 1)                     \
>   {                                       \
>      int fit = INTVAL (XEXP (X, 1)) <= (64 - GET_MODE_SIZE (MODE));       \
>     if (fit)                                  \
>   {                                   \
>         if (reg_equiv_address[REGNO (XEXP (X, 0))] != 0)          \
>       {                                   \
>         int regno = REGNO (XEXP (X, 0));                  \
>         rtx mem = make_memloc (X, regno);                 \
>          push_reload (XEXP (mem,0), NULL, &XEXP (mem,0), NULL,         \
>                  POINTER_REGS, Pmode, VOIDmode, 0, 0,         \
>                  1, ADDR_TYPE (TYPE));                \
>         push_reload (mem, NULL_RTX, &XEXP (X, 0), NULL,           \
>                  BASE_POINTER_REGS, GET_MODE (X), VOIDmode, 0, 0, \
>                  OPNUM, TYPE);                    \
>         goto WIN;                             \
>       }
>
> Im struggling with this part. however, the POINTER class is an advantageous
> in the first reloading the address from memory
> - where we would not want to waste BASE POINTER and also create overlap
> problem.

Yes.

> The implication of using this code is that without it we cannot  catch the
> inner reload POINTER oppertunity  if we leave reload to decompose
>  BASE+offset. I think this is correct :-(
>
>
>                              \
>     push_reload (XEXP (X, 0), NULL_RTX, &XEXP (X, 0), NULL,       \
>              BASE_POINTER_REGS, GET_MODE (X), VOIDmode, 0, 0,     \
>              OPNUM, TYPE);                        \
>         goto WIN;
>
> This part is duplicating reload and should not be here (I have patch pending
> with maintainer that removes this - for different bug)
>
>                     \
>   }                                   \
>      else if (! (frame_pointer_needed && XEXP (X,0) == frame_pointer_rtx)) \
>   {                                   \
>     push_reload (X, NULL_RTX, &X, NULL,                   \
>              POINTER_REGS, GET_MODE (X), VOIDmode, 0, 0,      \
>              OPNUM, TYPE);                        \
>         goto WIN;                             \
>   }                                   \
>   }                                       \
>
> This case is where offset is out of range

and we must reload not-the-frame_pointer.

The frame_pointer can be handled by reload more optimal because we have
Z register (Z+offset).

>  - so we allow POINTER class on the
> basis that this is no worse than BASE POINTER. However, we leave
> frame_pointer to reload.

Because reload-pass can reload frame_pointer+big_offset because we have
anower one pointer+offser register (Z+offset).

Denis.