Re: [PATCH, arm] Fix gcc.c-torture/execute/930628-1.c -O3 for Thumb

[Adam Nemet <anemet at Lnxw dot COM>]

> > While this might be a good idea in general it won't fix the issue I am
> > seeing.  INDEX_REG_CLASS is already defined as LO_REGS.  Yet in
> > the problematic insn:
> > 
> > (insn:HI 551 710 552 13 0x40128370 (set (mem/s:SF (plus:SI (reg:SI 51)
> >                 (reg/f:SI 40)) [14 bdm S4 A32]) 
> >         (reg:SF 150)) 195 {*thumb_movsf_insn} (nil) 
> >     (expr_list:REG_EQUAL (const_double:SF -100663296) 
> > 
> > pseudo (reg 40) will be assigned to SP.  The reason for this is that
> > (reg 40) corresponds to the address of an array on the stack which
> > becomes SP after elimination.
> 
> No, the point is that REG_OK_FOR_INDEX is too dumb.  For Thumb, it simply 
> says that if the tested register is in LO_REGS then it's ok to use as an 
> index off any base register; that's clearly insufficient.  So the solution 
> is to have a more complex test than REG_OK_FOR_INDEX currently supplies 
> (and which degenerates to the REG_OK_FOR_INDEX definition on other 
> machines).  The test needs to take into account the class of the base 
> register and the mode of the access.  Once we provide that information as 
> well, then the compiler can correctly determine that SP+reg is not
> legal.
> 
> > 
> > Therefore, I think that if we reject the LEGITIMIZE_RELOAD_ADDRESS
> > idea we will have to refine the REG*_OK_FOR_BASE_P family and
> > eventually BASE_REG_CLASS to only allow SP as a base register if the
> > index is constant.
> > 
> > I am thinking of something along this line:
> > 
> > #define THUMB_REGNO_MODE_OK_FOR_BASE_P(REGNO, MODE, INDEX)      \
> >   (TEST_REGNO (REGNO, <=, LAST_LO_REGNUM)                       \
> >    || (GET_CODE (INDEX) == CONST_INT                            \
> >        && GET_MODE_SIZE (MODE) >= 4                             \ 
> >        && TEST_REGNO (REGNO, ==, STACK_POINTER_REGNUM))) 
> > 
> > If you agree with this approach, I can give it a try and come up with
> > a working version implementing this idea.
> 
> I think there will have to be some (hopefully fairly straight-forward) 
> changes to the reload code itself, I don't think we can do this cleanly 
> entirely in the ARM back-end.

Sure not.  If you take a look at my THUMB_REGNO_MODE_OK_FOR_BASE_P
above it has a new parameter: the rtx of the index.  To incorporate
this new form we will have to change the way this set of macros are
used.  The patch where the MODE parameter was added to these macros is
a good source of information as to what should be looked at.

My point was that we should probaly change the interface of the base
macros rather than the index macros.  Clearly, we only have to change
one of them.

#define MODE_INDEX_REG_CLASS(BASE)
  (TARGET_THUMB ?
   ((BASE == stack_pointer_rtx) ? NO_RESG :  LO_REGS) : GENERAL_REGS)

Returning NO_REGS from INDEX_REG_CLASS if the base is SP just doesn't
look like a straight-forward change to me.  I don't know if the
callers can handle this without too much intrusion.

Whereas changing the BASE_REG family looks more straight-forward to
me: 

#define MODE_BASE_REG_CLASS(MODE, INDEX)				\
    (TARGET_ARM ? GENERAL_REGS :					\
     (((MODE) == SImode							\
       && (GET_CODE (INDEX) != CONST_INT)) ? BASE_REGS : LO_REGS))

Adam