On January 3, 2015 10:48:47 PM CET, "H.J. Lu" <hjl.to...@gmail.com> wrote: >On Sat, Jan 3, 2015 at 12:58 PM, John David Anglin ><dave.ang...@bell.net> wrote: >> On 2015-01-03, at 3:18 PM, H.J. Lu wrote: >> >>> On Sat, Jan 3, 2015 at 12:10 PM, John David Anglin ><dave.ang...@bell.net> wrote: >>>> On 2015-01-03, at 2:48 PM, H.J. Lu wrote: >>>> >>>>> On Sat, Jan 3, 2015 at 9:35 AM, John David Anglin >>>>> <d...@hiauly1.hia.nrc.ca> wrote: >>>>>> On Wed, 31 Dec 2014, H.J. Lu wrote: >>>>>> >>>>>>> - /* Arguments for a sibling call that are pushed to memory >are passed >>>>>>> - using the incoming argument pointer of the current >function. These >>>>>>> - may or may not be frame related depending on the target. >Since >>>>>>> - argument pointer related stores are not currently >tracked, we treat >>>>>>> - a sibling call as though it does a wild read. */ >>>>>>> - if (SIBLING_CALL_P (insn)) >>>>>>> + if (targetm.sibcall_wild_read_p (insn)) >>>>>>> { >>>>>>> add_wild_read (bb_info); >>>>>>> return; >>>>>> >>>>>> Instead of falling through to code designed to handle normal >calls, it >>>>>> would be better to treat them separately. Potentially, there are >other >>>>>> optimizations that may be applicable. If a sibcall doesn't read >from >>>>>> the frame, add_non_frame_wild_read() can be called. This would >restore >>>>>> the x86 optimization. >>>>>> >>>>> >>>>> That will a new optimization. I am trying to restore the old >behavior on >>>>> x86 with minimum impact in stage 3. >>>> >>>> >>>> Not really. In gcc.dg/pr44194-1.c, the sibcall was not a const >function and this case >>>> was covered by this hunk of code: >>>> >>>> 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); >>>> >>> >>> Revision 219037 has >>> >>> diff --git a/gcc/dse.c b/gcc/dse.c >>> index 2555bd1..3a7f31c 100644 >>> --- a/gcc/dse.c >>> +++ b/gcc/dse.c >>> @@ -2483,6 +2483,17 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn) >>> >>> insn_info->cannot_delete = true; >>> >>> + /* Arguments for a sibling call that are pushed to memory are >passed >>> + using the incoming argument pointer of the current function. >These >>> + may or may not be frame related depending on the target. Since >>> + argument pointer related stores are not currently tracked, we >treat >>> + a sibling call as though it does a wild read. */ >>> + if (SIBLING_CALL_P (insn)) >>> + { >>> + add_wild_read (bb_info); >>> + return; >>> + } >>> + >>> /* Const functions cannot do anything bad i.e. read memory, >>> however, they can read their parameters which may have >>> been pushed onto the stack. >>> >>> My patch changes it to >>> >>> diff --git a/gcc/dse.c b/gcc/dse.c >>> index 2555bd1..c0e1a0c 100644 >>> --- a/gcc/dse.c >>> +++ b/gcc/dse.c >>> @@ -2483,6 +2483,12 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn) >>> >>> insn_info->cannot_delete = true; >>> >>> + if (targetm.sibcall_wild_read_p (insn)) >>> + { >>> + add_wild_read (bb_info); >>> + return; >>> + } >>> + >>> /* Const functions cannot do anything bad i.e. read memory, >>> however, they can read their parameters which may have >>> been pushed onto the stack. >>> >>> On x86, it is the same as before revision 219037 since >>> targetm.sibcall_wild_read_p always returns false on x86. >> >> >> Understood. The point is the subsequent code for const functions is >based on assumptions that >> are not generally true for sibcalls: >> >> /* This field is only used for the processing of const functions. >> These functions cannot read memory, but they can read the stack >> because that is where they may get their parms. We need to be >> this conservative because, like the store motion pass, we don't >> consider CALL_INSN_FUNCTION_USAGE when processing call insns. >> Moreover, we need to distinguish two cases: >> 1. Before reload (register elimination), the stores related to >> outgoing arguments are stack pointer based and thus deemed >> of non-constant base in this pass. This requires special >> handling but also means that the frame pointer based stores >> need not be killed upon encountering a const function call. >> 2. After reload, the stores related to outgoing arguments can be >> either stack pointer or hard frame pointer based. This means >> that we have no other choice than also killing all the frame >> pointer based stores upon encountering a const function call. >> This field is set after reload for const function calls. Having >> this set is less severe than a wild read, it just means that all >> the frame related stores are killed rather than all the stores. >*/ >> bool frame_read; >> >> For example, the stores related to sibcall arguments are not in >general stack pointer based. This >> suggests to me that we don't have to always kill stack pointer based >stores in the const sibcall case >> and they can be optimized. >> >> For me, keeping the sibcall handling separate from normal calls is >easier to understand and >> potentially provides a means to optimize stack pointer based stores. >Are you sure that the prior >> behaviour was always correct on x86 (e.g., more than 6 arguments)? >> > >I'd like to do it in 2 steps: > >1. Bring x86 back to the behavior prior to revision 21903 since it >won't >cause any regressions. >2. Investigate if sibcall is handled correctly on x86.
But either your new hook or the original fix makes no sense. Richard.