diff mbox

[13/28] mn10300: cleanup secondary reloads

Message ID 1294691517-19580-14-git-send-email-rth@redhat.com
State New
Headers show

Commit Message

Richard Henderson Jan. 10, 2011, 8:31 p.m. UTC 
From: Richard Henderson <rth@twiddle.net>

Handles output reloads for QI/HImode properly; previously we were
only handing input reloads properly.

Handles reloads involving the stack pointer better; note that the
AM33 allows copying SP to DATA_REGS as well as ADDRESS and EXTENDED.
---
 gcc/config/mn10300/mn10300-protos.h |    1 -
 gcc/config/mn10300/mn10300.c        |  120 +++++++++++++++++++---------------
 gcc/config/mn10300/mn10300.h        |    3 -
 gcc/config/mn10300/mn10300.md       |   60 ++++++++++-------
 4 files changed, 102 insertions(+), 82 deletions(-)

Comments

Jeff Law Jan. 18, 2011, 5:08 p.m. UTC | #1 
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/10/11 13:31, Richard Henderson wrote:
> From: Richard Henderson <rth@twiddle.net>
> 
> Handles output reloads for QI/HImode properly; previously we were
> only handing input reloads properly.
> 
> Handles reloads involving the stack pointer better; note that the
> AM33 allows copying SP to DATA_REGS as well as ADDRESS and EXTENDED.
I'm not sure I understand the change to the preferred_reload_class.
ADDRESS_OR_EXTENDED_REGS seems to be more appropriate than
BASE_REG_CLASS.  Maybe the tighter preferred class is giving you better
code?

It looks OK though.

jeff

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJNNckSAAoJEBRtltQi2kC7tjIIALMkhJE+jSlm8wlYlEdL7Q93
TkGpWbaXYHEXBh9dNblUMlaGY/Gz2f+eVu7mnzgrvmIBHbB8mBhznT2uMB92tLZw
DtsoCGsUfTJJx+o0Yz3PQIoMwEz6gGLsKAQgZneAPbS4QQrJq4wSvkP5No6bV1/k
qAylRwkst0Yy2/MjZC2NKW3KFZcmHD28vOJuQZM7+DZuU7EHZ3zJ94ndGYLlYTqy
Cb0fCuufWvxCHuCCJAjMUQBGXPiExyJ5MVzEP+cq+pQ5tCVJGv137kgNB8kwRvfG
AY71UE93iUI8n9X108fV8mU3YHLnxmyDvBR0Mt47U21cifJIb1XvHvxEOa5S4QU=
=ohpC
-----END PGP SIGNATURE-----
Richard Henderson Jan. 18, 2011, 6:18 p.m. UTC | #2 
On 01/18/2011 09:08 AM, Jeff Law wrote:
> I'm not sure I understand the change to the preferred_reload_class.
> ADDRESS_OR_EXTENDED_REGS seems to be more appropriate than
> BASE_REG_CLASS.  Maybe the tighter preferred class is giving you better
> code?

I don't understand ADDRESS_OR_EXTENDED_REGS.  Once you leave the
slightly smaller encoding of ADDRESS_REGS, the encoding accepts
DATA_REGS just as easily as EXTENDED_REGS.

So I see no reason to exclude DATA_REGS from the mix.

I used BASE_REG_CLASS as that is already defined such that we get
GENERAL_REGS in AM33 mode and ADDRESS_REGS in MN103 mode.


r~
Jeff Law Jan. 18, 2011, 6:30 p.m. UTC | #3 
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/18/11 11:18, Richard Henderson wrote:
> On 01/18/2011 09:08 AM, Jeff Law wrote:
>> I'm not sure I understand the change to the preferred_reload_class.
>> ADDRESS_OR_EXTENDED_REGS seems to be more appropriate than
>> BASE_REG_CLASS.  Maybe the tighter preferred class is giving you better
>> code?
> 
> I don't understand ADDRESS_OR_EXTENDED_REGS.  Once you leave the
> slightly smaller encoding of ADDRESS_REGS, the encoding accepts
> DATA_REGS just as easily as EXTENDED_REGS.
I agree -- there's definitely something odd here.  It may have been a
mis-guided attempt at providing the proper set of union classes that the
old allocator/reload desired.  My memory is just too foggy on a lot of
this stuff.  I'm certainly willing to go with your implementation as I
can't put my finger on the purpose behind ADDRESS_OR_EXTENDED_REGS.


