Patchwork Fix overzealous DSE on sparc

login
register
mail settings
Submitter David Miller
Date May 2, 2012, 8:25 a.m.
Message ID <20120502.042508.1942825090945678387.davem@davemloft.net>
Download mbox | patch
Permalink /patch/156383/
State New
Headers show

Comments

David Miller - May 2, 2012, 8:25 a.m.
On targets such as sparc, where ARG_POINTER_REGNUM ==
FRAME_POINTER_REGNUM, some of the invariants currently built into DSE
simply do not hold.

Unlike how DSE assumes, we will in fact see stores to frame pointer
relative addresses for setting up outgoing arguments to a CALL.

The result is that DSE can eliminate stores that setup those
arguments.

Two test cases and a DSE debugging dump for one of them can be found
at:

	http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52684

Richard and Kenneth, can you take a look at this?  The patch is
against the 4.7 branch, which is where I've been tracking this down.
I'm currently running it through a regstrap and a glibc build (the
latter of which is where I originally saw this problem)

Thanks.

2012-05-01  David S. Miller  <davem@davemloft.net>

	PR target/53684
	* dse.c (scan_insn): When the frame pointer is used as the
	argument pointer, mark non-const calls as frame_read.
Kenneth Zadeck - May 2, 2012, 12:27 p.m.
this looks ok to me, but I am going to defer this to richard for final 
acceptance.   his contribution to this pass was his deep knowledge of 
rtl so that we could get the scanning correct and this is clearly in 
that domain.   He may have some trick that does not throw all of the 
baby out with the bath water.

Kenny

On 05/02/2012 04:25 AM, David Miller wrote:
> On targets such as sparc, where ARG_POINTER_REGNUM ==
> FRAME_POINTER_REGNUM, some of the invariants currently built into DSE
> simply do not hold.
>
> Unlike how DSE assumes, we will in fact see stores to frame pointer
> relative addresses for setting up outgoing arguments to a CALL.
>
> The result is that DSE can eliminate stores that setup those
> arguments.
>
> Two test cases and a DSE debugging dump for one of them can be found
> at:
>
> 	http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52684
>
> Richard and Kenneth, can you take a look at this?  The patch is
> against the 4.7 branch, which is where I've been tracking this down.
> I'm currently running it through a regstrap and a glibc build (the
> latter of which is where I originally saw this problem)
>
> Thanks.
>
> 2012-05-01  David S. Miller<davem@davemloft.net>
>
> 	PR target/53684
> 	* dse.c (scan_insn): When the frame pointer is used as the
> 	argument pointer, mark non-const calls as frame_read.
>
> --- gcc/dse.c~	2012-05-01 20:27:48.163611555 -0700
> +++ gcc/dse.c	2012-05-02 01:03:19.450719154 -0700
> @@ -2646,10 +2646,17 @@ scan_insn (bb_info_t bb_info, rtx insn)
>   	}
>
>         else
> -	/* Every other call, including pure functions, may read any memory
> -           that is not relative to the frame.  */
> -        add_non_frame_wild_read (bb_info);
> -
> +	{
> +	  /* Every other call, including pure functions, may read any memory
> +	     that is not relative to the frame.  */
> +	  add_non_frame_wild_read (bb_info);
> +#if FRAME_POINTER_REGNUM == ARG_POINTER_REGNUM
> +	  /* If the target uses the frame pointer as the argument
> +	     pointer, then we will encounter frame relative stores
> +	     that setup outgoing arguments to a function.  */
> +	  insn_info->frame_read = true;
> +#endif
> +	}
>         return;
>       }
>
Richard Sandiford - May 2, 2012, 7:37 p.m.
David Miller <davem@davemloft.net> writes:
> On targets such as sparc, where ARG_POINTER_REGNUM ==
> FRAME_POINTER_REGNUM, some of the invariants currently built into DSE
> simply do not hold.
>
> Unlike how DSE assumes, we will in fact see stores to frame pointer
> relative addresses for setting up outgoing arguments to a CALL.
>
> The result is that DSE can eliminate stores that setup those
> arguments.
>
> Two test cases and a DSE debugging dump for one of them can be found
> at:
>
> 	http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52684
>
> Richard and Kenneth, can you take a look at this?  The patch is
> against the 4.7 branch, which is where I've been tracking this down.
> I'm currently running it through a regstrap and a glibc build (the
> latter of which is where I originally saw this problem)

