Re: RFA: Fix PR 32897 (invalid constants generated by find_reloads_toplev)

[Richard Sandiford <richard at codesourcery dot com>]

Ping^2

Richard Sandiford <richard@codesourcery.com> writes:
> 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;
> +}