Second attempt at fixing reloading of even-only registers

Second attempt at fixing reloading of even-only registers

[Richard Sandiford]

This patch stops reload from turning SUBREGs of pseudos into invalid
hard REGs.  See:

   http://gcc.gnu.org/ml/gcc-patches/2008-07/msg01688.html

for a fuller analysis of how this happens.  The patch attached to that
message broke ia64, which was creating (subreg:M1 (reg:M2 R) ...) for
cases where (reg:M1 R) isn't valid.

So the missing HARD_REGNO_MODE_OK check should only be done when
replacing a SUBREG with a REG, as reload does.  The current canonical
check for this is in simplify_subreg.  However, reload doesn't actually
want a simplified rtx at this stage, so calling simplify_subreg itself
is a bit heavyweight.  I've therefore split the checks out into a
separate function, simplify_subreg_regno.

I wasn't sure where to put the new function.  On the one hand it has
"simplify" in the name, so simplify-rtx.c would seem a natural home.
On the other hand, the new function has a similar interface and function
to rtlanal.c routines like subreg_regno_offset.

In the end I plumped for rtlanal.c.  simplify_subreg uses both
subreg_offset_representable_p and subreg_regno_offset, so by putting
the new function in rtlanal.c, we can avoid recalculating the
subreg info.

As a "clean up", I wondered about using simplify_subreg_regno
in subreg_regno, and asserting that the result is >= 0.  Unfortunately,
the assert soon triggered on x86_64.  I didn't look why.  The problem
with subregs is that if you try to do more than you have to, you just
keep finding more things, so I just took the failure as proof that
I should keep things simple.

Tested on mips64-linux-gnu.  Bootstrapped & regression-tested on
x86_64-linux-gnu.  OK to install?

Richard


Index: gcc/rtl.h
===================================================================
--- gcc/rtl.h	2008-08-21 22:35:30.000000000 +0100
+++ gcc/rtl.h	2008-08-23 16:41:56.000000000 +0100
@@ -1084,6 +1084,8 @@ extern unsigned int subreg_regno_offset	
 extern bool subreg_offset_representable_p (unsigned int, enum machine_mode,
 					   unsigned int, enum machine_mode);
 extern unsigned int subreg_regno (const_rtx);
+extern int simplify_subreg_regno (unsigned int, enum machine_mode,
+				  unsigned int, enum machine_mode);
 extern unsigned int subreg_nregs (const_rtx);
 extern unsigned int subreg_nregs_with_regno (unsigned int, const_rtx);
 extern unsigned HOST_WIDE_INT nonzero_bits (const_rtx, enum machine_mode);
Index: gcc/rtlanal.c
===================================================================
--- gcc/rtlanal.c	2008-08-21 22:35:30.000000000 +0100
+++ gcc/rtlanal.c	2008-08-23 17:00:34.000000000 +0100
@@ -3244,6 +3244,64 @@ subreg_offset_representable_p (unsigned 
   return info.representable_p;
 }
 
+/* Return the number of a YMODE register to which
+
+       (subreg:YMODE (reg:XMODE XREGNO) OFFSET)
+
+   can be simplified.  Return -1 if the subreg can't be simplified.
+
+   XREGNO is a hard register number.  */
+
+int
+simplify_subreg_regno (unsigned int xregno, enum machine_mode xmode,
+		       unsigned int offset, enum machine_mode ymode)
+{
+  struct subreg_info info;
+  unsigned int yregno;
+
+#ifdef CANNOT_CHANGE_MODE_CLASS
+  /* Give the backend a chance to disallow the mode change.  */
+  if (GET_MODE_CLASS (xmode) != MODE_COMPLEX_INT
+      && GET_MODE_CLASS (xmode) != MODE_COMPLEX_FLOAT
+      && REG_CANNOT_CHANGE_MODE_P (xregno, xmode, ymode))
+    return -1;
+#endif
+
+  /* We shouldn't simplify stack-related registers.  */
+  if ((!reload_completed || frame_pointer_needed)
+      && (xregno == FRAME_POINTER_REGNUM
+	  || xregno == HARD_FRAME_POINTER_REGNUM))
+    return -1;
+
+  if (FRAME_POINTER_REGNUM != ARG_POINTER_REGNUM
+      && xregno == ARG_POINTER_REGNUM)
+    return -1;
+
+  if (xregno == STACK_POINTER_REGNUM)
+    return -1;
+
+  /* Try to get the register offset.  */
+  subreg_get_info (xregno, xmode, offset, ymode, &info);
+  if (!info.representable_p)
+    return -1;
+
+  /* Make sure that the offsetted register value is in range.  */
+  yregno = xregno + info.offset;
+  if (!HARD_REGISTER_NUM_P (yregno))
+    return -1;
+
+  /* See whether (reg:YMODE YREGNO) is valid.
+
+     ??? We allow invalid registers if (reg:XMODE XREGNO) is also invalid.
+     This is a kludge to work around how float/complex arguments are passed
+     on 32-bit SPARC and should be fixed.  */
+  if (!HARD_REGNO_MODE_OK (yregno, ymode)
+      && HARD_REGNO_MODE_OK (xregno, xmode))
+    return -1;
+
+  return (int) yregno;
+}
+
 /* Return the final regno that a subreg expression refers to.  */
 unsigned int
 subreg_regno (const_rtx x)