This looks a little odd.  ARG_POINTER_REGNUM is for incoming arguments
rather than outgoing arguments, so I don't see why it matters whether
it's the same as FRAME_POINTER_REGNUM.

I think the problem is in the way sparc.c:emit_soft_tfmode_libcall
reuses existing MEMs when passing arguments on the stack:

	  if (GET_CODE (this_arg) == MEM
	      && ! force_stack_temp)
	    this_arg = XEXP (this_arg, 0);

-ffloat-store forces "a" and "b" to be stored in their argument slots,
and emit_soft_tfmode_libcall then passes the address of these incoming
argument slots to the libcall.  But "a" and "b" don't escape, so DSE
thinks that the call can't read them.

Even after your change, I think the same thing could happen in cases
where -ffloat-store forces local variables rather than incoming
arguments to the stack, and where those local variables are then
passed as calls.

As you say, it works for 32-bit because that uses the normal
libcall code.

I think the DSE assuption is fair though.  If you reuse MEMs like this,
then they're no longer just serving the purpose described by MEM_EXPR.

Richard
David Miller - May 2, 2012, 11:36 p.m.
From: Richard Sandiford <rdsandiford@googlemail.com>
Date: Wed, 02 May 2012 20:37:58 +0100

> I think the problem is in the way sparc.c:emit_soft_tfmode_libcall
> reuses existing MEMs when passing arguments on the stack:
> 
> 	  if (GET_CODE (this_arg) == MEM
> 	      && ! force_stack_temp)
> 	    this_arg = XEXP (this_arg, 0);
> 
> -ffloat-store forces "a" and "b" to be stored in their argument slots,
> and emit_soft_tfmode_libcall then passes the address of these incoming
> argument slots to the libcall.  But "a" and "b" don't escape, so DSE
> thinks that the call can't read them.

I'm fine with adjusting how we emit libcalls to better show the compiler
what's going on.

Can you suggest a way that we can mark these MEMs so that the rest of
the compiler will know that these values can in fact escape?
David Miller - May 3, 2012, 2:21 a.m.
From: Richard Sandiford <rdsandiford@googlemail.com>
Date: Wed, 02 May 2012 20:37:58 +0100

> I think the DSE assuption is fair though.  If you reuse MEMs like this,
> then they're no longer just serving the purpose described by MEM_EXPR.

I think what Sparc does is fair, so if you are going to suggest that
I re-pop these values into new stack slots I'm going to have a hard
time swallowing that.

In fact, using the incoming argument slots when passing outgoing
arguments by reference is exactly what I want the sparc backend to be
doing.

Furthermore, in the future, I'd like the compiler to be able to use
these argument slots when stack temporaries are necessary since every
sparc stack frame has to allocate 6 argument slots even if no
arguments are passed to the function.

This argument slot re-use is a rather common optimization in hand
written sparc assembler because this allows us to avoid having to
allocate a register window and a stack frame at all, and thus end up
also with a leaf procedure even when we need stack temporaries.

Patch

--- gcc/dse.c~	2012-05-01 20:27:48.163611555 -0700
+++ gcc/dse.c	2012-05-02 01:03:19.450719154 -0700
@@ -2646,10 +2646,17 @@  scan_insn (bb_info_t bb_info, rtx insn)
 	}
 
       else
-	/* Every other call, including pure functions, may read any memory
-           that is not relative to the frame.  */
-        add_non_frame_wild_read (bb_info);
-
+	{
+	  /* Every other call, including pure functions, may read any memory
+	     that is not relative to the frame.  */
+	  add_non_frame_wild_read (bb_info);
+#if FRAME_POINTER_REGNUM == ARG_POINTER_REGNUM
+	  /* If the target uses the frame pointer as the argument
+	     pointer, then we will encounter frame relative stores
+	     that setup outgoing arguments to a function.  */
+	  insn_info->frame_read = true;
+#endif
+	}
       return;
     }