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

[H.J. Lu]

On Sun, Jan 4, 2015 at 3:37 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> 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.

All I want is to restore the old behavior on x86.  If the original fix
makes no sense, should it be reverted?


-- 
H.J.