Re: PATCH: [5 Regression] r219037 caused FAIL: gcc.dg/pr44194-1.c

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Sat, Jan 3, 2015 at 12:58 PM, John David Anglin <dave.anglin@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.anglin@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
>>>> <dave@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.