>
> So I see no reason to exclude DATA_REGS from the mix.
> 
> I used BASE_REG_CLASS as that is already defined such that we get
> GENERAL_REGS in AM33 mode and ADDRESS_REGS in MN103 mode.
Doesn't BASE_REG_CLASS include SP_REGS, which are highly restricted in
how they can be used?

jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJNNdxTAAoJEBRtltQi2kC7VSAH+wXW56apbxyDjCrCoC8HKkQj
9IpueDNd85BiieVhdlT1vhDKlOoL46vusR1ef1BgPRou2Qo1GDfrvWxkUP/dng7u
N1kH/F3tFh1fL3d0ieMrih871Yoh9XCsJLXRQlm2l4ctXBGPcEpyAgWWSUiGi/5s
waBC545NF+td+Ad0hKgG64LLRq/nqG+XRncmji3uWLdjIiT73OyVYSbfwOyThCr8
jfZP3sLk75xLtytwQJ9jZLOyo70P2p7Y8A8uvWAgftcpD/RbD5OqPQIM4t4PWGQI
jCpi2s0LLqmGk6xfUIR8O9YHq3BY+IlDaSbuta6gRVtxiW6FOgKtV43h4z2yPwE=
=cK4O
-----END PGP SIGNATURE-----
Richard Henderson Jan. 18, 2011, 6:35 p.m. UTC | #4 
On 01/18/2011 10:30 AM, Jeff Law wrote:
>> I used BASE_REG_CLASS as that is already defined such that we get
>> GENERAL_REGS in AM33 mode and ADDRESS_REGS in MN103 mode.
> Doesn't BASE_REG_CLASS include SP_REGS, which are highly restricted in
> how they can be used?

Hmm, true.

In this specific place we're looking for a place to move SP_REG.
I suppose a "move" of SP to SP would be ok, so long as it turns
out to be valid for everything else?  Or should I go ahead and
be safe and choose the class that excludes SP?


r~
Jeff Law Jan. 19, 2011, 3:31 p.m. UTC | #5 
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/18/11 11:35, Richard Henderson wrote:
> On 01/18/2011 10:30 AM, Jeff Law wrote:
>>> I used BASE_REG_CLASS as that is already defined such that we get
>>> GENERAL_REGS in AM33 mode and ADDRESS_REGS in MN103 mode.
>> Doesn't BASE_REG_CLASS include SP_REGS, which are highly restricted in
>> how they can be used?
> 
> Hmm, true.
> 
> In this specific place we're looking for a place to move SP_REG.
> I suppose a "move" of SP to SP would be ok, so long as it turns
> out to be valid for everything else?  Or should I go ahead and
> be safe and choose the class that excludes SP?
I think the safe thing to do is use a class which excludes SP; it may
not matter in the end, but we already know that works.  We could mark it
as a future TODO to just use BASE_REG_CLASS.

Jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJNNwPQAAoJEBRtltQi2kC7CuIH/R9U3WzHpiZT7FX1qvtV0HJN
a5DlbS1T5lGo6P/iZHPrr25Qf40p3aUVOkL0Mf7K96jrYBPpzWJr/ELEHja2Hpn8
aQxq7XzSCwY39ZKcin1U5/0gLzDCXnm+rkNQqx2M5+IZOMUn6RHg4tER52+61C21
i9xKnIxkXTwmHMuAX8cAveILUubxpq4Fx8Qe+aBb0Kj57Uvmphypj2o9HcoEiVDJ
AkyrtJZyZ74eW5jBTSJVkT/WR+2q5F26sZfoAKqW2IP4hLCyRNGwO7Zf+8dcetL8
Fw6BNe7GujMh1J98dSZHjFtFKMk8fx18gxGiPygdnQkpWIJSVn3s3f2QUDBxnxc=
=UUub
-----END PGP SIGNATURE-----
Richard Henderson Jan. 19, 2011, 4:44 p.m. UTC | #6 
On 01/19/2011 07:31 AM, Jeff Law wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 01/18/11 11:35, Richard Henderson wrote:
>> On 01/18/2011 10:30 AM, Jeff Law wrote:
>>>> I used BASE_REG_CLASS as that is already defined such that we get
>>>> GENERAL_REGS in AM33 mode and ADDRESS_REGS in MN103 mode.
>>> Doesn't BASE_REG_CLASS include SP_REGS, which are highly restricted in
>>> how they can be used?
>>
>> Hmm, true.
>>
>> In this specific place we're looking for a place to move SP_REG.
>> I suppose a "move" of SP to SP would be ok, so long as it turns
>> out to be valid for everything else?  Or should I go ahead and
>> be safe and choose the class that excludes SP?
> I think the safe thing to do is use a class which excludes SP; it may
> not matter in the end, but we already know that works.  We could mark it
> as a future TODO to just use BASE_REG_CLASS.

