Re: [PATCH] Treat a sibling call as though it does a wild read

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

On Tue, Dec 23, 2014 at 6:16 PM, John David Anglin <dave.anglin@bell.net> wrote:
> On 23-Dec-14, at 7:28 PM, H.J. Lu wrote:
>
>> On Tue, Dec 23, 2014 at 3:55 PM, John David Anglin <dave.anglin@bell.net>
>> wrote:
>>>
>>> On 23-Dec-14, at 5:37 PM, H.J. Lu wrote:
>>>
>>>> In this case,  arguments are passed in registers.  Isn't the
>>>> optimization
>>>> disabled for ia32, which passes arguments on stack, even before your
>>>> change?
>>>
>>>
>>>
>>> It's not disabled in dse.c.  Possibly, this occurs for some cases for
>>> ia32
>>> in ix86_function_ok_for_sibcall.
>>>
>>> The problem in dse is in how to distinguish stores used for arguments
>>> from
>>> those used for general
>>> manipulation of data.  It seems the argument data for the call to bar in
>>> the
>>> testcase are copied through
>>> frame memory on x86_64.
>>>
>>> We have two situations:
>>>
>>>  /* 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;
>>>
>>> Case 1 is incorrect for sibling calls as the call arguments are frame or
>>> argument pointer based
>>> when they are not passed in registers.
>>>
>>> When we don't have a const function, dse assumes:
>>>
>>>        /* Every other call, including pure functions, may read any memory
>>>           that is not relative to the frame.  */
>>>        add_non_frame_wild_read (bb_info);
>>>
>>> Again, this is incorrect for sibling calls (i.e., dse in general assumes
>>> that call arguments are register
>>> or stack pointer based before reload).
>>>
>>
>> This optimization is done on purpose to fix:
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=44194
>>
>> Unless it is also incorrect for x86, why not only disable it for
>> your target without penalizing x86 through a target hook.
>
>
>
> I tend to think the old code was wrong for x86 but I recognize the lost
> optimization.
>
> This isn't an optimization issue on hppa.  It's a wrong code bug.

I run gcc.dg/pr55023.c on x86-64 and x86 without your fix.  It works
fine.  Can you find a testcase, failing on x86-64 and x86, which is
fixed by your change?  If not,  this bug doesn't affect x86 and you
should find another way to fix your target problem without introducing
a regression on x86.

> Richard wrote regarding 44194:
> "So we create a stack representation to copy it to the stack temporary
> (which both
> are unneeded). We should see that we can avoid the temporary at all as there
> is no
> aggregate use of the struct left. At least we should avoid the 2nd
> temporary."
> The temporaries are still there and they are the problem.
> I don't see that this is a target issue.  There were nothing more than
> argument stores in
> the testcase for the PR 55023 and they were deleted by dse.
>

Thanks.

-- 
H.J.