Re: PR target/30961 (was: Re: GCC 4.3.0 Status Report (2007-11-04))

["H.J. Lu" <hjl at lucon dot org>]

On Tue, Nov 06, 2007 at 03:30:04PM +0100, Ulrich Weigand wrote:
> Mark Mitchell wrote:
> > H.J. Lu wrote:
> > 
> > > http://gcc.gnu.org/ml/gcc-patches/2007-08/msg01865.html
> > > 
> > > which involves reload.
> > 
> > I'm not going to try to wade into reload.  Ulrich, Eric, Ian -- would
> > one of you please review this patch?
> 
> @@ -1821,6 +1835,18 @@ find_reg (struct insn_chain *chain, int 
>  	    this_cost--;
>  	  if (rl->out && REG_P (rl->out) && REGNO (rl->out) == regno)
>  	    this_cost--;
> +#ifdef SECONDARY_MEMORY_NEEDED
> +	  /* If a memory location is needed for rl->in and dest_reg
> +	     is usable, we will favor it.  */
> +	  else if (dest_reg == regno
> +		   && rl->in
> +		   && REG_P (rl->in)
> +		   && REGNO (rl->in) < FIRST_PSEUDO_REGISTER
> +		   && SECONDARY_MEMORY_NEEDED (REGNO_REG_CLASS (REGNO (rl->in)),
> +					       rl->class,
> +					       rl->mode))
> +	    this_cost = 0;
> +#endif
> 
> 
> Hmm, this isn't really related to secondary memory.  In general,
> if we have a simple move with input reload, the destination of 
> the move should be the preferred reload register.  In fact, there
> already is code in find_reloads that is supposed to address this
> problem:
> 
>   /* Special case a simple move with an input reload and a
>      destination of a hard reg, if the hard reg is ok, use it.  */
>   for (i = 0; i < n_reloads; i++)
>     if (rld[i].when_needed == RELOAD_FOR_INPUT
>         && GET_CODE (PATTERN (insn)) == SET
>         && REG_P (SET_DEST (PATTERN (insn)))
>         && SET_SRC (PATTERN (insn)) == rld[i].in
>         && !elimination_target_reg_p (SET_DEST (PATTERN (insn))))
>       {
> 
> This does not trigger in the given test case because the SUBREG
> interferes:
> 
> Reloads for insn # 6
> Reload 0: reload_in (DF) = (reg:DF 5 di)
>         SSE_REGS, RELOAD_FOR_INPUT (opnum = 1), can't combine
>         reload_in_reg: (subreg:DF (reg/v:DI 5 di [orig:59 in ] [59]) 0)
> 
> (insn:HI 6 3 10 2 xxx.i:4
>         (set (reg:DF 21 xmm0 [orig:58 <result> ] [58])
>         (subreg:DF (reg/v:DI 5 di [orig:59 in ] [59]) 0)) 102
>  {*movdf_integer_rex64} (expr_list:REG_DEAD (reg/v:DI 5 di [orig:59 in ] [59])
>         (nil)))
> 
> Note how reload_in is not equal to the SET_SRC, but reload_in_reg is.
> In that case, the same special case should apply.
> 
> The following patch fixes the test case for me:
> 
> Index: gcc/reload.c
> ===================================================================
> --- gcc/reload.c	(revision 129925)
> +++ gcc/reload.c	(working copy)
> @@ -4462,7 +4462,8 @@
>      if (rld[i].when_needed == RELOAD_FOR_INPUT
>  	&& GET_CODE (PATTERN (insn)) == SET
>  	&& REG_P (SET_DEST (PATTERN (insn)))
> -	&& SET_SRC (PATTERN (insn)) == rld[i].in
> +	&& (SET_SRC (PATTERN (insn)) == rld[i].in
> +	    || SET_SRC (PATTERN (insn)) == rld[i].in_reg)
>  	&& !elimination_target_reg_p (SET_DEST (PATTERN (insn))))
>        {
>  	rtx dest = SET_DEST (PATTERN (insn));
> 
> 
> H.J., could you verify that this solves your problem?
> 

Yes, it works for me. I tested it on Linux/ia32, Linux/intel64
and linux/ia64. There are no regressions.

Thanks.


H.J.
----
gcc/

2007-11-06  Ulrich Weigand  <Ulrich.Weigand@de.ibm.com>

	PR target/30961
	* reload1.c (find_reloads): Also check in_reg when handling a
	simple move with an input reload and a destination of a hard
	register.

gcc/testsuite/

2007-11-06  H.J. Lu  <hongjiu.lu@intel.com>

	PR target/30961
	* gcc.target/i386/pr30961-1.c: New.

--- gcc/reload.c.second	2007-09-08 09:50:55.000000000 -0700
+++ gcc/reload.c	2007-11-06 07:43:52.000000000 -0800
@@ -4464,7 +4464,8 @@ find_reloads (rtx insn, int replace, int
     if (rld[i].when_needed == RELOAD_FOR_INPUT
 	&& GET_CODE (PATTERN (insn)) == SET
 	&& REG_P (SET_DEST (PATTERN (insn)))
-	&& SET_SRC (PATTERN (insn)) == rld[i].in)
+	&& (SET_SRC (PATTERN (insn)) == rld[i].in
+	    || SET_SRC (PATTERN (insn)) == rld[i].in_reg))
       {
 	rtx dest = SET_DEST (PATTERN (insn));
 	unsigned int regno = REGNO (dest);
--- gcc/testsuite/gcc.target/i386/pr30961-1.c.second	2007-11-06 07:41:26.000000000 -0800
+++ gcc/testsuite/gcc.target/i386/pr30961-1.c	2007-11-06 07:41:26.000000000 -0800
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target lp64 } */
+/* { dg-options "-O2" } */
+
+double
+convert (long long in)
+{
+  double f;
+  __builtin_memcpy( &f, &in, sizeof( in ) );
+  return f;
+}
+
+/* { dg-final { scan-assembler-not "movapd" } } */