public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* RFA: Fix PR 32897 (invalid constants generated by find_reloads_toplev)
@ 2007-07-26 12:56 Richard Sandiford
  2007-08-06 10:25 ` Richard Sandiford
  2007-08-14 13:18 ` Richard Sandiford
  0 siblings, 2 replies; 4+ messages in thread
From: Richard Sandiford @ 2007-07-26 12:56 UTC (permalink / raw)
  To: gcc-patches

The following testcase:

---------------------------------------------------------------------------
volatile int g[32];
long long gll;
double gd;

#define MULTI(X) \
        X( 1), X( 2), X( 3), X( 4), X( 5), X( 6), X( 7), X( 8), X( 9), X(10), \
        X(11), X(12), X(13), X(14), X(15), X(16), X(17), X(18), X(19), X(20), \
        X(21), X(22), X(23), X(24), X(25), X(26), X(27), X(28), X(29), X(30)

#define DECLARE(INDEX) x##INDEX
#define COPY_IN(INDEX) x##INDEX = g[INDEX]
#define COPY_OUT(INDEX) g[INDEX] = x##INDEX

void
test (int n)
{
  union { long long l; double d; } u = { 0x12345678 };
  gll = u.l;
  int MULTI (DECLARE);
  MULTI (COPY_IN);
  MULTI (COPY_OUT);
  MULTI (COPY_OUT);
  MULTI (COPY_OUT);
  gd = u.d;
}
---------------------------------------------------------------------------

fails for mips*-elf targets when compiled with -G0 and with
optimisation enabled:

internal compiler error: in emit_move_insn, at expr.c:3317

The pseudo register that holds "u" is not allocated a hard
register, so we rematerialise the constant 0x1234567 on demand.
We check when setting reg_equiv_constant that 0x1234567 does indeed
satisfy LEGITIMATE_CONSTANT_P; we'd force it into memory otherwise.

However, find_reloads_toplev assumes that simplified subregs of
reg_equiv_constant entries also satisfy LEGITIMATE_CONSTANT_P.
This isn't true here: the original 0x12345678 CONST_INT is legitimate,
but the equivalent DFmode CONST_DOUBLE is not.

(I came across this while working on another patch.  The original
test was more natural; the one above was just reverse-engineered
after the fact.)

I think the fix is to force the subregged constant into the
constant pool if it isn't itself legitimate.  We must then
reload the new MEM's address.

There are actually two blocks of code that try to create the
subreg of a constant:

      if (subreg_lowpart_p (x)
	  && regno >= FIRST_PSEUDO_REGISTER
	  && reg_renumber[regno] < 0
	  && reg_equiv_constant[regno] != 0
	  && (tem = gen_lowpart_common (GET_MODE (x),
					reg_equiv_constant[regno])) != 0)
	return tem;

      if (regno >= FIRST_PSEUDO_REGISTER
	  && reg_renumber[regno] < 0
	  && reg_equiv_constant[regno] != 0)
	{
	  tem =
	    simplify_gen_subreg (GET_MODE (x), reg_equiv_constant[regno],
				 GET_MODE (SUBREG_REG (x)), SUBREG_BYTE (x));
	  gcc_assert (tem);
	  return tem;
	}

But this is redundant: gen_lowpart_common now uses simplify_gen_subreg
for constants anyway.  The first block isn't even a fast path; we spend
longer working out whether the special case applies, and what
simplify_gen_subreg's arguments should be if so, than we would if we
went straight to the second block.

So, rather than add special constant pool handling to both blocks of code,
I simply removed the first block and added the special handling to the second.

find_reloads assumes that, if find_reloads_toplev returns a MEM for
a SUBREG of a REG, the inner register must be equivalent to a memory
location.  That's no longer true, so I've made it also check that the
REG is not equivalent to a constant.  (I think that's better than checking
the reg_equiv_mem* arrays, because it documents why the extra condition
is needed.)

Bootstrapped & regression tested on x86_64-linux-gnu.  Also regression-
tested on mipsisa64-elf (flags {,-mips32}{-EB,-EL}{,-msoft-float}).
OK to install?

Richard


