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

[Richard Biener]

On January 3, 2015 10:48:47 PM CET, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>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.

But either your new hook or the original fix makes no sense.

Richard.