Re: [PATCH] Fix overzealous DSE on sparc
> Ok, so I plan to push this sparc fix into mainline and the 4.7 branch after > my testing is done. > > Eric, any objections? For the record, none. -- Eric Botcazou
Re: [PATCH] Fix overzealous DSE on sparc
From: Richard Guenther Date: Thu, 3 May 2012 12:25:11 +0200 > On Thu, May 3, 2012 at 11:52 AM, David Miller wrote: >> From: Richard Guenther >> Date: Thu, 3 May 2012 11:48:03 +0200 >> >>> calls.c is unsafe, too. Which is probably why making DSE stronger for >>> calls using the usual alias predicates did not work. >> >> Well, when calls.c does something more sophisticated we can do similarly >> for cases like sparc's libcalls. > > Sure. Ok, so I plan to push this sparc fix into mainline and the 4.7 branch after my testing is done. Eric, any objections?
Re: [PATCH] Fix overzealous DSE on sparc
On Thu, May 3, 2012 at 11:52 AM, David Miller wrote: > From: Richard Guenther > Date: Thu, 3 May 2012 11:48:03 +0200 > >> calls.c is unsafe, too. Which is probably why making DSE stronger for >> calls using the usual alias predicates did not work. > > Well, when calls.c does something more sophisticated we can do similarly > for cases like sparc's libcalls. Sure.
Re: [PATCH] Fix overzealous DSE on sparc
From: Richard Guenther Date: Thu, 3 May 2012 11:48:03 +0200 > calls.c is unsafe, too. Which is probably why making DSE stronger for > calls using the usual alias predicates did not work. Well, when calls.c does something more sophisticated we can do similarly for cases like sparc's libcalls.
Re: [PATCH] Fix overzealous DSE on sparc
On Thu, May 3, 2012 at 11:38 AM, David Miller wrote: > From: Richard Sandiford > Date: Thu, 03 May 2012 10:17:44 +0100 > >> Richard Guenther writes: >>> On Thu, May 3, 2012 at 10:31 AM, Richard Sandiford >>> wrote: David Miller writes: > From: Richard Sandiford > 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. > > The following seems to work, and matches what calls.c does when it > passes a value by reference. Is this what you had in mind? Yeah, looks good to me. >>> >>> I don't think that will work reliably (well, maybe now by luck, so better >>> than >>> nothing). You'd at least need to adjust the ESCAPED points-to set of the >>> function, too (yes, DSE does very very conservative use analysis right now). >> >> Ah. >> >>> Why not simply clear MEM_EXPR for the MEM? >> >> The problem is that MEM rtxes aren't shared. AIUI, the store will already >> have been emitted by this point, using a distinct (but equivalent) MEM rtx. > > Again, why doesn't calls.c have to do anything about this when it > passes arguments by reference? It does exactly what I'm changing the > sparc libcall code to do. calls.c is unsafe, too. Which is probably why making DSE stronger for calls using the usual alias predicates did not work. Richard.
Re: [PATCH] Fix overzealous DSE on sparc
From: Richard Sandiford Date: Thu, 03 May 2012 10:17:44 +0100 > Richard Guenther writes: >> On Thu, May 3, 2012 at 10:31 AM, Richard Sandiford >> wrote: >>> David Miller writes: From: Richard Sandiford 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. The following seems to work, and matches what calls.c does when it passes a value by reference. Is this what you had in mind? >>> >>> Yeah, looks good to me. >> >> I don't think that will work reliably (well, maybe now by luck, so better >> than >> nothing). You'd at least need to adjust the ESCAPED points-to set of the >> function, too (yes, DSE does very very conservative use analysis right now). > > Ah. > >> Why not simply clear MEM_EXPR for the MEM? > > The problem is that MEM rtxes aren't shared. AIUI, the store will already > have been emitted by this point, using a distinct (but equivalent) MEM rtx. Again, why doesn't calls.c have to do anything about this when it passes arguments by reference? It does exactly what I'm changing the sparc libcall code to do.
Re: [PATCH] Fix overzealous DSE on sparc
From: Richard Guenther Date: Thu, 3 May 2012 10:42:30 +0200 > On Thu, May 3, 2012 at 10:31 AM, Richard Sandiford > wrote: >> David Miller writes: >>> From: Richard Sandiford >>> 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. >>> >>> The following seems to work, and matches what calls.c does when it >>> passes a value by reference. Is this what you had in mind? >> >> Yeah, looks good to me. > > I don't think that will work reliably (well, maybe now by luck, so better than > nothing). You'd at least need to adjust the ESCAPED points-to set of the > function, too (yes, DSE does very very conservative use analysis right now). > Why not simply clear MEM_EXPR for the MEM? Then why does calls.c not have to do this?
Re: [PATCH] Fix overzealous DSE on sparc
Richard Guenther writes: > On Thu, May 3, 2012 at 10:31 AM, Richard Sandiford > wrote: >> David Miller writes: >>> From: Richard Sandiford >>> 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. >>> >>> The following seems to work, and matches what calls.c does when it >>> passes a value by reference. Is this what you had in mind? >> >> Yeah, looks good to me. > > I don't think that will work reliably (well, maybe now by luck, so better than > nothing). You'd at least need to adjust the ESCAPED points-to set of the > function, too (yes, DSE does very very conservative use analysis right now). Ah. > Why not simply clear MEM_EXPR for the MEM? The problem is that MEM rtxes aren't shared. AIUI, the store will already have been emitted by this point, using a distinct (but equivalent) MEM rtx. Richard
Re: [PATCH] Fix overzealous DSE on sparc
On Thu, May 3, 2012 at 10:31 AM, Richard Sandiford wrote: > David Miller writes: >> From: Richard Sandiford >> 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. >> >> The following seems to work, and matches what calls.c does when it >> passes a value by reference. Is this what you had in mind? > > Yeah, looks good to me. I don't think that will work reliably (well, maybe now by luck, so better than nothing). You'd at least need to adjust the ESCAPED points-to set of the function, too (yes, DSE does very very conservative use analysis right now). Why not simply clear MEM_EXPR for the MEM? Richard. > Richard
Re: [PATCH] Fix overzealous DSE on sparc
David Miller writes: > From: Richard Sandiford > 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. > > The following seems to work, and matches what calls.c does when it > passes a value by reference. Is this what you had in mind? Yeah, looks good to me. Richard
Re: [PATCH] Fix overzealous DSE on sparc
From: Richard Sandiford
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.
The following seems to work, and matches what calls.c does when it
passes a value by reference. Is this what you had in mind?
2012-05-02 David S. Miller
* config/sparc/sparc.c (emit_soft_tfmode_libcall): If we pass a
MEM directly into a libcall, mark it's MEM_EXPR as addressable.
(sparc_emit_float_lib_cmp): Likewise.
diff --git a/gcc/config/sparc/sparc.c b/gcc/config/sparc/sparc.c
index b987648..7434c0f 100644
--- a/gcc/config/sparc/sparc.c
+++ b/gcc/config/sparc/sparc.c
@@ -2698,7 +2698,12 @@ emit_soft_tfmode_libcall (const char *func_name, int
nargs, rtx *operands)
if (GET_CODE (this_arg) == MEM
&& ! force_stack_temp)
- this_arg = XEXP (this_arg, 0);
+ {
+ tree expr = MEM_EXPR (this_arg);
+ if (expr)
+ mark_addressable (expr);
+ this_arg = XEXP (this_arg, 0);
+ }
else if (CONSTANT_P (this_arg)
&& ! force_stack_temp)
{
@@ -7387,7 +7392,12 @@ sparc_emit_float_lib_cmp (rtx x, rtx y, enum rtx_code
comparison)
if (TARGET_ARCH64)
{
if (MEM_P (x))
- slot0 = x;
+ {
+ tree expr = MEM_EXPR (x);
+ if (expr)
+ mark_addressable (expr);
+ slot0 = x;
+ }
else
{
slot0 = assign_stack_temp (TFmode, GET_MODE_SIZE(TFmode), 0);
@@ -7395,7 +7405,12 @@ sparc_emit_float_lib_cmp (rtx x, rtx y, enum rtx_code
comparison)
}
if (MEM_P (y))
- slot1 = y;
+ {
+ tree expr = MEM_EXPR (y);
+ if (expr)
+ mark_addressable (expr);
+ slot1 = y;
+ }
else
{
slot1 = assign_stack_temp (TFmode, GET_MODE_SIZE(TFmode), 0);
Re: [PATCH] Fix overzealous DSE on sparc
From: Richard Sandiford 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.
Re: [PATCH] Fix overzealous DSE on sparc
From: Richard Sandiford 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?
Re: [PATCH] Fix overzealous DSE on sparc
David Miller 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
Re: [PATCH] Fix overzealous DSE on sparc
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
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;
}