Index: gcc/simplify-rtx.c
===================================================================
--- gcc/simplify-rtx.c	2008-08-21 22:35:30.000000000 +0100
+++ gcc/simplify-rtx.c	2008-08-23 16:41:56.000000000 +0100
@@ -5069,35 +5069,13 @@ simplify_subreg (enum machine_mode outer
      suppress this simplification.  If the hard register is the stack,
      frame, or argument pointer, leave this as a SUBREG.  */
 
-  if (REG_P (op)
-      && REGNO (op) < FIRST_PSEUDO_REGISTER
-#ifdef CANNOT_CHANGE_MODE_CLASS
-      && ! (REG_CANNOT_CHANGE_MODE_P (REGNO (op), innermode, outermode)
-	    && GET_MODE_CLASS (innermode) != MODE_COMPLEX_INT
-	    && GET_MODE_CLASS (innermode) != MODE_COMPLEX_FLOAT)
-#endif
-      && ((reload_completed && !frame_pointer_needed)
-	  || (REGNO (op) != FRAME_POINTER_REGNUM
-#if HARD_FRAME_POINTER_REGNUM != FRAME_POINTER_REGNUM
-	      && REGNO (op) != HARD_FRAME_POINTER_REGNUM
-#endif
-	     ))
-#if FRAME_POINTER_REGNUM != ARG_POINTER_REGNUM
-      && REGNO (op) != ARG_POINTER_REGNUM
-#endif
-      && REGNO (op) != STACK_POINTER_REGNUM
-      && subreg_offset_representable_p (REGNO (op), innermode,
-					byte, outermode))
+  if (REG_P (op) && HARD_REGISTER_P (op))
     {
-      unsigned int regno = REGNO (op);
-      unsigned int final_regno
-	= regno + subreg_regno_offset (regno, innermode, byte, outermode);
-
-      /* ??? We do allow it if the current REG is not valid for
-	 its mode.  This is a kludge to work around how float/complex
-	 arguments are passed on 32-bit SPARC and should be fixed.  */
-      if (HARD_REGNO_MODE_OK (final_regno, outermode)
-	  || ! HARD_REGNO_MODE_OK (regno, innermode))
+      unsigned int regno, final_regno;
+
+      regno = REGNO (op);
+      final_regno = simplify_subreg_regno (regno, innermode, byte, outermode);
+      if (HARD_REGISTER_NUM_P (final_regno))
 	{
 	  rtx x;
 	  int final_offset = byte;
Index: gcc/reload.c
===================================================================
--- gcc/reload.c	2008-08-21 22:35:29.000000000 +0100
+++ gcc/reload.c	2008-08-23 16:41:56.000000000 +0100
@@ -2995,12 +2995,11 @@ find_reloads (rtx insn, int replace, int
 	      if (REG_P (SUBREG_REG (operand))
 		  && REGNO (SUBREG_REG (operand)) < FIRST_PSEUDO_REGISTER)
 		{
-		  if (!subreg_offset_representable_p
-			(REGNO (SUBREG_REG (operand)),
-			 GET_MODE (SUBREG_REG (operand)),
-			 SUBREG_BYTE (operand),
-			 GET_MODE (operand)))
-		     force_reload = 1;
+		  if (simplify_subreg_regno (REGNO (SUBREG_REG (operand)),
+					     GET_MODE (SUBREG_REG (operand)),
+					     SUBREG_BYTE (operand),
+					     GET_MODE (operand)) < 0)
+		    force_reload = 1;
 		  offset += subreg_regno_offset (REGNO (SUBREG_REG (operand)),
 						 GET_MODE (SUBREG_REG (operand)),
 						 SUBREG_BYTE (operand),

Re: Second attempt at fixing reloading of even-only registers

[Mark Mitchell]

Richard Sandiford wrote:
> This patch stops reload from turning SUBREGs of pseudos into invalid
> hard REGs.  See:
> 
>    http://gcc.gnu.org/ml/gcc-patches/2008-07/msg01688.html

> I wasn't sure where to put the new function.  On the one hand it has
> "simplify" in the name, so simplify-rtx.c would seem a natural home.
> On the other hand, the new function has a similar interface and function
> to rtlanal.c routines like subreg_regno_offset.

I'd probably have used "get_" instead of "simplify", which makes your
choice of rtlanal.c seem more logical. :-)  You're not so much
simplifying something as just working out what register you mean.  But,
that's just semantics; either name is fine.

> Tested on mips64-linux-gnu.  Bootstrapped & regression-tested on
> x86_64-linux-gnu.  OK to install?

Missing ChangeLog, but OK otherwise.  Please add and check in.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Second attempt at fixing reloading of even-only registers

[Richard Sandiford]

Thanks for the review ;)

Mark Mitchell <mark@codesourcery.com> writes:
> Richard Sandiford wrote:
>> This patch stops reload from turning SUBREGs of pseudos into invalid
>> hard REGs.  See:
>> 
>>    http://gcc.gnu.org/ml/gcc-patches/2008-07/msg01688.html
>
>> I wasn't sure where to put the new function.  On the one hand it has
>> "simplify" in the name, so simplify-rtx.c would seem a natural home.
>> On the other hand, the new function has a similar interface and function
>> to rtlanal.c routines like subreg_regno_offset.
>
> I'd probably have used "get_" instead of "simplify", which makes your
> choice of rtlanal.c seem more logical. :-)  You're not so much
> simplifying something as just working out what register you mean.  But,
> that's just semantics; either name is fine.

Hmm.  The problem here is that we also have plain "subreg_regno",
so having "get_subreg_regno" might be a bit confusing.

The first cut actually had "subreg_maybe_regno", but I renamed
it when I decided to steal the simplify_subreg logic.  Would that
be better?

>> Tested on mips64-linux-gnu.  Bootstrapped & regression-tested on
>> x86_64-linux-gnu.  OK to install?
>
> Missing ChangeLog, but OK otherwise.  Please add and check in.

Doh, sorry.  I'd even written one before sending.

gcc/
	* rtl.h (simplify_subreg_regno): Declare.
	* rtlanal.c (simplify_subreg_regno): New function, split out from...
	* simplify-rtx.c (simplify_subreg): ...here.
	* reload.c (find_reloads): Use simplify_subreg_regno instead of
	subreg_offset_representable_p.

Richard

Re: Second attempt at fixing reloading of even-only registers

[Mark Mitchell]

Richard Sandiford wrote:

>>>    http://gcc.gnu.org/ml/gcc-patches/2008-07/msg01688.html
>>> I wasn't sure where to put the new function.  On the one hand it has
>>> "simplify" in the name, so simplify-rtx.c would seem a natural home.
>>> On the other hand, the new function has a similar interface and function
>>> to rtlanal.c routines like subreg_regno_offset.
>> I'd probably have used "get_" instead of "simplify", which makes your
>> choice of rtlanal.c seem more logical. :-)  You're not so much
>> simplifying something as just working out what register you mean.  But,
>> that's just semantics; either name is fine.
> 
> Hmm.  The problem here is that we also have plain "subreg_regno",
> so having "get_subreg_regno" might be a bit confusing.
> 
> The first cut actually had "subreg_maybe_regno", but I renamed
> it when I decided to steal the simplify_subreg logic.  Would that
> be better?

I don't think so; let's just go with what you've already got.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713