gcc/
	PR middle-end/32897
	* reload.c (find_reloads): Check that the memory returned by
	find_reloads_toplev was not the result of forcing a constant
	to memory.
	(find_reloads_toplev): Always use simplify_gen_subreg to get
	the subreg of a constant.  If the result is also a constant,
	but not a legitimate one, force it into the constant pool
	and reload its address.

gcc/testsuite/
	* gcc.dg/torture/pr32897.c: New test.

Index: gcc/reload.c
===================================================================
--- gcc/reload.c	(revision 126918)
+++ gcc/reload.c	(working copy)
@@ -2795,7 +2795,8 @@ find_reloads (rtx insn, int replace, int
 	      && MEM_P (op)
 	      && REG_P (reg)
 	      && (GET_MODE_SIZE (GET_MODE (reg))
-		  >= GET_MODE_SIZE (GET_MODE (op))))
+		  >= GET_MODE_SIZE (GET_MODE (op)))
+	      && reg_equiv_constant[REGNO (reg)] == 0)
 	    set_unique_reg_note (emit_insn_before (gen_rtx_USE (VOIDmode, reg),
 						   insn),
 				 REG_EQUAL, reg_equiv_memory_loc[REGNO (reg)]);
@@ -4601,14 +4602,6 @@ find_reloads_toplev (rtx x, int opnum, e
       int regno = REGNO (SUBREG_REG (x));
       rtx tem;
 
-      if (subreg_lowpart_p (x)
-	  && regno >= FIRST_PSEUDO_REGISTER
-	  && reg_renumber[regno] < 0
-	  && reg_equiv_constant[regno] != 0
-	  && (tem = gen_lowpart_common (GET_MODE (x),
-					reg_equiv_constant[regno])) != 0)
-	return tem;
-
       if (regno >= FIRST_PSEUDO_REGISTER
 	  && reg_renumber[regno] < 0
 	  && reg_equiv_constant[regno] != 0)
@@ -4617,6 +4610,15 @@ find_reloads_toplev (rtx x, int opnum, e
 	    simplify_gen_subreg (GET_MODE (x), reg_equiv_constant[regno],
 				 GET_MODE (SUBREG_REG (x)), SUBREG_BYTE (x));
 	  gcc_assert (tem);
+	  if (CONSTANT_P (tem) && !LEGITIMATE_CONSTANT_P (tem))
+	    {
+	      tem = force_const_mem (GET_MODE (x), tem);
+	      i = find_reloads_address (GET_MODE (tem), &tem, XEXP (tem, 0),
+					&XEXP (tem, 0), opnum, type,
+					ind_levels, insn);
+	      if (address_reloaded)
+		*address_reloaded = i;
+	    }
 	  return tem;
 	}
 
Index: gcc/testsuite/gcc.dg/torture/pr32897.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr32897.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr32897.c	(revision 0)
@@ -0,0 +1,27 @@
+/* { dg-options "-G0" { target mips*-*-* } } */
+
+volatile int g[32];
+long long gll;
+double gd;
+
+#define MULTI(X) \
+	X( 1), X( 2), X( 3), X( 4), X( 5), X( 6), X( 7), X( 8), X( 9), X(10), \
+	X(11), X(12), X(13), X(14), X(15), X(16), X(17), X(18), X(19), X(20), \
+	X(21), X(22), X(23), X(24), X(25), X(26), X(27), X(28), X(29), X(30)
+
+#define DECLARE(INDEX) x##INDEX
+#define COPY_IN(INDEX) x##INDEX = g[INDEX]
+#define COPY_OUT(INDEX) g[INDEX] = x##INDEX
+
+void
+test (int n)
+{
+  union { long long l; double d; } u = { 0x12345678 };
+  gll = u.l;
+  int MULTI (DECLARE);
+  MULTI (COPY_IN);
+  MULTI (COPY_OUT);
+  MULTI (COPY_OUT);
+  MULTI (COPY_OUT);
+  gd = u.d;
+}

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2007-08-15  6:45 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-07-26 12:56 RFA: Fix PR 32897 (invalid constants generated by find_reloads_toplev) Richard Sandiford
2007-08-06 10:25 ` Richard Sandiford
2007-08-14 13:18 ` Richard Sandiford
2007-08-15  6:45   ` Mark Mitchell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).