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.
--
H.J.