Right.  I've changed these lines to 

  return (TARGET_AM33 ? GENERAL_REGS : ADDRESS_REGS)

in my tree.


r~
diff mbox

Patch

diff --git a/gcc/config/mn10300/mn10300-protos.h b/gcc/config/mn10300/mn10300-protos.h
index d6cf850..37968ff 100644
--- a/gcc/config/mn10300/mn10300-protos.h
+++ b/gcc/config/mn10300/mn10300-protos.h
@@ -35,7 +35,6 @@  extern Cstar mn10300_output_cmp (rtx, rtx);
 extern void  mn10300_print_operand (FILE *, rtx, int);
 extern void  mn10300_print_operand_address (FILE *, rtx);
 extern void  mn10300_print_reg_list (FILE *, int);
-extern Rclas mn10300_secondary_reload_class (Rclas, Mmode, rtx);
 extern Mmode mn10300_select_cc_mode (rtx);
 extern int   mn10300_store_multiple_operation (rtx, Mmode);
 extern int   mn10300_symbolic_operand (rtx, Mmode);
diff --git a/gcc/config/mn10300/mn10300.c b/gcc/config/mn10300/mn10300.c
index 5f2d63b..78b5b5d 100644
--- a/gcc/config/mn10300/mn10300.c
+++ b/gcc/config/mn10300/mn10300.c
@@ -1321,7 +1321,7 @@  static reg_class_t
 mn10300_preferred_reload_class (rtx x, reg_class_t rclass)
 {
   if (x == stack_pointer_rtx && rclass != SP_REGS)
-     return ADDRESS_OR_EXTENDED_REGS;
+    return BASE_REG_CLASS;
   else if (MEM_P (x)
 	   || (REG_P (x) 
 	       && !HARD_REGISTER_P (x))
@@ -1339,72 +1339,83 @@  static reg_class_t
 mn10300_preferred_output_reload_class (rtx x, reg_class_t rclass)
 {
   if (x == stack_pointer_rtx && rclass != SP_REGS)
-    return ADDRESS_OR_EXTENDED_REGS;
-
+    return BASE_REG_CLASS;
   return rclass;
 }
 
-/* What (if any) secondary registers are needed to move IN with mode
-   MODE into a register in register class RCLASS.
-
-   We might be able to simplify this.  */
+/* Implement TARGET_SECONDARY_RELOAD.  */
 
-enum reg_class
-mn10300_secondary_reload_class (enum reg_class rclass, enum machine_mode mode,
-				rtx in)
+static reg_class_t
+mn10300_secondary_reload (bool in_p, rtx x, reg_class_t rclass_i,
+			  enum machine_mode mode, secondary_reload_info *sri)
 {
-  rtx inner = in;
-
-  /* Strip off any SUBREG expressions from IN.  Basically we want
-     to know if IN is a pseudo or (subreg (pseudo)) as those can
-     turn into MEMs during reload.  */
-  while (GET_CODE (inner) == SUBREG)
-    inner = SUBREG_REG (inner);
-
-  /* Memory loads less than a full word wide can't have an
-     address or stack pointer destination.  They must use
-     a data register as an intermediate register.  */
-  if ((MEM_P (in)
-       || (REG_P (inner)
-	   && REGNO (inner) >= FIRST_PSEUDO_REGISTER))
-      && (mode == QImode || mode == HImode)
-      && (rclass == ADDRESS_REGS || rclass == SP_REGS
-	  || rclass == SP_OR_ADDRESS_REGS))
+  enum reg_class rclass = (enum reg_class) rclass_i;
+  enum reg_class xclass = NO_REGS;
+  unsigned int xregno = INVALID_REGNUM;
+
+  if (REG_P (x))
     {
-      if (TARGET_AM33)
-	return DATA_OR_EXTENDED_REGS;
-      return DATA_REGS;
+      xregno = REGNO (x);
+      if (xregno >= FIRST_PSEUDO_REGISTER)
+	xregno = true_regnum (x);
+      if (xregno != INVALID_REGNUM)
+	xclass = REGNO_REG_CLASS (xregno);
+    }
+
+  if (!TARGET_AM33)
+    {
+      /* Memory load/stores less than a full word wide can't have an
+         address or stack pointer destination.  They must use a data
+         register as an intermediate register.  */
+      if (rclass != DATA_REGS
+	  && (mode == QImode || mode == HImode)
+	  && xclass == NO_REGS)
+	return DATA_REGS;
+
+      /* We can only move SP to/from an address register.  */
+      if (in_p
+	  && rclass == SP_REGS
+	  && xclass != ADDRESS_REGS)
+	return ADDRESS_REGS;
+      if (!in_p
+	  && xclass == SP_REGS
+	  && rclass != ADDRESS_REGS
+	  && rclass != SP_OR_ADDRESS_REGS)
+	return ADDRESS_REGS;
     }
 
-  /* We can't directly load sp + const_int into a data register;
-     we must use an address register as an intermediate.  */
-  if (rclass != SP_REGS
-      && rclass != ADDRESS_REGS
+  /* We can't directly load sp + const_int into a register;
+     we must use an address register as an scratch.  */
+  if (in_p
+      && rclass != SP_REGS
       && rclass != SP_OR_ADDRESS_REGS
       && rclass != SP_OR_EXTENDED_REGS
-      && rclass != ADDRESS_OR_EXTENDED_REGS
       && rclass != SP_OR_ADDRESS_OR_EXTENDED_REGS
-      && (in == stack_pointer_rtx
-	  || (GET_CODE (in) == PLUS
-	      && (XEXP (in, 0) == stack_pointer_rtx
-		  || XEXP (in, 1) == stack_pointer_rtx))))
-    return ADDRESS_REGS;
+      && GET_CODE (x) == PLUS
+      && (XEXP (x, 0) == stack_pointer_rtx
+	  || XEXP (x, 1) == stack_pointer_rtx))
+    {
+      sri->icode = CODE_FOR_reload_plus_sp_const;
+      return NO_REGS;
+    }
 
+  /* We can't load/store an FP register from a constant address.  */
   if (TARGET_AM33_2
-      && rclass == FP_REGS)
+      && (rclass == FP_REGS || xclass == FP_REGS)
+      && (xclass == NO_REGS || rclass == NO_REGS))
     {
-      /* We can't load directly into an FP register from a	
-	 constant address.  */
-      if (MEM_P (in)
-	  && CONSTANT_ADDRESS_P (XEXP (in, 0)))
-	return DATA_OR_EXTENDED_REGS;
+      rtx addr = NULL;
+
+      if (xregno >= FIRST_PSEUDO_REGISTER && xregno != INVALID_REGNUM)
+	{
+	  addr = reg_equiv_mem [xregno];
+	  if (addr)
+	    addr = XEXP (addr, 0);
+	}
+      else if (MEM_P (x))
+	addr = XEXP (x, 0);
 
-      /* Handle case were a pseudo may not get a hard register
-	 but has an equivalent memory location defined.  */
-      if (REG_P (inner)
-	  && REGNO (inner) >= FIRST_PSEUDO_REGISTER
-	  && reg_equiv_mem [REGNO (inner)]
-	  && CONSTANT_ADDRESS_P (XEXP (reg_equiv_mem [REGNO (inner)], 0)))
+      if (addr && CONSTANT_ADDRESS_P (addr))
 	return DATA_OR_EXTENDED_REGS;
     }
 
@@ -2720,7 +2731,10 @@  mn10300_conditional_register_usage (void)
 #undef  TARGET_PREFERRED_RELOAD_CLASS
 #define TARGET_PREFERRED_RELOAD_CLASS mn10300_preferred_reload_class
 #undef  TARGET_PREFERRED_OUTPUT_RELOAD_CLASS
-#define TARGET_PREFERRED_OUTPUT_RELOAD_CLASS mn10300_preferred_output_reload_class
+#define TARGET_PREFERRED_OUTPUT_RELOAD_CLASS \
+  mn10300_preferred_output_reload_class
+#undef  TARGET_SECONDARY_RELOAD
+#define TARGET_SECONDARY_RELOAD  mn10300_secondary_reload
 
 #undef  TARGET_TRAMPOLINE_INIT
 #define TARGET_TRAMPOLINE_INIT mn10300_trampoline_init
diff --git a/gcc/config/mn10300/mn10300.h b/gcc/config/mn10300/mn10300.h
index f5f6416..4fe0c48 100644
--- a/gcc/config/mn10300/mn10300.h
+++ b/gcc/config/mn10300/mn10300.h
@@ -416,9 +416,6 @@  enum reg_class
 #define LIMIT_RELOAD_CLASS(MODE, CLASS) \
   (!TARGET_AM33 && (MODE == QImode || MODE == HImode) ? DATA_REGS : CLASS)
 
-#define SECONDARY_RELOAD_CLASS(CLASS,MODE,IN) \
-  mn10300_secondary_reload_class(CLASS,MODE,IN)
-
 /* Return the maximum number of consecutive registers
    needed to represent mode MODE in a register of class CLASS.  */
 
diff --git a/gcc/config/mn10300/mn10300.md b/gcc/config/mn10300/mn10300.md
index 6e0e1a2..457b17a 100644
--- a/gcc/config/mn10300/mn10300.md
+++ b/gcc/config/mn10300/mn10300.md
@@ -274,42 +274,52 @@ 
 ;; We use this to handle addition of two values when one operand is the
 ;; stack pointer and the other is a memory reference of some kind.  Reload
 ;; does not handle them correctly without this expander.
-(define_expand "reload_insi"
-  [(set (match_operand:SI     0 "register_operand" "=a")
+(define_expand "reload_plus_sp_const"
+  [(set (match_operand:SI     0 "register_operand" "=r")
 	(match_operand:SI     1 "impossible_plus_operand" ""))
-   (clobber (match_operand:SI 2 "register_operand" "=&r"))]
+   (clobber (match_operand:SI 2 "register_operand" "=&A"))]
   ""
   "
 {
-  gcc_assert (REGNO (operands[0]) != REGNO (operands[2]));
+  rtx dest, scratch, other;
 
-  if (XEXP (operands[1], 0) == stack_pointer_rtx)
+  dest = operands[0];
+  scratch = operands[2];
+
+  other = XEXP (operands[1], 1);
+  if (other == stack_pointer_rtx)
+    other = XEXP (operands[1], 0);
+
+  if (true_regnum (other) == true_regnum (dest))
     {
-      if (GET_CODE (XEXP (operands[1], 1)) == SUBREG
-	  && (GET_MODE_SIZE (GET_MODE (XEXP (operands[1], 1)))
-	      > GET_MODE_SIZE (GET_MODE (SUBREG_REG (XEXP (operands[1], 1))))))
-	emit_move_insn (operands[2],
-			gen_rtx_ZERO_EXTEND
-			(GET_MODE (XEXP (operands[1], 1)),
-			 SUBREG_REG (XEXP (operands[1], 1))));
-      else
-	emit_move_insn (operands[2], XEXP (operands[1], 1));
-      emit_move_insn (operands[0], XEXP (operands[1], 0));
+      gcc_assert (true_regnum (scratch) != true_regnum (dest));
+      emit_move_insn (scratch, stack_pointer_rtx);
+      emit_insn (gen_addsi3 (dest, dest, scratch));
+    }
+  else if (TARGET_AM33 || REGNO_REG_CLASS (true_regnum (dest)) == ADDRESS_REGS)
+    {
+      emit_move_insn (dest, stack_pointer_rtx);
+      if (other == stack_pointer_rtx)
+        emit_insn (gen_addsi3 (dest, dest, dest));
+      else if (other != const0_rtx)
+        emit_insn (gen_addsi3 (dest, dest, other));
     }
   else
     {
-      if (GET_CODE (XEXP (operands[1], 0)) == SUBREG
-	  && (GET_MODE_SIZE (GET_MODE (XEXP (operands[1], 0)))
-	      > GET_MODE_SIZE (GET_MODE (SUBREG_REG (XEXP (operands[1], 0))))))
-	emit_move_insn (operands[2],
-			gen_rtx_ZERO_EXTEND
-			(GET_MODE (XEXP (operands[1], 0)),
-			 SUBREG_REG (XEXP (operands[1], 0))));
+      emit_move_insn (scratch, stack_pointer_rtx);
+      if (other == stack_pointer_rtx)
+	{
+	  emit_move_insn (dest, scratch);
+          emit_insn (gen_addsi3 (dest, dest, dest));
+	}
+      else if (other != const0_rtx)
+	{
+	  emit_move_insn (dest, other);
+          emit_insn (gen_addsi3 (dest, dest, scratch));
+	}
       else
-	emit_move_insn (operands[2], XEXP (operands[1], 0));
-      emit_move_insn (operands[0], XEXP (operands[1], 1));
+	emit_move_insn (dest, scratch);
     }
-  emit_insn (gen_addsi3 (operands[0], operands[0], operands[2]));
   DONE;
 }")