On 11/28/14 18:05, John David Anglin wrote:
The attached simple change fixes a long standing regression since 4.2.
When we have stack arguments and a sibling
call, the dse pass deletes stores to the parent's stack frame when we
have a sibling call because they are are off frame
for the current function. As a result, the sibling call arguments are
not passed correctly on PA.
The attached change disables this behavior.
Tested on hppa2.0w-hp-hpux11.11, hppa64-hp-hpux11.11 and
hppa-unknown-linux-gnu.
Okay for trunk, 4.9 and 4.8?
What is the insn_info for the sibcall itself? I'm particularly
interested in the frame_read flag.
Prior to reload (ie, in DSE1) there's a bit of magic in that we do not
set frame_read on call insns. That may in fact be wrong and possibly
the source of the problem.
/* 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;
As a test, what happens if we change:
/* See the head comment of the frame_read field. */
if (reload_completed)
insn_info->frame_read = true;
To do unconditionally set frame_read? Or if we don't want to get that
drastic,
if (reload_completed || SIBLING_CALL_P (insn))
insn_info->frame_read = true;
As for the patch itself, it'd be good to include a testcase,
particularly since the BZ has a nice simple one.
It feels like setting stores_off_frame_dead_at_return, while effective
is the wrong solution. But if we go in that direction, then we clearly
need a comment where we set that for sibling calls and the comment for
that variable will need to be updated since it says it's only used for
stdarg functions.
Jeff
*I
Jeff
