[PATCH] Set stores_off_frame_dead_at_return to false if sibling call

[PATCH] Set stores_off_frame_dead_at_return to false if sibling call

[John David Anglin]

[-- 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.

Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call

[Jeff Law]

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

Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call

[John David Anglin]

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

Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call

[John David Anglin]

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

[PATCH] Correctly handle stack arguments of sibling calls in DSE pass

[John David Anglin]

[-- 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;
+}

Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call

[Jeff Law]

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

Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call

[Jeff Law]

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
>

Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call

[John David Anglin]

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

Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call

[John David Anglin]

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

Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call

[Jeff Law]

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

Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call

[Jeff Law]

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

Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call

[John David Anglin]

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

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

[John David Anglin]

[-- 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.

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

[John David Anglin]

[-- 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;
+}

Re: [PATCH] Set stores_off_frame_dead_at_return to false if sibling call

[Jeff Law]

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

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

[Jeff Law]

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

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

[H.J. Lu]

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.

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

[John David Anglin]

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

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

[H.J. Lu]

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.

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

[John David Anglin]

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

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

[H.J. Lu]

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.

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

[John David Anglin]

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

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

[H.J. Lu]

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.

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

[Jeff Law]

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

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

[H.J. Lu]

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.