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

[Richard Earnshaw <rearnsha at arm dot com>]

> > No, it just means you are papering over a deficiency elsewhere in reload.  
> > LEGITIMIZE_RELOAD_ADDRESS is like LEGITIMIZE_ADDRESS in this respect, it 
> > *should not be needed* to generate correct code; it's too crude an 
> > instrument.
> 
> I can see the point that LEGITIMIZE_RELOAD_ADDRESS is not the best
> place to take care of this issue from a design point of view.  I was
> only looking at the existing facilities to implement the change and
> this was the only one that looked reasonable.
> 
> Also, I had the impression from the comment before the original
> THUMB_LEGITIMIZE_RELOAD_ADDRESS that the code was already _fixing_
> addresses by reloading them (the HImode SP + large_offset_addr case).
> I tried to find out more about the origin of this code from the late
> thumb.h but I couldn't find the corresponding posting on gcc-patches.
> 
> > I've been thinking about this particular case, but the fix is quite 
> > complex and involves introducing a new macro that allows the index reg 
> > class to be based both on the mode of access and the class of base 
> > register in use.  Unfortunately I don't have a patch available for this 
> > yet.
> 
> 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.

R.