| Message ID | 4C36F1BC.8080401@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
Fix subreg handling of HARD_FRAME_POINTER_REGNUM and revert a Thumb patch which seems to work around the problem: http://gcc.gnu.org/ml/gcc-patches/2010-07/msg00756.html Bernd
Fix subreg handling of HARD_FRAME_POINTER_REGNUM (not an ARM patch, needs review) and revert a Thumb patch which seems to work around the problem: http://gcc.gnu.org/ml/gcc-patches/2010-07/msg00756.html Bernd
On 07/23/2010 03:21 AM, Bernd Schmidt wrote: > Fix subreg handling of HARD_FRAME_POINTER_REGNUM (not an ARM patch, > needs review) and revert a Thumb patch which seems to work around the > problem: > > http://gcc.gnu.org/ml/gcc-patches/2010-07/msg00756.html The reload patch looks ok. The arm patch looks... odd. What use is a define_expand that only aborts? Why not delete the pattern entirely, particularly since it isn't a pattern name known by the middle-end? r~
On 07/28/2010 06:57 PM, Richard Henderson wrote: > On 07/23/2010 03:21 AM, Bernd Schmidt wrote: >> Fix subreg handling of HARD_FRAME_POINTER_REGNUM (not an ARM patch, >> needs review) and revert a Thumb patch which seems to work around the >> problem: >> >> http://gcc.gnu.org/ml/gcc-patches/2010-07/msg00756.html > > The reload patch looks ok. Thanks! > The arm patch looks... odd. What use is a define_expand that > only aborts? Why not delete the pattern entirely, particularly > since it isn't a pattern name known by the middle-end? Yeah. I'm just reverting it to a previous state; I was hoping the other Richard would either point out why we need the pattern or suggest we remove it. Bernd
On Wed, 2010-07-28 at 19:03 +0200, Bernd Schmidt wrote: > On 07/28/2010 06:57 PM, Richard Henderson wrote: > > On 07/23/2010 03:21 AM, Bernd Schmidt wrote: > >> Fix subreg handling of HARD_FRAME_POINTER_REGNUM (not an ARM patch, > >> needs review) and revert a Thumb patch which seems to work around the > >> problem: > >> > >> http://gcc.gnu.org/ml/gcc-patches/2010-07/msg00756.html > > > > The reload patch looks ok. > > Thanks! > > > The arm patch looks... odd. What use is a define_expand that > > only aborts? Why not delete the pattern entirely, particularly > > since it isn't a pattern name known by the middle-end? > > Yeah. I'm just reverting it to a previous state; I was hoping the other > Richard would either point out why we need the pattern or suggest we > remove it. This is part of the reload_{in,out}hi sequences. These have always blown my mind away and I've never fully understood when and why these are really needed. In thumb1 you potentially need some sort of recovery sequence if the compiler needs to spill a register held in r8-r14 (since these registers can't be stored directly to memory), but why it really needs a secondary reload escapes me -- the compiler should just copy the register to a lo-reg and then store that (or vice-versa for a load). I suggest we get rid of the pattern entirely, and the code chain that can end up calling it (ie thumb_reload_out_hi. We should probably also take a look at the reload code in arm.md at some point: it still uses the now deprecated reload support hooks and the infrastructure that (I think) Joern wrote a few years back has never been implemented on ARM. R.
Index: rtlanal.c =================================================================== --- rtlanal.c (revision 161987) +++ rtlanal.c (working copy) @@ -3297,8 +3297,7 @@ simplify_subreg_regno (unsigned int xreg /* We shouldn't simplify stack-related registers. */ if ((!reload_completed || frame_pointer_needed) - && (xregno == FRAME_POINTER_REGNUM - || xregno == HARD_FRAME_POINTER_REGNUM)) + && xregno == FRAME_POINTER_REGNUM) return -1; if (FRAME_POINTER_REGNUM != ARG_POINTER_REGNUM Index: config/arm/arm.md =================================================================== --- config/arm/arm.md (revision 161987) +++ config/arm/arm.md (working copy) @@ -5666,17 +5666,9 @@ (define_expand "thumb_movhi_clobber" (match_operand:HI 1 "register_operand" "")) (clobber (match_operand:DI 2 "register_operand" ""))] "TARGET_THUMB1" - " - if (strict_memory_address_p (HImode, XEXP (operands[0], 0)) - && REGNO (operands[1]) <= LAST_LO_REGNUM) - { - emit_insn (gen_movhi (operands[0], operands[1])); - DONE; - } - /* XXX Fixme, need to handle other cases here as well. */ +{ gcc_unreachable (); - " -) +}) ;; We use a DImode scratch because we may occasionally need an additional ;; temporary if the address isn't offsettable -- push_reload doesn't seem
While testing some other Thumb-1 changes, I got an abort while generating thumb_movhi_clobber for a secondary reload. The pattern calls gcc_unreachable when it finds a case it can't handle. There's a FIXME there that other cases might need to be handled, but looking closer, the entire pattern seems bogus; it requests two scratch regs (one DImode reg) and doesn't use them. The circumstances in which we arrive there also seemed suspect. We were reloading an insn of the form (set (mem:HI (some address)) (subreg:HI (reg:SI 7))) Reload did _not_ choose the reg->mem alternative of the movhi pattern, which is crazy. The reason it didn't is that reg 7 is the hard frame pointer, and simplify_subreg_regno didn't want to simplify the subreg here (-fomit-frame-pointer was on of course). So, find_reloads set force_reload for the register operand, which seems to have been enough to confuse its cost calculations. Fixed by the first part of this patch; I see no reason to treat the hard frame pointer register specially - it's typically just a general reg. If FRAME_POINTER_REGNUM == HARD_FRAME_POINTER_REGNUM, the test will have the same effect as before, which is probably still wrong, but for now it's a safe change to make. The other part removes the strange-looking part frmo the thumb_movhi_clobber register and restores it to just a call to gcc_unreachable. This is still ugly, but it's essentially the state it was in before http://gcc.gnu.org/ml/gcc-patches/2005-08/msg01210.html which presumably changed this because of the bug I'm fixing here. Regression tested, together with some other patches, on qemu-system-armv7-3/arch=armv7-a/thumb qemu-system-armv7-3/thumb qemu-system-armv7-3 which only showed up one unrelated problem for which I'm testing a fix. Ok (rtlanal and ARM parts)? Bernd * rtlanal.c (simplify_subreg_regno): Don't treat HARD_FRAME_POINTER_REGNUM specially. * config/arm/arm.md (thumb_movhi_clobber): Restore previous version of the pattern that always calls gcc_unreachable.