On Sun, Jan 4, 2015 at 3:37 AM, Richard Biener
<richard.guent...@gmail.com> wrote:
> 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.
All I want is to restore the old behavior on x86. If the original fix
makes no sense, should it be reverted?
--
H.J.
