| Message ID | 20120502.042508.1942825090945678387.davem@davemloft.net |
|---|---|
| State | New |
| Headers | show |
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; > } >
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
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?
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.
--- 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; }