Patchwork [RFC] fix reload causing ICE in subreg_get_info on m68k (PR58369)

login
register
mail settings
Submitter Mikael Pettersson
Date Sept. 28, 2013, 3:30 p.m.
Message ID <21062.62988.797370.296379@gargle.gargle.HOWL>
Download mbox | patch
Permalink /patch/278750/
State New
Headers show

Comments

Mikael Pettersson - Sept. 28, 2013, 3:30 p.m.
This patch fixes PR58369, an ICE in subreg_get_info when compiling
boost for m68k-linux.

choose_reload_regs attempts to reload a DFmode (8-byte) reg, finds
an XFmode (12-byte) reg in "last_reg", and calls subreg_regno_offset
with these two modes and a subreg offset of zero.  However, this is
not a correct lowpart subreg offset for big-endian and these two modes,
so the lowpart subreg check in subreg_get_info fails, and the code
continues to

    gcc_assert ((GET_MODE_SIZE (xmode) % GET_MODE_SIZE (ymode)) == 0);

which fails because (12 % 8) != 0.

choose_reload_regs passes the constant zero, in all cases where the reg
isn't already a subreg, as the subreg offset to subreg_regno_offset, even
though lowpart subregs on big-endian targets require an explicit offset
computation.  I think that is a bug.

I believe other big-endian targets don't see this ICE because
a) they define CANNOT_CHANGE_MODE_CLASS to reject differently-sized
   modes in floating-point registers (which prevents this path in
   choose_reload_regs), or
b) their differently-sized modes are such that the size of a larger
   mode is a whole multiple of the size of the smaller mode (which
   allows the gcc_assert above to pass).

This patch changes choose_reload_regs to call subreg_lowpart_offset
to pass an endian-correct offset to subreg_regno_offset, except where
the offset comes from a pre-existing subreg.

[Defining CANNOT_CHANGE_MODE_CLASS appropriately for m68k also fixes
the ICE, but I don't think the m68k backend really wants that, and I
think it just papers over a generic bug.]

Tested with trunk and 4.8 on {m68k,sparc64,powerpc64}-linux (big-endian),
and on x86_64-linux/armv5tel-linux-gnueabi (little-endian).  No regressions.

Comments?
Is this Ok for trunk?

gcc/

2013-09-28  Mikael Pettersson  <mikpelinux@gmail.com>

	PR rtl-optimization/58369
	* reload1.c (choose_reload_regs): Use subreg_lowpart_offset
	to pass endian-correct lowpart offset to subreg_regno_offset.
Jeff Law - Oct. 9, 2013, 7:20 p.m.
On 09/28/13 09:30, Mikael Pettersson wrote:
> This patch fixes PR58369, an ICE in subreg_get_info when compiling
> boost for m68k-linux.
>
> choose_reload_regs attempts to reload a DFmode (8-byte) reg, finds
> an XFmode (12-byte) reg in "last_reg", and calls subreg_regno_offset
> with these two modes and a subreg offset of zero.  However, this is
> not a correct lowpart subreg offset for big-endian and these two modes,
> so the lowpart subreg check in subreg_get_info fails, and the code
> continues to
>
>      gcc_assert ((GET_MODE_SIZE (xmode) % GET_MODE_SIZE (ymode)) == 0);
>
> which fails because (12 % 8) != 0.
>
> choose_reload_regs passes the constant zero, in all cases where the reg
> isn't already a subreg, as the subreg offset to subreg_regno_offset, even
> though lowpart subregs on big-endian targets require an explicit offset
> computation.  I think that is a bug.
>
> I believe other big-endian targets don't see this ICE because
> a) they define CANNOT_CHANGE_MODE_CLASS to reject differently-sized
>     modes in floating-point registers (which prevents this path in
>     choose_reload_regs), or
> b) their differently-sized modes are such that the size of a larger
>     mode is a whole multiple of the size of the smaller mode (which
>     allows the gcc_assert above to pass).
>
> This patch changes choose_reload_regs to call subreg_lowpart_offset
> to pass an endian-correct offset to subreg_regno_offset, except where
> the offset comes from a pre-existing subreg.
>
> [Defining CANNOT_CHANGE_MODE_CLASS appropriately for m68k also fixes
> the ICE, but I don't think the m68k backend really wants that, and I
> think it just papers over a generic bug.]
>
> Tested with trunk and 4.8 on {m68k,sparc64,powerpc64}-linux (big-endian),
> and on x86_64-linux/armv5tel-linux-gnueabi (little-endian).  No regressions.
>
> Comments?
> Is this Ok for trunk?
>
> gcc/
>
> 2013-09-28  Mikael Pettersson  <mikpelinux@gmail.com>
>
> 	PR rtl-optimization/58369
> 	* reload1.c (choose_reload_regs): Use subreg_lowpart_offset
> 	to pass endian-correct lowpart offset to subreg_regno_offset.
Thanks Mikael.  My only concern is the lack of adjustment when the value 
found was already a SUBREG.

ie, let's assume rld[r].in_reg was something like
(subreg:XF (reg:DF) 0)

and our target is (reg:DF)

In this case it seems to me we still want to compute the subreg offset, 
right?

jeff

Patch

--- gcc-4.9-20130922/gcc/reload1.c.~1~	2013-09-09 15:07:10.000000000 +0200
+++ gcc-4.9-20130922/gcc/reload1.c	2013-09-28 16:24:21.068294912 +0200
@@ -6497,6 +6497,7 @@  choose_reload_regs (struct insn_chain *c
 	  if (inheritance)
 	    {
 	      int byte = 0;
+	      bool byte_is_fixed = false;
 	      int regno = -1;
 	      enum machine_mode mode = VOIDmode;
 
@@ -6519,7 +6520,10 @@  choose_reload_regs (struct insn_chain *c
 		  if (regno < FIRST_PSEUDO_REGISTER)
 		    regno = subreg_regno (rld[r].in_reg);
 		  else
-		    byte = SUBREG_BYTE (rld[r].in_reg);
+		    {
+		      byte = SUBREG_BYTE (rld[r].in_reg);
+		      byte_is_fixed = true;
+		    }
 		  mode = GET_MODE (rld[r].in_reg);
 		}
 #ifdef AUTO_INC_DEC
@@ -6557,6 +6561,8 @@  choose_reload_regs (struct insn_chain *c
 		  rtx last_reg = reg_last_reload_reg[regno];
 
 		  i = REGNO (last_reg);
+		  if (! byte_is_fixed)
+		    byte = subreg_lowpart_offset (mode, GET_MODE (last_reg));
 		  i += subreg_regno_offset (i, GET_MODE (last_reg), byte, mode);
 		  last_class = REGNO_REG_CLASS (i);