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

[John David Anglin <dave dot anglin at bell dot net>]

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)?

Dave
--
John David Anglin	dave.anglin@bell.net