Patchwork A problem simplifying subregs of the hard frame pointer

login
register
mail settings
Submitter Bernd Schmidt
Date July 9, 2010, 9:54 a.m.
Message ID <4C36F1BC.8080401@codesourcery.com>
Download mbox | patch
Permalink /patch/58381/
State New
Headers show

Comments

Bernd Schmidt - July 9, 2010, 9:54 a.m.
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.
Bernd Schmidt - July 16, 2010, 11:04 a.m.
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
Bernd Schmidt - July 23, 2010, 10:21 a.m.
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
Richard Henderson - July 28, 2010, 4:57 p.m.
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~
Bernd Schmidt - July 28, 2010, 5:03 p.m.
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
Richard Earnshaw - July 31, 2010, 11:45 a.m.
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.

Patch

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