diff mbox

[ARM] Fix PR48808, PR48792: More work on CANNOT_CHANGE_MODE_CLASS

Message ID 87mxi5jlgv.fsf@firetop.home
State New
Headers show

Commit Message

Richard Sandiford May 29, 2011, 8:33 p.m. UTC 
Chung-Lin Tang <cltang@codesourcery.com> writes:
> An alternative fix direction would be to add similar mode/regclass
> validity checks to all places where CANNOT_CHANGE_MODE_CLASS is used in
> the RTL core, which may stratify things a bit more (the internals
> documentation does not mention anything on whether the backend should
> check this mode/regclass condition by itself). But this seems a bit
> scattered and probably a little risky, so I'll settle for the ARM
> specific fix for now.

I don't think the backend should need to check it, although there are
probably backends have done so as the path of least resistance.

It's true that CANNOT_CHANGE_CLASS_MODE is used in places where
unsimplifiable subregs of hard regs are otherwise allowed.
This is because some ports generate such subregs deliberately.
(That is, they say that register R cannot have mode N,
then generate (subreg:N (reg:M R)) in cases where they'd like
to refer to R in mode N anyway.)  No-one has been brave enough
to clean that up.

But what's happening here seems like a plain bug to me.
Or rather, two bugs:

- push_reload turns (subreg:M (foo:N)) reloads into (foo:N)
  reloads without first checking whether the register class
  has any registers of mode N.  (Note that IRA _does_ perform
  the corresponding check.  That is, IRA checks contains_reg_of_mode
  whenever it checks invalid_mode_change_p.  This seems like a
  good thing.)

- If we have a reload of a paradoxical subreg, reload can turn the other
  reload operand into an unsimplifiable (subreg:M (reg:N R)).  We might
  have to allow ports to generate that kind of subreg themselves
  (for now!), but reload shouldn't be generating new ones like this.

Something like the attached patch.  Not tested yet, and I'm sure
it'll break things in lots of fun and interesting ways...

Richard

Comments

Eric Botcazou May 29, 2011, 9:28 p.m. UTC | #1 
> I don't think the backend should need to check it, although there are
> probably backends have done so as the path of least resistance.

My opinion as well.  CANNOT_CHANGE_CLASS_MODE shoudn't have to duplicate the 
information provided by HARD_REGNO_MODE_OK.

> - If we have a reload of a paradoxical subreg, reload can turn the other
>   reload operand into an unsimplifiable (subreg:M (reg:N R)).  We might
>   have to allow ports to generate that kind of subreg themselves
>   (for now!), but reload shouldn't be generating new ones like this.

This is also what happens on the SPARC for PR target/48830.

> Something like the attached patch.  Not tested yet, and I'm sure
> it'll break things in lots of fun and interesting ways...

Mind posting a (temporarily) definitive version?  I'll give it a whirl on the 
SPARC and IA-64 to see how it would fare.
diff mbox

Patch

Index: gcc/reload.c
===================================================================
--- gcc/reload.c	(revision 174400)
+++ gcc/reload.c	(working copy)
@@ -1019,6 +1019,7 @@ 
 #ifdef CANNOT_CHANGE_MODE_CLASS
       && !CANNOT_CHANGE_MODE_CLASS (GET_MODE (SUBREG_REG (in)), inmode, rclass)
 #endif
