* [PATCH] Set stores_off_frame_dead_at_return to false if sibling call
@ 2014-11-29 10:26 John David Anglin
2014-12-01 16:57 ` Jeff Law
0 siblings, 1 reply; 25+ messages in thread
From: John David Anglin @ 2014-11-29 10:26 UTC (permalink / raw)
To: GCC Patches; +Cc: Jeff Law
[-- Attachment #1: Type: text/plain, Size: 550 bytes --]
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?
Dave
--
John David Anglin dave.anglin@bell.net
[-- Attachment #2: dse.c.d.txt --]
[-- Type: text/plain, Size: 602 bytes --]
2014-11-28 John David Anglin <danglin@gcc.gnu.org>
PR target/55023
* dse.c (scan_insn): Set stores_off_frame_dead_at_return to false if
we have a sibling call.
Index: dse.c
===================================================================
--- dse.c (revision 217974)
+++ dse.c (working copy)
@@ -2483,6 +2483,9 @@
insn_info->cannot_delete = true;
+ if (SIBLING_CALL_P (insn))
+ stores_off_frame_dead_at_return = false;
+
/* Const functions cannot do anything bad i.e. read memory,
however, they can read their parameters which may have
been pushed onto the stack.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call
2014-11-29 10:26 [PATCH] Set stores_off_frame_dead_at_return to false if sibling call John David Anglin
@ 2014-12-01 16:57 ` Jeff Law
2014-12-01 18:44 ` John David Anglin
` (2 more replies)
0 siblings, 3 replies; 25+ messages in thread
From: Jeff Law @ 2014-12-01 16:57 UTC (permalink / raw)
To: John David Anglin, GCC Patches
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call
2014-12-01 16:57 ` Jeff Law
@ 2014-12-01 18:44 ` John David Anglin
2014-12-08 21:14 ` Jeff Law
2014-12-04 15:50 ` John David Anglin
2014-12-07 19:11 ` [PATCH] Correctly handle stack arguments of sibling calls in DSE pass John David Anglin
2 siblings, 1 reply; 25+ messages in thread
From: John David Anglin @ 2014-12-01 18:44 UTC (permalink / raw)
To: Jeff Law, GCC Patches
On 12/1/2014 11:57 AM, Jeff Law wrote:
> 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;
Will test but I, if I read the code correctly, setting
insn_info->frame_read = true
results in frame related stores being killed in a constant call. This
doesn't seem
like the right solution.
Here we have frame related calls being killed before reload because the
argument
stores for the sibcall are off frame:
/* Set the gen set of the exit block, and also any block with no
successors that does not have a wild read. */
static void
dse_step3_exit_block_scan (bb_info_t bb_info)
{
/* The gen set is all 0's for the exit block except for the
frame_pointer_group. */
if (stores_off_frame_dead_at_return)
{
unsigned int i;
group_info_t group;
FOR_EACH_VEC_ELT (rtx_group_vec, i, group)
{
if (group->process_globally && group->frame_related)
bitmap_ior_into (bb_info->gen, group->group_kill);
}
}
}
Dave
--
John David Anglin dave.anglin@bell.net
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call
2014-12-01 16:57 ` Jeff Law
2014-12-01 18:44 ` John David Anglin
@ 2014-12-04 15:50 ` John David Anglin
2014-12-08 21:20 ` Jeff Law
2014-12-07 19:11 ` [PATCH] Correctly handle stack arguments of sibling calls in DSE pass John David Anglin
2 siblings, 1 reply; 25+ messages in thread
From: John David Anglin @ 2014-12-04 15:50 UTC (permalink / raw)
To: Jeff Law, GCC Patches
On 12/1/2014 11:57 AM, Jeff Law wrote:
> 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.
The above is wrong for sibcalls. Sibcall arguments are relative to the
incoming
argument pointer. Is this always the frame pointer?
If that is the case, then it may be possible to eliminate stack based
stores when
we have a sibcall before reload.
> if (reload_completed || SIBLING_CALL_P (insn))
> insn_info->frame_read = true;
This works.
Dave
--
John David Anglin dave.anglin@bell.net
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] Correctly handle stack arguments of sibling calls in DSE pass
2014-12-01 16:57 ` Jeff Law
2014-12-01 18:44 ` John David Anglin
2014-12-04 15:50 ` John David Anglin
@ 2014-12-07 19:11 ` John David Anglin
2 siblings, 0 replies; 25+ messages in thread
From: John David Anglin @ 2014-12-07 19:11 UTC (permalink / raw)
To: Jeff Law; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 2561 bytes --]
On 1-Dec-14, at 11:57 AM, Jeff Law wrote:
> 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;
The attached patch uses your suggestion above. It also doesn't kill
stack based stores
before reload when we have a sibling call.
Sibling calls use the incoming argument pointer to store arguments for
the sibcall. Typically,
the argument pointer and the frame pointer are related, but not on
hppa64 where sibcalls are
disabled. So, I have to wonder if the first approach wasn't better.
The change updates the comment for the frame_read field to mention
sibling calls and
adds a testcase.
Tested on hppa-unknown-linux-gnu, hppa2.0w-hp-hpux11.11, hppa64-hp-
hpux11.11
with no observed regressions.
Bootstrap tested on on i686-unknown-linux-gnu.
Okay?
Dave
--
John David Anglin dave.anglin@bell.net
[-- Attachment #2: dse.c.d.2.txt --]
[-- Type: text/plain, Size: 2549 bytes --]
2014-12-07 John David Anglin <danglin@gcc.gnu.org>
PR target/55023
* dse.c (frame_read): Update comment.
(scan_insn): Set insn_info->frame_read to true for sibling calls.
Don't remove stack pointer based stores for sibling calls before
reload.
Index: gcc/dse.c
===================================================================
--- gcc/dse.c (revision 218467)
+++ gcc/dse.c (working copy)
@@ -363,7 +363,8 @@
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
+ outgoing arguments are stack pointer based, except for sibling
+ calls which use incoming arguments, and thus they are 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.
@@ -2516,7 +2517,7 @@
const_call ? "const" : "memset", INSN_UID (insn));
/* See the head comment of the frame_read field. */
- if (reload_completed)
+ if (reload_completed || SIBLING_CALL_P (insn))
insn_info->frame_read = true;
/* Loop over the active stores and remove those which are
@@ -2525,8 +2526,11 @@
{
bool remove_store = false;
- /* The stack pointer based stores are always killed. */
- if (i_ptr->stack_pointer_based)
+ /* The stack pointer based stores are always killed for
+ non sibling calls. Before reload, the arguments of a
+ sibling call are frame pointer based. */
+ if (i_ptr->stack_pointer_based
+ && (reload_completed || !SIBLING_CALL_P (insn)))
remove_store = true;
/* If the frame is read, the frame related stores are killed. */
--- /dev/null 2014-11-28 19:44:51.440000000 -0500
+++ gcc/testsuite/gcc.dg/pr55023.c 2014-12-04 19:49:45.887701600 -0500
@@ -0,0 +1,33 @@
+/* PR rtl-optimization/55023 */
+/* { dg-do run } */
+/* { dg-options "-O2 -fno-inline" } */
+
+extern void abort (void);
+typedef long long int64_t;
+
+struct foo {
+ int x;
+ int y;
+};
+
+int64_t foo(int64_t a, int64_t b, int64_t c)
+{
+ return a + b + c;
+}
+
+int64_t bar(int64_t a, struct foo bq, struct foo cq)
+{
+ int64_t b = bq.x + bq.y;
+ int64_t c = cq.x + cq.y;
+ return foo(a, b, c);
+}
+
+int main(void)
+{
+ int64_t a = 1;
+ struct foo b = { 2, 3 };
+ struct foo c = { 4, 5 };
+ if (bar (a, b, c) != 15)
+ abort ();
+ return 0;
+}
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call
2014-12-01 18:44 ` John David Anglin
@ 2014-12-08 21:14 ` Jeff Law
2014-12-08 21:49 ` John David Anglin
0 siblings, 1 reply; 25+ messages in thread
From: Jeff Law @ 2014-12-08 21:14 UTC (permalink / raw)
To: John David Anglin, GCC Patches
On 12/01/14 11:44, John David Anglin wrote:
>>
>> 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;
> Will test but I, if I read the code correctly, setting
> insn_info->frame_read = true results in frame related stores being
> killed in a constant call. This doesn't seem like the right
> solution.
But isn't killing in this context referring to GEN/KILL types of
operations for global dataflow? ISTM that GEN is a store operation
while KILL is a load operation (over-simplification, but stick with me).
Thus a GEN-GEN will get optimized into a single GEN (dead store
eliminated) while a GEN-KILL-GEN can not be optimized by DSE because of
the KILL.
It should always be safe to have an extraneous KILL since that merely
inhibits optimization. While an extraneous GEN can be a disaster.
So with setting frame_read, we're going to have more KILLs in the
dataflow sets, which results in fewer stores being eliminated. They
come from:
/* If the frame is read, the frame related stores are
killed. */
else if (insn_info->frame_read)
{
store_info_t store_info = i_ptr->store_rec;
/* Skip the clobbers. */
while (!store_info->is_set)
store_info = store_info->next;
if (store_info->group_id >= 0
&&
rtx_group_vec[store_info->group_id]->frame_related)
remove_store = true;
}
if (remove_store)
{
if (dump_file && (dump_flags & TDF_DETAILS))
dump_insn_info ("removing from active", i_ptr);
active_local_stores_len--;
if (last)
last->next_local_store = i_ptr->next_local_store;
else
active_local_stores = i_ptr->next_local_store;
}
else
last = i_ptr;
i_ptr = i_ptr->next_local_store;
AFAICT in this loop, setting FRAME_READ causes us to set REMOVE_STORE
more often. REMOVE_STORE in this context seems to indicate that we're
going to remove a store from the list we are tracking for potential
removal. Which seems exactly right.
Here as well:
/* If this insn reads the frame, kill all the frame related stores. */
if (insn_info->frame_read)
{
FOR_EACH_VEC_ELT (rtx_group_vec, i, group)
if (group->process_globally && group->frame_related)
{
if (kill)
bitmap_ior_into (kill, group->group_kill);
bitmap_and_compl_into (gen, group->group_kill);
}
}
Which also seems like exactly what we want since when we set FRAME_READ
we end up with a bigger KILL set and a smaller GEN set.
I think the terminology and variable names certainly makes this tougher
to follow than it should.
>
> Here we have frame related calls being killed before reload because
> the argument stores for the sibcall are off frame:
>
> /* Set the gen set of the exit block, and also any block with no
> successors that does not have a wild read. */
>
> static void dse_step3_exit_block_scan (bb_info_t bb_info) { /* The
> gen set is all 0's for the exit block except for the
> frame_pointer_group. */
>
> if (stores_off_frame_dead_at_return) { unsigned int i; group_info_t
> group;
>
> FOR_EACH_VEC_ELT (rtx_group_vec, i, group) { if
> (group->process_globally && group->frame_related) bitmap_ior_into
> (bb_info->gen, group->group_kill); } } }
I see your point. Expanding the GEN set here seems wrong. If we go
with setting STORES_OFF_FRAME_DEAD_AT_RETURN, then we definitely need to
update its documentation.
I think an argument could be made that we want both changes if I've
interpreted this code correctly.
Jeff
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call
2014-12-04 15:50 ` John David Anglin
@ 2014-12-08 21:20 ` Jeff Law
2014-12-08 22:14 ` John David Anglin
0 siblings, 1 reply; 25+ messages in thread
From: Jeff Law @ 2014-12-08 21:20 UTC (permalink / raw)
To: John David Anglin, GCC Patches
On 12/04/14 08:50, John David Anglin wrote:
> On 12/1/2014 11:57 AM, Jeff Law wrote:
>> 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.
> The above is wrong for sibcalls. Sibcall arguments are relative to
> the incoming argument pointer. Is this always the frame pointer?
I don't think it's always the frame pointer. Don't we use an argument
pointer for the PA64 runtime? If I recall, it was the only port that
had a non-eliminable argument pointer at the time.
> If that is the case, then it may be possible to eliminate stack based
> stores when we have a sibcall before reload.
>> if (reload_completed || SIBLING_CALL_P (insn))
>> insn_info->frame_read = true;
> This works.
>
> Dave
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call
2014-12-08 21:14 ` Jeff Law
@ 2014-12-08 21:49 ` John David Anglin
2014-12-09 20:00 ` Jeff Law
0 siblings, 1 reply; 25+ messages in thread
From: John David Anglin @ 2014-12-08 21:49 UTC (permalink / raw)
To: Jeff Law, GCC Patches
On 12/8/2014 3:49 PM, Jeff Law wrote:
> I think the terminology and variable names certainly makes this
> tougher to follow than it should.
I certainly agree. When I first looked at the code, I thought it
was completely backwards.
Dave
--
John David Anglin dave.anglin@bell.net
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call
2014-12-08 21:20 ` Jeff Law
@ 2014-12-08 22:14 ` John David Anglin
2014-12-08 22:45 ` Jeff Law
0 siblings, 1 reply; 25+ messages in thread
From: John David Anglin @ 2014-12-08 22:14 UTC (permalink / raw)
To: Jeff Law, GCC Patches
On 12/8/2014 3:01 PM, Jeff Law wrote:
>> The above is wrong for sibcalls. Sibcall arguments are relative to
>> the incoming argument pointer. Is this always the frame pointer?
> I don't think it's always the frame pointer. Don't we use an argument
> pointer for the PA64 runtime? If I recall, it was the only port that
> had a non-eliminable argument pointer at the time.
I don't think PA64 is an argument against this as sibcalls don't
work in the PA64 runtime (they are disabled in pa.c) because the
argument pointer isn't a
fixed register. I guess in theory it could be fixed if it was saved and
restored across calls.
DSE as it stands doesn't look at argument pointer based stores and I
suspect they would
be deleted with current code.
Dave
--
John David Anglin dave.anglin@bell.net
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call
2014-12-08 22:14 ` John David Anglin
@ 2014-12-08 22:45 ` Jeff Law
2014-12-17 2:03 ` [PATCH] Treat a sibling call as though it does a wild read John David Anglin
0 siblings, 1 reply; 25+ messages in thread
From: Jeff Law @ 2014-12-08 22:45 UTC (permalink / raw)
To: John David Anglin, GCC Patches
On 12/08/14 15:15, John David Anglin wrote:
> On 12/8/2014 3:01 PM, Jeff Law wrote:
>>> The above is wrong for sibcalls. Sibcall arguments are relative
>>> to the incoming argument pointer. Is this always the frame
>>> pointer?
>> I don't think it's always the frame pointer. Don't we use an
>> argument pointer for the PA64 runtime? If I recall, it was the
>> only port that had a non-eliminable argument pointer at the time.
> I don't think PA64 is an argument against this as sibcalls don't work
> in the PA64 runtime (they are disabled in pa.c) because the argument
> pointer isn't a fixed register. I guess in theory it could be fixed
> if it was saved and restored across calls.
But there's nothing that says another port in the future won't have
similar characteristics as the PA, so while the PA isn't particularly
important, it shows there's cases where arguments won't be accessed by
the FP.
>
> DSE as it stands doesn't look at argument pointer based stores and I
> suspect they would be deleted with current code.
Agreed.
jeff
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call
2014-12-08 21:49 ` John David Anglin
@ 2014-12-09 20:00 ` Jeff Law
2014-12-09 21:08 ` John David Anglin
0 siblings, 1 reply; 25+ messages in thread
From: Jeff Law @ 2014-12-09 20:00 UTC (permalink / raw)
To: John David Anglin, GCC Patches
On 12/08/14 14:50, John David Anglin wrote:
> On 12/8/2014 3:49 PM, Jeff Law wrote:
>> I think the terminology and variable names certainly makes this
>> tougher to follow than it should.
> I certainly agree. When I first looked at the code, I thought it
> was completely backwards.
Me too.
Thoughts on using both patches to ensure the we don't have extraneous
entries in the GEN set and that we're adding more stuff to the KILL set
for sibcalls?
Jeff
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call
2014-12-09 20:00 ` Jeff Law
@ 2014-12-09 21:08 ` John David Anglin
2014-12-22 17:00 ` Jeff Law
0 siblings, 1 reply; 25+ messages in thread
From: John David Anglin @ 2014-12-09 21:08 UTC (permalink / raw)
To: Jeff Law, GCC Patches
On 12/9/2014 3:00 PM, Jeff Law wrote:
> Thoughts on using both patches to ensure the we don't have extraneous
> entries in the GEN set and that we're adding more stuff to the KILL
> set for sibcalls?
Maybe we should add all stores to the KILL set
for a sibling call and not worry about whether
they are frame related or not.
Dave
--
John David Anglin dave.anglin@bell.net
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] Treat a sibling call as though it does a wild read
2014-12-08 22:45 ` Jeff Law
@ 2014-12-17 2:03 ` John David Anglin
2014-12-19 23:52 ` John David Anglin
2014-12-23 17:49 ` H.J. Lu
0 siblings, 2 replies; 25+ messages in thread
From: John David Anglin @ 2014-12-17 2:03 UTC (permalink / raw)
To: Jeff Law; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 1533 bytes --]
On 8-Dec-14, at 5:36 PM, Jeff Law wrote:
> On 12/08/14 15:15, John David Anglin wrote:
>> On 12/8/2014 3:01 PM, Jeff Law wrote:
>>>> The above is wrong for sibcalls. Sibcall arguments are relative
>>>> to the incoming argument pointer. Is this always the frame
>>>> pointer?
>>> I don't think it's always the frame pointer. Don't we use an
>>> argument pointer for the PA64 runtime? If I recall, it was the
>>> only port that had a non-eliminable argument pointer at the time.
>> I don't think PA64 is an argument against this as sibcalls don't work
>> in the PA64 runtime (they are disabled in pa.c) because the argument
>> pointer isn't a fixed register. I guess in theory it could be fixed
>> if it was saved and restored across calls.
> But there's nothing that says another port in the future won't have
> similar characteristics as the PA, so while the PA isn't
> particularly important, it shows there's cases where arguments won't
> be accessed by the FP.
>
>
>>
>> DSE as it stands doesn't look at argument pointer based stores and I
>> suspect they would be deleted with current code.
> Agreed.
I believe that this version addresses the above issues. While there
may be some opportunity to
optimize the handling of sibling call arguments, I think it is more
important to get the overall logic
correct. Also, it's obviously a rare situation for the arguments to
be pushed to the stack.
Tested on hppa-unknown-linux-gnu and hppa64-hp-hpux11.11.
Okay?
Dave
--
John David Anglin dave.anglin@bell.net
[-- Attachment #2: dse.c.d.4.txt --]
[-- Type: text/plain, Size: 924 bytes --]
2014-12-16 John David Anglin <danglin@gcc.gnu.org>
PR target/55023
* dse.c (scan_insn): Treat sibling call as though it does a wild read.
Index: dse.c
===================================================================
--- dse.c (revision 218616)
+++ dse.c (working copy)
@@ -2483,6 +2483,17 @@
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Treat a sibling call as though it does a wild read
2014-12-17 2:03 ` [PATCH] Treat a sibling call as though it does a wild read John David Anglin
@ 2014-12-19 23:52 ` John David Anglin
2014-12-22 17:03 ` Jeff Law
2014-12-23 17:49 ` H.J. Lu
1 sibling, 1 reply; 25+ messages in thread
From: John David Anglin @ 2014-12-19 23:52 UTC (permalink / raw)
To: John David Anglin; +Cc: Jeff Law, GCC Patches
[-- Attachment #1: Type: text/plain, Size: 1699 bytes --]
On 16-Dec-14, at 8:17 PM, John David Anglin wrote:
> On 8-Dec-14, at 5:36 PM, Jeff Law wrote:
>
>> On 12/08/14 15:15, John David Anglin wrote:
>>> On 12/8/2014 3:01 PM, Jeff Law wrote:
>>>>> The above is wrong for sibcalls. Sibcall arguments are relative
>>>>> to the incoming argument pointer. Is this always the frame
>>>>> pointer?
>>>> I don't think it's always the frame pointer. Don't we use an
>>>> argument pointer for the PA64 runtime? If I recall, it was the
>>>> only port that had a non-eliminable argument pointer at the time.
>>> I don't think PA64 is an argument against this as sibcalls don't
>>> work
>>> in the PA64 runtime (they are disabled in pa.c) because the argument
>>> pointer isn't a fixed register. I guess in theory it could be fixed
>>> if it was saved and restored across calls.
>> But there's nothing that says another port in the future won't have
>> similar characteristics as the PA, so while the PA isn't
>> particularly important, it shows there's cases where arguments
>> won't be accessed by the FP.
>>
>>
>>>
>>> DSE as it stands doesn't look at argument pointer based stores and I
>>> suspect they would be deleted with current code.
>> Agreed.
>
>
> I believe that this version addresses the above issues. While there
> may be some opportunity to
> optimize the handling of sibling call arguments, I think it is more
> important to get the overall logic
> correct. Also, it's obviously a rare situation for the arguments to
> be pushed to the stack.
>
> Tested on hppa-unknown-linux-gnu and hppa64-hp-hpux11.11.
>
> Okay?
Sorry, forgot to append testcase and ChangeLog entry for it.
Dave
--
John David Anglin dave.anglin@bell.net
[-- Attachment #2: dse.c.d.4.txt --]
[-- Type: text/plain, Size: 1668 bytes --]
2014-12-16 John David Anglin <danglin@gcc.gnu.org>
PR target/55023
* dse.c (scan_insn): Treat sibling call as though it does a wild read.
* gcc.dg/pr55023.c: New file.
Index: dse.c
===================================================================
--- dse.c (revision 218616)
+++ dse.c (working copy)
@@ -2483,6 +2483,17 @@
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.
--- /dev/null 2014-11-28 19:44:51.440000000 -0500
+++ gcc/testsuite/gcc.dg/pr55023.c 2014-12-04 19:49:45.887701600 -0500
@@ -0,0 +1,33 @@
+/* PR rtl-optimization/55023 */
+/* { dg-do run } */
+/* { dg-options "-O2 -fno-inline" } */
+
+extern void abort (void);
+typedef long long int64_t;
+
+struct foo {
+ int x;
+ int y;
+};
+
+int64_t foo(int64_t a, int64_t b, int64_t c)
+{
+ return a + b + c;
+}
+
+int64_t bar(int64_t a, struct foo bq, struct foo cq)
+{
+ int64_t b = bq.x + bq.y;
+ int64_t c = cq.x + cq.y;
+ return foo(a, b, c);
+}
+
+int main(void)
+{
+ int64_t a = 1;
+ struct foo b = { 2, 3 };
+ struct foo c = { 4, 5 };
+ if (bar (a, b, c) != 15)
+ abort ();
+ return 0;
+}
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call
2014-12-09 21:08 ` John David Anglin
@ 2014-12-22 17:00 ` Jeff Law
0 siblings, 0 replies; 25+ messages in thread
From: Jeff Law @ 2014-12-22 17:00 UTC (permalink / raw)
To: John David Anglin, GCC Patches
On 12/09/14 14:09, John David Anglin wrote:
> On 12/9/2014 3:00 PM, Jeff Law wrote:
>> Thoughts on using both patches to ensure the we don't have extraneous
>> entries in the GEN set and that we're adding more stuff to the KILL
>> set for sibcalls?
> Maybe we should add all stores to the KILL set
> for a sibling call and not worry about whether
> they are frame related or not.
For a sibling call, that may not be a terrible idea. We're not really
able to track the frame/arg pointer stuff well in that case and once
those are off the table, there's not much benefit in tracking stuff for
sibcalls.
Jeff
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Treat a sibling call as though it does a wild read
2014-12-19 23:52 ` John David Anglin
@ 2014-12-22 17:03 ` Jeff Law
0 siblings, 0 replies; 25+ messages in thread
From: Jeff Law @ 2014-12-22 17:03 UTC (permalink / raw)
To: John David Anglin; +Cc: GCC Patches
On 12/19/14 16:45, John David Anglin wrote:
>> I believe that this version addresses the above issues. While there
>> may be some opportunity to
>> optimize the handling of sibling call arguments, I think it is more
>> important to get the overall logic
>> correct. Also, it's obviously a rare situation for the arguments to
>> be pushed to the stack.
>>
>> Tested on hppa-unknown-linux-gnu and hppa64-hp-hpux11.11.
>>
>> Okay?
>
>
> Sorry, forgot to append testcase and ChangeLog entry for it.
No worries, I was sick last week and didn't do any patch review as a
result.
>
> Dave
> --
> John David Anglin dave.anglin@bell.net
>
>
>
> dse.c.d.4.txt
>
>
> 2014-12-16 John David Anglin<danglin@gcc.gnu.org>
>
> PR target/55023
> * dse.c (scan_insn): Treat sibling call as though it does a wild read.
>
> * gcc.dg/pr55023.c: New file.
OK.
jeff
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Treat a sibling call as though it does a wild read
2014-12-17 2:03 ` [PATCH] Treat a sibling call as though it does a wild read John David Anglin
2014-12-19 23:52 ` John David Anglin
@ 2014-12-23 17:49 ` H.J. Lu
2014-12-23 22:37 ` John David Anglin
1 sibling, 1 reply; 25+ messages in thread
From: H.J. Lu @ 2014-12-23 17:49 UTC (permalink / raw)
To: John David Anglin; +Cc: Jeff Law, GCC Patches
On Tue, Dec 16, 2014 at 5:17 PM, John David Anglin <dave.anglin@bell.net> wrote:
> On 8-Dec-14, at 5:36 PM, Jeff Law wrote:
>
>> On 12/08/14 15:15, John David Anglin wrote:
>>>
>>> On 12/8/2014 3:01 PM, Jeff Law wrote:
>>>>>
>>>>> The above is wrong for sibcalls. Sibcall arguments are relative
>>>>> to the incoming argument pointer. Is this always the frame
>>>>> pointer?
>>>>
>>>> I don't think it's always the frame pointer. Don't we use an
>>>> argument pointer for the PA64 runtime? If I recall, it was the
>>>> only port that had a non-eliminable argument pointer at the time.
>>>
>>> I don't think PA64 is an argument against this as sibcalls don't work
>>> in the PA64 runtime (they are disabled in pa.c) because the argument
>>> pointer isn't a fixed register. I guess in theory it could be fixed
>>> if it was saved and restored across calls.
>>
>> But there's nothing that says another port in the future won't have
>> similar characteristics as the PA, so while the PA isn't particularly
>> important, it shows there's cases where arguments won't be accessed by the
>> FP.
>>
>>
>>>
>>> DSE as it stands doesn't look at argument pointer based stores and I
>>> suspect they would be deleted with current code.
>>
>> Agreed.
>
>
>
> I believe that this version addresses the above issues. While there may be
> some opportunity to
> optimize the handling of sibling call arguments, I think it is more
> important to get the overall logic
> correct. Also, it's obviously a rare situation for the arguments to be
> pushed to the stack.
>
> Tested on hppa-unknown-linux-gnu and hppa64-hp-hpux11.11.
>
This caused:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64388
--
H.J.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Treat a sibling call as though it does a wild read
2014-12-23 17:49 ` H.J. Lu
@ 2014-12-23 22:37 ` John David Anglin
2014-12-23 23:21 ` H.J. Lu
0 siblings, 1 reply; 25+ messages in thread
From: John David Anglin @ 2014-12-23 22:37 UTC (permalink / raw)
To: H.J. Lu; +Cc: Jeff Law, GCC Patches
On 2014-12-23 12:32 PM, H.J. Lu wrote:
> On Tue, Dec 16, 2014 at 5:17 PM, John David Anglin <dave.anglin@bell.net> wrote:
>> On 8-Dec-14, at 5:36 PM, Jeff Law wrote:
>>
>>> On 12/08/14 15:15, John David Anglin wrote:
>>>> On 12/8/2014 3:01 PM, Jeff Law wrote:
>>>>>> The above is wrong for sibcalls. Sibcall arguments are relative
>>>>>> to the incoming argument pointer. Is this always the frame
>>>>>> pointer?
>>>>> I don't think it's always the frame pointer. Don't we use an
>>>>> argument pointer for the PA64 runtime? If I recall, it was the
>>>>> only port that had a non-eliminable argument pointer at the time.
>>>> I don't think PA64 is an argument against this as sibcalls don't work
>>>> in the PA64 runtime (they are disabled in pa.c) because the argument
>>>> pointer isn't a fixed register. I guess in theory it could be fixed
>>>> if it was saved and restored across calls.
>>> But there's nothing that says another port in the future won't have
>>> similar characteristics as the PA, so while the PA isn't particularly
>>> important, it shows there's cases where arguments won't be accessed by the
>>> FP.
>>>
>>>
>>>> DSE as it stands doesn't look at argument pointer based stores and I
>>>> suspect they would be deleted with current code.
>>> Agreed.
>>
>>
>> I believe that this version addresses the above issues. While there may be
>> some opportunity to
>> optimize the handling of sibling call arguments, I think it is more
>> important to get the overall logic
>> correct. Also, it's obviously a rare situation for the arguments to be
>> pushed to the stack.
>>
>> Tested on hppa-unknown-linux-gnu and hppa64-hp-hpux11.11.
>>
> This caused:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64388
>
I tend to think the dse behavior expected by the testcase is wrong and
this only worked
before because the arguments for the call to bar are passed in registers.
Dave
--
John David Anglin dave.anglin@bell.net
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Treat a sibling call as though it does a wild read
2014-12-23 22:37 ` John David Anglin
@ 2014-12-23 23:21 ` H.J. Lu
2014-12-24 0:28 ` John David Anglin
0 siblings, 1 reply; 25+ messages in thread
From: H.J. Lu @ 2014-12-23 23:21 UTC (permalink / raw)
To: John David Anglin; +Cc: Jeff Law, GCC Patches
On Tue, Dec 23, 2014 at 2:24 PM, John David Anglin <dave.anglin@bell.net> wrote:
> On 2014-12-23 12:32 PM, H.J. Lu wrote:
>>
>> On Tue, Dec 16, 2014 at 5:17 PM, John David Anglin <dave.anglin@bell.net>
>> wrote:
>>>
>>> On 8-Dec-14, at 5:36 PM, Jeff Law wrote:
>>>
>>>> On 12/08/14 15:15, John David Anglin wrote:
>>>>>
>>>>> On 12/8/2014 3:01 PM, Jeff Law wrote:
>>>>>>>
>>>>>>> The above is wrong for sibcalls. Sibcall arguments are relative
>>>>>>> to the incoming argument pointer. Is this always the frame
>>>>>>> pointer?
>>>>>>
>>>>>> I don't think it's always the frame pointer. Don't we use an
>>>>>> argument pointer for the PA64 runtime? If I recall, it was the
>>>>>> only port that had a non-eliminable argument pointer at the time.
>>>>>
>>>>> I don't think PA64 is an argument against this as sibcalls don't work
>>>>> in the PA64 runtime (they are disabled in pa.c) because the argument
>>>>> pointer isn't a fixed register. I guess in theory it could be fixed
>>>>> if it was saved and restored across calls.
>>>>
>>>> But there's nothing that says another port in the future won't have
>>>> similar characteristics as the PA, so while the PA isn't particularly
>>>> important, it shows there's cases where arguments won't be accessed by
>>>> the
>>>> FP.
>>>>
>>>>
>>>>> DSE as it stands doesn't look at argument pointer based stores and I
>>>>> suspect they would be deleted with current code.
>>>>
>>>> Agreed.
>>>
>>>
>>>
>>> I believe that this version addresses the above issues. While there may
>>> be
>>> some opportunity to
>>> optimize the handling of sibling call arguments, I think it is more
>>> important to get the overall logic
>>> correct. Also, it's obviously a rare situation for the arguments to be
>>> pushed to the stack.
>>>
>>> Tested on hppa-unknown-linux-gnu and hppa64-hp-hpux11.11.
>>>
>> This caused:
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64388
>>
> I tend to think the dse behavior expected by the testcase is wrong and this
> only worked
> before because the arguments for the call to bar are passed in registers.
>
>
The testcase has
/* { dg-do compile { target { { { { { { { { i?86-*-* x86_64-*-* } && x32 } || lp
64 } && { ! s390*-*-* } } && { ! hppa*64*-*-* } } && { ! alpha*-*-* } } && { { !
powerpc*-*-linux* } || powerpc_elfv2 } && { ! nvptx-*-* } } } } } */
In this case, arguments are passed in registers. Isn't the optimization
disabled for ia32, which passes arguments on stack, even before your
change?
--
H.J.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Treat a sibling call as though it does a wild read
2014-12-23 23:21 ` H.J. Lu
@ 2014-12-24 0:28 ` John David Anglin
2014-12-24 1:09 ` H.J. Lu
0 siblings, 1 reply; 25+ messages in thread
From: John David Anglin @ 2014-12-24 0:28 UTC (permalink / raw)
To: H.J. Lu; +Cc: Jeff Law, GCC Patches
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).
Dave
--
John David Anglin dave.anglin@bell.net
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Treat a sibling call as though it does a wild read
2014-12-24 0:28 ` John David Anglin
@ 2014-12-24 1:09 ` H.J. Lu
2014-12-24 2:48 ` John David Anglin
0 siblings, 1 reply; 25+ messages in thread
From: H.J. Lu @ 2014-12-24 1:09 UTC (permalink / raw)
To: John David Anglin; +Cc: Jeff Law, GCC Patches
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.
Thanks.
--
H.J.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Treat a sibling call as though it does a wild read
2014-12-24 1:09 ` H.J. Lu
@ 2014-12-24 2:48 ` John David Anglin
2014-12-24 3:00 ` H.J. Lu
0 siblings, 1 reply; 25+ messages in thread
From: John David Anglin @ 2014-12-24 2:48 UTC (permalink / raw)
To: H.J. Lu; +Cc: Jeff Law, GCC Patches
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.
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.
Dave
--
John David Anglin dave.anglin@bell.net
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Treat a sibling call as though it does a wild read
2014-12-24 2:48 ` John David Anglin
@ 2014-12-24 3:00 ` H.J. Lu
2014-12-24 3:11 ` Jeff Law
0 siblings, 1 reply; 25+ messages in thread
From: H.J. Lu @ 2014-12-24 3:00 UTC (permalink / raw)
To: John David Anglin; +Cc: Jeff Law, GCC Patches
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Treat a sibling call as though it does a wild read
2014-12-24 3:00 ` H.J. Lu
@ 2014-12-24 3:11 ` Jeff Law
2014-12-24 3:16 ` H.J. Lu
0 siblings, 1 reply; 25+ messages in thread
From: Jeff Law @ 2014-12-24 3:11 UTC (permalink / raw)
To: H.J. Lu, John David Anglin; +Cc: GCC Patches
On 12/23/14 19:47, H.J. Lu wrote:
> 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.
Just to clarify, this is not a target problem, this is a problem that
DSE is not designed to handle certain situations WRT argument stores
that occur on targets with certain properties.
Jeff
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Treat a sibling call as though it does a wild read
2014-12-24 3:11 ` Jeff Law
@ 2014-12-24 3:16 ` H.J. Lu
0 siblings, 0 replies; 25+ messages in thread
From: H.J. Lu @ 2014-12-24 3:16 UTC (permalink / raw)
To: Jeff Law; +Cc: John David Anglin, GCC Patches
On Tue, Dec 23, 2014 at 6:59 PM, Jeff Law <law@redhat.com> wrote:
> On 12/23/14 19:47, H.J. Lu wrote:
>>
>> 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.
>
> Just to clarify, this is not a target problem, this is a problem that DSE is
> not designed to handle certain situations WRT argument stores that occur on
> targets with certain properties.
Why not limit the change to targets with those certain properties?
--
H.J.
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2014-12-24 3:11 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-29 10:26 [PATCH] Set stores_off_frame_dead_at_return to false if sibling call John David Anglin
2014-12-01 16:57 ` Jeff Law
2014-12-01 18:44 ` John David Anglin
2014-12-08 21:14 ` Jeff Law
2014-12-08 21:49 ` John David Anglin
2014-12-09 20:00 ` Jeff Law
2014-12-09 21:08 ` John David Anglin
2014-12-22 17:00 ` Jeff Law
2014-12-04 15:50 ` John David Anglin
2014-12-08 21:20 ` Jeff Law
2014-12-08 22:14 ` John David Anglin
2014-12-08 22:45 ` Jeff Law
2014-12-17 2:03 ` [PATCH] Treat a sibling call as though it does a wild read John David Anglin
2014-12-19 23:52 ` John David Anglin
2014-12-22 17:03 ` Jeff Law
2014-12-23 17:49 ` H.J. Lu
2014-12-23 22:37 ` John David Anglin
2014-12-23 23:21 ` H.J. Lu
2014-12-24 0:28 ` John David Anglin
2014-12-24 1:09 ` H.J. Lu
2014-12-24 2:48 ` John David Anglin
2014-12-24 3:00 ` H.J. Lu
2014-12-24 3:11 ` Jeff Law
2014-12-24 3:16 ` H.J. Lu
2014-12-07 19:11 ` [PATCH] Correctly handle stack arguments of sibling calls in DSE pass John David Anglin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).