Re: Help with reload and naked constant sum causing ICE

[hutchinsonandy at aim dot com]

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