+      && contains_reg_of_mode[(int) rclass][(int) GET_MODE (SUBREG_REG (in))]
       && (CONSTANT_P (SUBREG_REG (in))
 	  || GET_CODE (SUBREG_REG (in)) == PLUS
 	  || strict_low
@@ -1125,6 +1126,7 @@ 
 #ifdef CANNOT_CHANGE_MODE_CLASS
       && !CANNOT_CHANGE_MODE_CLASS (GET_MODE (SUBREG_REG (out)), outmode, rclass)
 #endif
+      && contains_reg_of_mode[(int) rclass][(int) GET_MODE (SUBREG_REG (out))]
       && (CONSTANT_P (SUBREG_REG (out))
 	  || strict_low
 	  || (((REG_P (SUBREG_REG (out))
Index: gcc/reload1.c
===================================================================
--- gcc/reload1.c	(revision 174400)
+++ gcc/reload1.c	(working copy)
@@ -4471,6 +4471,43 @@ 
 	}
     }
 }
+
+/* *FOCUS_PTR and *OTHER_PTR are two operands to a conceptual reload.
+   If *FOCUS_PTR is a paradoxical subreg, try to remove that subreg
+   and apply the corresponding narrowing subreg to *OTHER_PTR.
+   Return true if the operands were changed, false otherwise.  */
+
+static bool
+strip_paradoxical_subreg (rtx *focus_ptr, rtx *other_ptr)
+{
+  rtx focus, inner, other, tem;
+
+  focus = *focus_ptr;
+  if (GET_MODE (focus) != SUBREG)
+    return false;
+
+  inner = SUBREG_REG (focus);
+  if (GET_MODE_SIZE (GET_MODE (focus)) <= GET_MODE_SIZE (GET_MODE (inner)))
+    return false;
+
+  other = *other_ptr;
+  tem = gen_lowpart_common (GET_MODE (inner), other);
+  if (!tem)
+    return false;
+
+  /* If the lowpart operation turned a hard register into a subreg,
+     rather than simplifying it to another hard register, then the
+     mode change cannot be properly represented.  For example, OTHER
+     might be valid in its current mode, but not in the new one.  */
+  if (GET_CODE (tem) == SUBREG
+      && REG_P (other)
+      && HARD_REGISTER_P (other))
+    return false;
+
+  *focus_ptr = inner;
+  *other_ptr = tem;
+  return true;
+}
 
 /* A subroutine of reload_as_needed.  If INSN has a REG_EH_REGION note,
    examine all of the reload insns between PREV and NEXT exclusive, and
@@ -5538,7 +5575,7 @@ 
      chain reloads or do need an intermediate hard registers.  */
   bool result = true;
   int regno, n, code;
-  rtx out, in, tem, insn;
+  rtx out, in, insn;
   rtx last = get_last_insn ();
 
   /* Make r2 a component of r1.  */
@@ -5557,11 +5594,7 @@ 
 
   /* If IN is a paradoxical SUBREG, remove it and try to put the
      opposite SUBREG on OUT.  Likewise for a paradoxical SUBREG on OUT.  */
-  if (GET_CODE (in) == SUBREG
-      && (GET_MODE_SIZE (GET_MODE (in))
-	  > GET_MODE_SIZE (GET_MODE (SUBREG_REG (in))))
-      && (tem = gen_lowpart_common (GET_MODE (SUBREG_REG (in)), out)) != 0)
-    in = SUBREG_REG (in), out = tem;
+  strip_paradoxical_subreg (&in, &out);
 
   if (GET_CODE (in) == PLUS
       && (REG_P (XEXP (in, 0))
@@ -7557,7 +7590,6 @@ 
 	      if (tertiary_icode != CODE_FOR_nothing)
 		{
 		  rtx third_reloadreg = rld[tertiary_reload].reg_rtx;
-		  rtx tem;
 
 		  /* Copy primary reload reg to secondary reload reg.
 		     (Note that these have been swapped above, then
@@ -7566,13 +7598,7 @@ 
 		  /* If REAL_OLD is a paradoxical SUBREG, remove it
 		     and try to put the opposite SUBREG on
 		     RELOADREG.  */
-		  if (GET_CODE (real_old) == SUBREG
-		      && (GET_MODE_SIZE (GET_MODE (real_old))
-			  > GET_MODE_SIZE (GET_MODE (SUBREG_REG (real_old))))
-		      && 0 != (tem = gen_lowpart_common
-			       (GET_MODE (SUBREG_REG (real_old)),
-				reloadreg)))
-		    real_old = SUBREG_REG (real_old), reloadreg = tem;
+		  strip_paradoxical_subreg (&real_old, &reloadreg);
 
 		  gen_reload (reloadreg, second_reloadreg,
 			      rl->opnum, rl->when_needed);
@@ -8388,16 +8414,8 @@ 
 
   /* If IN is a paradoxical SUBREG, remove it and try to put the
      opposite SUBREG on OUT.  Likewise for a paradoxical SUBREG on OUT.  */
-  if (GET_CODE (in) == SUBREG
-      && (GET_MODE_SIZE (GET_MODE (in))
-	  > GET_MODE_SIZE (GET_MODE (SUBREG_REG (in))))
-      && (tem = gen_lowpart_common (GET_MODE (SUBREG_REG (in)), out)) != 0)
-    in = SUBREG_REG (in), out = tem;
-  else if (GET_CODE (out) == SUBREG
-	   && (GET_MODE_SIZE (GET_MODE (out))
-	       > GET_MODE_SIZE (GET_MODE (SUBREG_REG (out))))
-	   && (tem = gen_lowpart_common (GET_MODE (SUBREG_REG (out)), in)) != 0)
-    out = SUBREG_REG (out), in = tem;
+  if (!strip_paradoxical_subreg (&in, &out))
+    strip_paradoxical_subreg (&out, &in);
 
   /* How to do this reload can get quite tricky.  Normally, we are being
      asked to reload a simple operand, such as a MEM, a constant, or a pseudo