On January 4, 2015 3:57:07 PM CET, "H.J. Lu" <hjl.to...@gmail.com> wrote:
>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?
I think the original fix is too conservative
But I also think there is a target independent bug, thus a target hook to "fix"
things for x86 makes no sense.
Richard.
