A problem simplifying subregs of the hard frame pointer

A problem simplifying subregs of the hard frame pointer

[Bernd Schmidt]

[-- Attachment #1: Type: text/plain, Size: 1831 bytes --]

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

[-- Attachment #2: subreg-problem.diff --]
[-- Type: text/plain, Size: 1536 bytes --]

	* 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.

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

Ping: A problem simplifying subregs of the hard frame pointer

[Bernd Schmidt]

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

Ping^2: A problem simplifying subregs of the hard frame pointer

[Bernd Schmidt]

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

Re: Ping^2: A problem simplifying subregs of the hard frame pointer

[Bernd Schmidt]

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

Re: Ping^2: A problem simplifying subregs of the hard frame pointer

[Richard Henderson]

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~

Re: Ping^2: A problem simplifying subregs of the hard frame pointer

[Richard Earnshaw]

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.

Re: Ping^2: A problem simplifying subregs of the hard frame pointer

[Bernd Schmidt]

[-- Attachment #1: Type: text/plain, Size: 1307 bytes --]

On 07/31/2010 01:45 PM, Richard Earnshaw wrote:
> 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.

Essentially, whenever reload may have to move something from A to B,
but can't do it without a scratch reg.

> 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).

And I think that happens in all normal cases.  For example, arithmetic
operations use "l" constraints which means reload regs are LO_REGS and
no scratch reg is required to move anything into or out of it.

> I suggest we get rid of the pattern entirely, and the code chain that
> can end up calling it (ie thumb_reload_out_hi.

As below?  I've had this in my last test run (before checking in the
other patches) and that was successful.  It has a slight change in that
it uses TARGET_32BIT instead of TARGET_ARM now.  It may be worthwhile as
an experiment trying to delete the arm branch as well to see if that
causes problems.


Bernd

[-- Attachment #2: del-unnecessary-pattern.diff --]
[-- Type: text/plain, Size: 3384 bytes --]

	* config/arm/arm.c (thumb_reload_out_hi, thumb_reload_in_hi):
	Delete.
	* config/arm/arm.md (thumb_movhi_clobber): Delete.
	(reload_outhi): Enabled only for TARGET_32BIT.  Remove Thumb
	branch.
	(reload_inhi): Likewise.
	* config/arm/arm-protos.h (thumb_reload_out_hi, thumb_reload_in_hi):
	Don't declare.
	
Index: gcc/config/arm/arm.c
===================================================================
--- gcc.orig/config/arm/arm.c
+++ gcc/config/arm/arm.c
@@ -20883,19 +20883,6 @@ thumb_expand_movmemqi (rtx *operands)
     }
 }
 
-void
-thumb_reload_out_hi (rtx *operands)
-{
-  emit_insn (gen_thumb_movhi_clobber (operands[0], operands[1], operands[2]));
-}
-
-/* Handle reading a half-word from memory during reload.  */
-void
-thumb_reload_in_hi (rtx *operands ATTRIBUTE_UNUSED)
-{
-  gcc_unreachable ();
-}
-
 /* Return the length of a function name prefix
     that starts with the character 'c'.  */
 static int
Index: gcc/config/arm/arm.md
===================================================================
--- gcc.orig/config/arm/arm.md
+++ gcc/config/arm/arm.md
@@ -5734,23 +5734,6 @@
   [(set_attr "predicable" "yes")]
 )
 
-(define_expand "thumb_movhi_clobber"
-  [(set (match_operand:HI     0 "memory_operand"   "")
-	(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
 ;; to take any notice of the "o" constraints on reload_memory_operand operand.
@@ -5758,27 +5741,21 @@
   [(parallel [(match_operand:HI 0 "arm_reload_memory_operand" "=o")
 	      (match_operand:HI 1 "s_register_operand"        "r")
 	      (match_operand:DI 2 "s_register_operand"        "=&l")])]
-  "TARGET_EITHER"
-  "if (TARGET_ARM)
-     arm_reload_out_hi (operands);
-   else
-     thumb_reload_out_hi (operands);
+  "TARGET_32BIT"
+{
+  arm_reload_out_hi (operands);
   DONE;
-  "
-)
+})
 
 (define_expand "reload_inhi"
   [(parallel [(match_operand:HI 0 "s_register_operand" "=r")
 	      (match_operand:HI 1 "arm_reload_memory_operand" "o")
 	      (match_operand:DI 2 "s_register_operand" "=&r")])]
-  "TARGET_EITHER"
-  "
-  if (TARGET_ARM)
-    arm_reload_in_hi (operands);
-  else
-    thumb_reload_out_hi (operands);
+  "TARGET_32BIT"
+{
+  arm_reload_in_hi (operands);
   DONE;
-")
+})
 
 (define_expand "movqi"
   [(set (match_operand:QI 0 "general_operand" "")
Index: gcc/config/arm/arm-protos.h
===================================================================
--- gcc.orig/config/arm/arm-protos.h
+++ gcc/config/arm/arm-protos.h
@@ -180,8 +180,6 @@ extern const char *thumb_output_move_mem
 extern const char *thumb_call_via_reg (rtx);
 extern void thumb_expand_movmemqi (rtx *);
 extern rtx arm_return_addr (int, rtx);
-extern void thumb_reload_out_hi (rtx *);
-extern void thumb_reload_in_hi (rtx *);
 extern void thumb_set_return_address (rtx, rtx);
 extern const char *thumb1_output_casesi (rtx *);
 extern const char *thumb2_output_casesi (rtx *);

Re: Ping^2: A problem simplifying subregs of the hard frame pointer

[Bernd Schmidt]

On 08/04/2010 01:59 AM, Bernd Schmidt wrote:
> On 07/31/2010 01:45 PM, Richard Earnshaw wrote:
>> 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.
> 
> Essentially, whenever reload may have to move something from A to B,
> but can't do it without a scratch reg.

Sorry, I was somewhat inaccurate there.  Trying to explain it at 3am
probably won't lead to much better results than at 1am, but anyway...

Normally, you just need to tell reload you need a scratch.  The insn
pattern comes into play when you need to do something more interesting
than to move the value from A to scratch to B.  I think on ARM, that's
primarily for ((MODE) == HImode && ! arm_arch4) where we don't have the
necessary memory access insns IIRC.  Thumb should be OK as far as I can
tell.


Bernd