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

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

[H.J. Lu]

To fix a wrong code bug on HPPA with sibcall optimization, r219037 changes
DSE to treat sibcall as though it does a wild read.  However, it causes
a regression on x86:

FAIL: gcc.dg/pr44194-1.c scan-rtl-dump dse1 "global deletions = (2|3)"
FAIL: gcc.dg/pr44194-1.c scan-rtl-dump-not final "insn[: ][^\n]*set \\(mem(?![^\n]*scratch)"

This patch adds a sibcall_wild_read_p target hook, which returns true if
a sibcall have a wild read.  It defaults to SIBLING_CALL_P.  I added
hook_bool_rtx_insn_false and define TARGET_SIBCALL_WILD_READ_P as
hook_bool_rtx_insn_false for x86.  This patch restores the old behavior
before r219037.  Tested on x86.  OK for trunk?

Thanks.

H.J.
---
	PR middle-end/64388
	* dse.c (scan_insn): Call targetm.sibcall_wild_read_p to check
	if a sibcall may have a wild read.
	* hooks.c (hook_bool_rtx_insn_false): New.
	* hooks.h (hook_bool_rtx_insn_false): Likewise.
	* targhooks.c (default_sibcall_wild_read_p): Likewise.
	* targhooks.h (default_sibcall_wild_read_p): Likewise.
	* target.def (sibcall_wild_read_p): A new hook.
	* config/i386/i386.c (TARGET_SIBCALL_WILD_READ_P): New. Defined
	to hook_bool_rtx_insn_false.
	* doc/tm.texi: Regenerated.
	* doc/tm.texi.in: Add hook for TARGET_SIBCALL_WILD_READ_P.
---
 gcc/ChangeLog          | 15 +++++++++++++++
 gcc/config/i386/i386.c |  3 +++
 gcc/doc/tm.texi        |  5 +++++
 gcc/doc/tm.texi.in     |  2 ++
 gcc/dse.c              |  7 +------
 gcc/hooks.c            |  6 ++++++
 gcc/hooks.h            |  1 +
 gcc/target.def         |  8 ++++++++
 gcc/targhooks.c        | 12 ++++++++++++
 gcc/targhooks.h        |  2 ++
 10 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 162fe26..9909391 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -51711,6 +51711,9 @@ ix86_initialize_bounds (tree var, tree lb, tree ub, tree *stmts)
 #undef TARGET_FUNCTION_OK_FOR_SIBCALL
 #define TARGET_FUNCTION_OK_FOR_SIBCALL ix86_function_ok_for_sibcall
 
+#undef TARGET_SIBCALL_WILD_READ_P
+#define TARGET_SIBCALL_WILD_READ_P hook_bool_rtx_insn_false
+
 #undef TARGET_MEMMODEL_CHECK
 #define TARGET_MEMMODEL_CHECK ix86_memmodel_check
 
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index a3fda45..1a78f05 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -2857,6 +2857,11 @@ machines with non orthogonal register usage for addressing, such
 as SH, this hook can be used to avoid excessive spilling.
 @end deftypefn
 
+@deftypefn {Target Hook} bool TARGET_SIBCALL_WILD_READ_P (rtx_insn *@var{insn})
+A target hook which returns @code{true} if a sibcall @var{insn} may
+have a wild read.
+@end deftypefn
+
 @deftypefn {Target Hook} bool TARGET_LEGITIMIZE_ADDRESS_DISPLACEMENT (rtx *@var{disp}, rtx *@var{offset}, machine_mode @var{mode})
 A target hook which returns @code{true} if *@var{disp} is
 legitimezed to valid address displacement with subtracting *@var{offset}
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index 20c0129..0358235 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -2483,6 +2483,8 @@ as below:
 
 @hook TARGET_CANNOT_SUBSTITUTE_MEM_EQUIV_P
 
+@hook TARGET_SIBCALL_WILD_READ_P
+
 @hook TARGET_LEGITIMIZE_ADDRESS_DISPLACEMENT
 
 @hook TARGET_SPILL_CLASS
diff --git a/gcc/dse.c b/gcc/dse.c
index 3a7f31c..c0e1a0c 100644
--- a/gcc/dse.c
+++ b/gcc/dse.c
@@ -2483,12 +2483,7 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn)
 
       insn_info->cannot_delete = true;
 
-      /* Arguments for a sibling call that are pushed to memory are passed
-	 using the incoming argument pointer of the current function.  These
-	 may or may not be frame related depending on the target.  Since
-	 argument pointer related stores are not currently tracked, we treat
-	 a sibling call as though it does a wild read.  */
-      if (SIBLING_CALL_P (insn))
+      if (targetm.sibcall_wild_read_p (insn))
 	{
 	  add_wild_read (bb_info);
 	  return;
diff --git a/gcc/hooks.c b/gcc/hooks.c
index f698d1d..7ac6c8a 100644
--- a/gcc/hooks.c
+++ b/gcc/hooks.c
@@ -320,6 +320,12 @@ hook_bool_rtx_insn_true (rtx_insn *insn ATTRIBUTE_UNUSED)
 }
 
 bool
+hook_bool_rtx_insn_false (rtx_insn *insn ATTRIBUTE_UNUSED)
+{
+  return false;
+}
+
+bool
 hook_bool_rtx_false (rtx a ATTRIBUTE_UNUSED)
 {
   return false;
diff --git a/gcc/hooks.h b/gcc/hooks.h
index b1b312d..988724c 100644
--- a/gcc/hooks.h
+++ b/gcc/hooks.h
@@ -54,6 +54,7 @@ extern bool hook_bool_const_tree_hwi_hwi_const_tree_true (const_tree,
 							  HOST_WIDE_INT,
 							  const_tree);
 extern bool hook_bool_rtx_insn_true (rtx_insn *);
+extern bool hook_bool_rtx_insn_false (rtx_insn *);
 extern bool hook_bool_rtx_false (rtx);
 extern bool hook_bool_rtx_insn_int_false (rtx_insn *, int);
 extern bool hook_bool_uintp_uintp_false (unsigned int *, unsigned int *);
diff --git a/gcc/target.def b/gcc/target.def
index 6258b3a..68c126a 100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -5051,6 +5051,14 @@ as SH, this hook can be used to avoid excessive spilling.",
  bool, (rtx subst),
  hook_bool_rtx_false)
 
+/* This target hook allows the backend to optimize sibcall in DSE.  */
+DEFHOOK
+(sibcall_wild_read_p,
+ "A target hook which returns @code{true} if a sibcall @var{insn} may\n\
+have a wild read.",
+ bool, (rtx_insn *insn),
+ default_sibcall_wild_read_p)
+
 /* This target hook allows the backend to legitimize base plus
    displacement addressing.  */
 DEFHOOK
diff --git a/gcc/targhooks.c b/gcc/targhooks.c
index eedcc80..45dd0bb 100644
--- a/gcc/targhooks.c
+++ b/gcc/targhooks.c
@@ -1921,4 +1921,16 @@ can_use_doloop_if_innermost (const widest_int &, const widest_int &,
   return loop_depth == 1;
 }
 
+/* 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.  */
+
+bool
+default_sibcall_wild_read_p (rtx_insn *insn)
+{
+  return SIBLING_CALL_P (insn);
+}
+
 #include "gt-targhooks.h"
diff --git a/gcc/targhooks.h b/gcc/targhooks.h
index 26e4f5f..e8f4dc0c 100644
--- a/gcc/targhooks.h
+++ b/gcc/targhooks.h
@@ -239,4 +239,6 @@ extern void default_setup_incoming_vararg_bounds (cumulative_args_t ca ATTRIBUTE
 						  tree type ATTRIBUTE_UNUSED,
 						  int *pretend_arg_size ATTRIBUTE_UNUSED,
 						  int second_time ATTRIBUTE_UNUSED);
+
+extern bool default_sibcall_wild_read_p (rtx_insn *);
 #endif /* GCC_TARGHOOKS_H */
-- 
1.9.3

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

[John David Anglin]

On Wed, 31 Dec 2014, H.J. Lu wrote:

> -      /* Arguments for a sibling call that are pushed to memory are passed
> -	 using the incoming argument pointer of the current function.  These
> -	 may or may not be frame related depending on the target.  Since
> -	 argument pointer related stores are not currently tracked, we treat
> -	 a sibling call as though it does a wild read.  */
> -      if (SIBLING_CALL_P (insn))
> +      if (targetm.sibcall_wild_read_p (insn))
>  	{
>  	  add_wild_read (bb_info);
>  	  return;

Instead of falling through to code designed to handle normal calls, it
would be better to treat them separately.  Potentially, there are other
optimizations that may be applicable.  If a sibcall doesn't read from
the frame, add_non_frame_wild_read() can be called.  This would restore
the x86 optimization.

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

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

[H.J. Lu]

On Sat, Jan 3, 2015 at 9:35 AM, John David Anglin
<dave@hiauly1.hia.nrc.ca> wrote:
> On Wed, 31 Dec 2014, H.J. Lu wrote:
>
>> -      /* Arguments for a sibling call that are pushed to memory are passed
>> -      using the incoming argument pointer of the current function.  These
>> -      may or may not be frame related depending on the target.  Since
>> -      argument pointer related stores are not currently tracked, we treat
>> -      a sibling call as though it does a wild read.  */
>> -      if (SIBLING_CALL_P (insn))
>> +      if (targetm.sibcall_wild_read_p (insn))
>>       {
>>         add_wild_read (bb_info);
>>         return;
>
> Instead of falling through to code designed to handle normal calls, it
> would be better to treat them separately.  Potentially, there are other
> optimizations that may be applicable.  If a sibcall doesn't read from
> the frame, add_non_frame_wild_read() can be called.  This would restore
> the x86 optimization.
>

That will a new optimization.  I am trying to restore the old behavior on
x86 with minimum impact in stage 3.


-- 
H.J.

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

[John David Anglin]

On 2015-01-03, at 2:48 PM, H.J. Lu wrote:

> On Sat, Jan 3, 2015 at 9:35 AM, John David Anglin
> <dave@hiauly1.hia.nrc.ca> wrote:
>> On Wed, 31 Dec 2014, H.J. Lu wrote:
>> 
>>> -      /* Arguments for a sibling call that are pushed to memory are passed
>>> -      using the incoming argument pointer of the current function.  These
>>> -      may or may not be frame related depending on the target.  Since
>>> -      argument pointer related stores are not currently tracked, we treat
>>> -      a sibling call as though it does a wild read.  */
>>> -      if (SIBLING_CALL_P (insn))
>>> +      if (targetm.sibcall_wild_read_p (insn))
>>>      {
>>>        add_wild_read (bb_info);
>>>        return;
>> 
>> Instead of falling through to code designed to handle normal calls, it
>> would be better to treat them separately.  Potentially, there are other
>> optimizations that may be applicable.  If a sibcall doesn't read from
>> the frame, add_non_frame_wild_read() can be called.  This would restore
>> the x86 optimization.
>> 
> 
> That will a new optimization.  I am trying to restore the old behavior on
> x86 with minimum impact in stage 3.


Not really.  In gcc.dg/pr44194-1.c, the sibcall was not a const function and this case
was covered by this hunk of code:

      else
        /* Every other call, including pure functions, may read any memory
           that is not relative to the frame.  */
        add_non_frame_wild_read (bb_info);

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

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

[H.J. Lu]

On Sat, Jan 3, 2015 at 12:10 PM, John David Anglin <dave.anglin@bell.net> wrote:
> On 2015-01-03, at 2:48 PM, H.J. Lu wrote:
>
>> On Sat, Jan 3, 2015 at 9:35 AM, John David Anglin
>> <dave@hiauly1.hia.nrc.ca> wrote:
>>> On Wed, 31 Dec 2014, H.J. Lu wrote:
>>>
>>>> -      /* Arguments for a sibling call that are pushed to memory are passed
>>>> -      using the incoming argument pointer of the current function.  These
>>>> -      may or may not be frame related depending on the target.  Since
>>>> -      argument pointer related stores are not currently tracked, we treat
>>>> -      a sibling call as though it does a wild read.  */
>>>> -      if (SIBLING_CALL_P (insn))
>>>> +      if (targetm.sibcall_wild_read_p (insn))
>>>>      {
>>>>        add_wild_read (bb_info);
>>>>        return;
>>>
>>> Instead of falling through to code designed to handle normal calls, it
>>> would be better to treat them separately.  Potentially, there are other
>>> optimizations that may be applicable.  If a sibcall doesn't read from
>>> the frame, add_non_frame_wild_read() can be called.  This would restore
>>> the x86 optimization.
>>>
>>
>> That will a new optimization.  I am trying to restore the old behavior on
>> x86 with minimum impact in stage 3.
>
>
> Not really.  In gcc.dg/pr44194-1.c, the sibcall was not a const function and this case
> was covered by this hunk of code:
>
>       else
>         /* Every other call, including pure functions, may read any memory
>            that is not relative to the frame.  */
>         add_non_frame_wild_read (bb_info);
>

Revision 219037 has

diff --git a/gcc/dse.c b/gcc/dse.c
index 2555bd1..3a7f31c 100644
--- a/gcc/dse.c
+++ b/gcc/dse.c
@@ -2483,6 +2483,17 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn)

       insn_info->cannot_delete = true;

+      /* Arguments for a sibling call that are pushed to memory are passed
+   using the incoming argument pointer of the current function.  These
+   may or may not be frame related depending on the target.  Since
+   argument pointer related stores are not currently tracked, we treat
+   a sibling call as though it does a wild read.  */
+      if (SIBLING_CALL_P (insn))
+  {
+    add_wild_read (bb_info);
+    return;
+  }
+
       /* Const functions cannot do anything bad i.e. read memory,
    however, they can read their parameters which may have
    been pushed onto the stack.

My patch changes it to

diff --git a/gcc/dse.c b/gcc/dse.c
index 2555bd1..c0e1a0c 100644
--- a/gcc/dse.c
+++ b/gcc/dse.c
@@ -2483,6 +2483,12 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn)

       insn_info->cannot_delete = true;

+      if (targetm.sibcall_wild_read_p (insn))
+  {
+    add_wild_read (bb_info);
+    return;
+  }
+
       /* Const functions cannot do anything bad i.e. read memory,
    however, they can read their parameters which may have
    been pushed onto the stack.

On x86, it is the same as before revision 219037 since
targetm.sibcall_wild_read_p always returns false on x86.



-- 
H.J.

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

[John David Anglin]

On 2015-01-03, at 3:18 PM, H.J. Lu wrote:

> On Sat, Jan 3, 2015 at 12:10 PM, John David Anglin <dave.anglin@bell.net> wrote:
>> On 2015-01-03, at 2:48 PM, H.J. Lu wrote:
>> 
>>> On Sat, Jan 3, 2015 at 9:35 AM, John David Anglin
>>> <dave@hiauly1.hia.nrc.ca> wrote:
>>>> On Wed, 31 Dec 2014, H.J. Lu wrote:
>>>> 
>>>>> -      /* Arguments for a sibling call that are pushed to memory are passed
>>>>> -      using the incoming argument pointer of the current function.  These
>>>>> -      may or may not be frame related depending on the target.  Since
>>>>> -      argument pointer related stores are not currently tracked, we treat
>>>>> -      a sibling call as though it does a wild read.  */
>>>>> -      if (SIBLING_CALL_P (insn))
>>>>> +      if (targetm.sibcall_wild_read_p (insn))
>>>>>     {
>>>>>       add_wild_read (bb_info);
>>>>>       return;
>>>> 
>>>> Instead of falling through to code designed to handle normal calls, it
>>>> would be better to treat them separately.  Potentially, there are other
>>>> optimizations that may be applicable.  If a sibcall doesn't read from
>>>> the frame, add_non_frame_wild_read() can be called.  This would restore
>>>> the x86 optimization.
>>>> 
>>> 
>>> That will a new optimization.  I am trying to restore the old behavior on
>>> x86 with minimum impact in stage 3.
>> 
>> 
>> Not really.  In gcc.dg/pr44194-1.c, the sibcall was not a const function and this case
>> was covered by this hunk of code:
>> 
>>      else
>>        /* Every other call, including pure functions, may read any memory
>>           that is not relative to the frame.  */
>>        add_non_frame_wild_read (bb_info);
>> 
> 
> Revision 219037 has
> 
> diff --git a/gcc/dse.c b/gcc/dse.c
> index 2555bd1..3a7f31c 100644
> --- a/gcc/dse.c
> +++ b/gcc/dse.c
> @@ -2483,6 +2483,17 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn)
> 
>       insn_info->cannot_delete = true;
> 
> +      /* Arguments for a sibling call that are pushed to memory are passed
> +   using the incoming argument pointer of the current function.  These
> +   may or may not be frame related depending on the target.  Since
> +   argument pointer related stores are not currently tracked, we treat
> +   a sibling call as though it does a wild read.  */
> +      if (SIBLING_CALL_P (insn))
> +  {
> +    add_wild_read (bb_info);
> +    return;
> +  }
> +
>       /* Const functions cannot do anything bad i.e. read memory,
>    however, they can read their parameters which may have
>    been pushed onto the stack.
> 
> My patch changes it to
> 
> diff --git a/gcc/dse.c b/gcc/dse.c
> index 2555bd1..c0e1a0c 100644
> --- a/gcc/dse.c
> +++ b/gcc/dse.c
> @@ -2483,6 +2483,12 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn)
> 
>       insn_info->cannot_delete = true;
> 
> +      if (targetm.sibcall_wild_read_p (insn))
> +  {
> +    add_wild_read (bb_info);
> +    return;
> +  }
> +
>       /* Const functions cannot do anything bad i.e. read memory,
>    however, they can read their parameters which may have
>    been pushed onto the stack.
> 
> On x86, it is the same as before revision 219037 since
> targetm.sibcall_wild_read_p always returns false on x86.


Understood.  The point is the subsequent code for const functions is based on assumptions that
are not generally true for sibcalls:

  /* This field is only used for the processing of const functions.
     These functions cannot read memory, but they can read the stack
     because that is where they may get their parms.  We need to be
     this conservative because, like the store motion pass, we don't
     consider CALL_INSN_FUNCTION_USAGE when processing call insns.
     Moreover, we need to distinguish two cases:
     1. Before reload (register elimination), the stores related to
        outgoing arguments are stack pointer based and thus deemed
        of non-constant base in this pass.  This requires special
        handling but also means that the frame pointer based stores
        need not be killed upon encountering a const function call.
     2. After reload, the stores related to outgoing arguments can be
        either stack pointer or hard frame pointer based.  This means
        that we have no other choice than also killing all the frame
        pointer based stores upon encountering a const function call.
     This field is set after reload for const function calls.  Having
     this set is less severe than a wild read, it just means that all
     the frame related stores are killed rather than all the stores.  */
  bool frame_read;

For example, the stores related to sibcall arguments are not in general stack pointer based.  This
suggests to me that we don't have to always kill stack pointer based stores in the const sibcall case
and they can be optimized.

For me, keeping the sibcall handling separate from normal calls is easier to understand and
potentially provides a means to optimize stack pointer based stores.  Are you sure that the prior
behaviour was always correct on x86 (e.g., more than 6 arguments)?

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

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

[H.J. Lu]

On Sat, Jan 3, 2015 at 12:58 PM, John David Anglin <dave.anglin@bell.net> wrote:
> On 2015-01-03, at 3:18 PM, H.J. Lu wrote:
>
>> On Sat, Jan 3, 2015 at 12:10 PM, John David Anglin <dave.anglin@bell.net> wrote:
>>> On 2015-01-03, at 2:48 PM, H.J. Lu wrote:
>>>
>>>> On Sat, Jan 3, 2015 at 9:35 AM, John David Anglin
>>>> <dave@hiauly1.hia.nrc.ca> wrote:
>>>>> On Wed, 31 Dec 2014, H.J. Lu wrote:
>>>>>
>>>>>> -      /* Arguments for a sibling call that are pushed to memory are passed
>>>>>> -      using the incoming argument pointer of the current function.  These
>>>>>> -      may or may not be frame related depending on the target.  Since
>>>>>> -      argument pointer related stores are not currently tracked, we treat
>>>>>> -      a sibling call as though it does a wild read.  */
>>>>>> -      if (SIBLING_CALL_P (insn))
>>>>>> +      if (targetm.sibcall_wild_read_p (insn))
>>>>>>     {
>>>>>>       add_wild_read (bb_info);
>>>>>>       return;
>>>>>
>>>>> Instead of falling through to code designed to handle normal calls, it
>>>>> would be better to treat them separately.  Potentially, there are other
>>>>> optimizations that may be applicable.  If a sibcall doesn't read from
>>>>> the frame, add_non_frame_wild_read() can be called.  This would restore
>>>>> the x86 optimization.
>>>>>
>>>>
>>>> That will a new optimization.  I am trying to restore the old behavior on
>>>> x86 with minimum impact in stage 3.
>>>
>>>
>>> Not really.  In gcc.dg/pr44194-1.c, the sibcall was not a const function and this case
>>> was covered by this hunk of code:
>>>
>>>      else
>>>        /* Every other call, including pure functions, may read any memory
>>>           that is not relative to the frame.  */
>>>        add_non_frame_wild_read (bb_info);
>>>
>>
>> Revision 219037 has
>>
>> diff --git a/gcc/dse.c b/gcc/dse.c
>> index 2555bd1..3a7f31c 100644
>> --- a/gcc/dse.c
>> +++ b/gcc/dse.c
>> @@ -2483,6 +2483,17 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn)
>>
>>       insn_info->cannot_delete = true;
>>
>> +      /* Arguments for a sibling call that are pushed to memory are passed
>> +   using the incoming argument pointer of the current function.  These
>> +   may or may not be frame related depending on the target.  Since
>> +   argument pointer related stores are not currently tracked, we treat
>> +   a sibling call as though it does a wild read.  */
>> +      if (SIBLING_CALL_P (insn))
>> +  {
>> +    add_wild_read (bb_info);
>> +    return;
>> +  }
>> +
>>       /* Const functions cannot do anything bad i.e. read memory,
>>    however, they can read their parameters which may have
>>    been pushed onto the stack.
>>
>> My patch changes it to
>>
>> diff --git a/gcc/dse.c b/gcc/dse.c
>> index 2555bd1..c0e1a0c 100644
>> --- a/gcc/dse.c
>> +++ b/gcc/dse.c
>> @@ -2483,6 +2483,12 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn)
>>
>>       insn_info->cannot_delete = true;
>>
>> +      if (targetm.sibcall_wild_read_p (insn))
>> +  {
>> +    add_wild_read (bb_info);
>> +    return;
>> +  }
>> +
>>       /* Const functions cannot do anything bad i.e. read memory,
>>    however, they can read their parameters which may have
>>    been pushed onto the stack.
>>
>> On x86, it is the same as before revision 219037 since
>> targetm.sibcall_wild_read_p always returns false on x86.
>
>
> Understood.  The point is the subsequent code for const functions is based on assumptions that
> are not generally true for sibcalls:
>
>   /* This field is only used for the processing of const functions.
>      These functions cannot read memory, but they can read the stack
>      because that is where they may get their parms.  We need to be
>      this conservative because, like the store motion pass, we don't
>      consider CALL_INSN_FUNCTION_USAGE when processing call insns.
>      Moreover, we need to distinguish two cases:
>      1. Before reload (register elimination), the stores related to
>         outgoing arguments are stack pointer based and thus deemed
>         of non-constant base in this pass.  This requires special
>         handling but also means that the frame pointer based stores
>         need not be killed upon encountering a const function call.
>      2. After reload, the stores related to outgoing arguments can be
>         either stack pointer or hard frame pointer based.  This means
>         that we have no other choice than also killing all the frame
>         pointer based stores upon encountering a const function call.
>      This field is set after reload for const function calls.  Having
>      this set is less severe than a wild read, it just means that all
>      the frame related stores are killed rather than all the stores.  */
>   bool frame_read;
>
> For example, the stores related to sibcall arguments are not in general stack pointer based.  This
> suggests to me that we don't have to always kill stack pointer based stores in the const sibcall case
> and they can be optimized.
>
> For me, keeping the sibcall handling separate from normal calls is easier to understand and
> potentially provides a means to optimize stack pointer based stores.  Are you sure that the prior
> behaviour was always correct on x86 (e.g., more than 6 arguments)?
>

I'd like to do it in 2 steps:

1. Bring x86 back to the behavior prior to revision 21903 since it won't
cause any regressions.
2. Investigate if sibcall is handled correctly on x86.


-- 
H.J.

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

[Richard Biener]

On January 3, 2015 10:48:47 PM CET, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>On Sat, Jan 3, 2015 at 12:58 PM, John David Anglin
><dave.anglin@bell.net> wrote:
>> On 2015-01-03, at 3:18 PM, H.J. Lu wrote:
>>
>>> On Sat, Jan 3, 2015 at 12:10 PM, John David Anglin
><dave.anglin@bell.net> wrote:
>>>> On 2015-01-03, at 2:48 PM, H.J. Lu wrote:
>>>>
>>>>> On Sat, Jan 3, 2015 at 9:35 AM, John David Anglin
>>>>> <dave@hiauly1.hia.nrc.ca> wrote:
>>>>>> On Wed, 31 Dec 2014, H.J. Lu wrote:
>>>>>>
>>>>>>> -      /* Arguments for a sibling call that are pushed to memory
>are passed
>>>>>>> -      using the incoming argument pointer of the current
>function.  These
>>>>>>> -      may or may not be frame related depending on the target. 
>Since
>>>>>>> -      argument pointer related stores are not currently
>tracked, we treat
>>>>>>> -      a sibling call as though it does a wild read.  */
>>>>>>> -      if (SIBLING_CALL_P (insn))
>>>>>>> +      if (targetm.sibcall_wild_read_p (insn))
>>>>>>>     {
>>>>>>>       add_wild_read (bb_info);
>>>>>>>       return;
>>>>>>
>>>>>> Instead of falling through to code designed to handle normal
>calls, it
>>>>>> would be better to treat them separately.  Potentially, there are
>other
>>>>>> optimizations that may be applicable.  If a sibcall doesn't read
>from
>>>>>> the frame, add_non_frame_wild_read() can be called.  This would
>restore
>>>>>> the x86 optimization.
>>>>>>
>>>>>
>>>>> That will a new optimization.  I am trying to restore the old
>behavior on
>>>>> x86 with minimum impact in stage 3.
>>>>
>>>>
>>>> Not really.  In gcc.dg/pr44194-1.c, the sibcall was not a const
>function and this case
>>>> was covered by this hunk of code:
>>>>
>>>>      else
>>>>        /* Every other call, including pure functions, may read any
>memory
>>>>           that is not relative to the frame.  */
>>>>        add_non_frame_wild_read (bb_info);
>>>>
>>>
>>> Revision 219037 has
>>>
>>> diff --git a/gcc/dse.c b/gcc/dse.c
>>> index 2555bd1..3a7f31c 100644
>>> --- a/gcc/dse.c
>>> +++ b/gcc/dse.c
>>> @@ -2483,6 +2483,17 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn)
>>>
>>>       insn_info->cannot_delete = true;
>>>
>>> +      /* Arguments for a sibling call that are pushed to memory are
>passed
>>> +   using the incoming argument pointer of the current function. 
>These
>>> +   may or may not be frame related depending on the target.  Since
>>> +   argument pointer related stores are not currently tracked, we
>treat
>>> +   a sibling call as though it does a wild read.  */
>>> +      if (SIBLING_CALL_P (insn))
>>> +  {
>>> +    add_wild_read (bb_info);
>>> +    return;
>>> +  }
>>> +
>>>       /* Const functions cannot do anything bad i.e. read memory,
>>>    however, they can read their parameters which may have
>>>    been pushed onto the stack.
>>>
>>> My patch changes it to
>>>
>>> diff --git a/gcc/dse.c b/gcc/dse.c
>>> index 2555bd1..c0e1a0c 100644
>>> --- a/gcc/dse.c
>>> +++ b/gcc/dse.c
>>> @@ -2483,6 +2483,12 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn)
>>>
>>>       insn_info->cannot_delete = true;
>>>
>>> +      if (targetm.sibcall_wild_read_p (insn))
>>> +  {
>>> +    add_wild_read (bb_info);
>>> +    return;
>>> +  }
>>> +
>>>       /* Const functions cannot do anything bad i.e. read memory,
>>>    however, they can read their parameters which may have
>>>    been pushed onto the stack.
>>>
>>> On x86, it is the same as before revision 219037 since
>>> targetm.sibcall_wild_read_p always returns false on x86.
>>
>>
>> Understood.  The point is the subsequent code for const functions is
>based on assumptions that
>> are not generally true for sibcalls:
>>
>>   /* This field is only used for the processing of const functions.
>>      These functions cannot read memory, but they can read the stack
>>      because that is where they may get their parms.  We need to be
>>      this conservative because, like the store motion pass, we don't
>>      consider CALL_INSN_FUNCTION_USAGE when processing call insns.
>>      Moreover, we need to distinguish two cases:
>>      1. Before reload (register elimination), the stores related to
>>         outgoing arguments are stack pointer based and thus deemed
>>         of non-constant base in this pass.  This requires special
>>         handling but also means that the frame pointer based stores
>>         need not be killed upon encountering a const function call.
>>      2. After reload, the stores related to outgoing arguments can be
>>         either stack pointer or hard frame pointer based.  This means
>>         that we have no other choice than also killing all the frame
>>         pointer based stores upon encountering a const function call.
>>      This field is set after reload for const function calls.  Having
>>      this set is less severe than a wild read, it just means that all
>>      the frame related stores are killed rather than all the stores. 
>*/
>>   bool frame_read;
>>
>> For example, the stores related to sibcall arguments are not in
>general stack pointer based.  This
>> suggests to me that we don't have to always kill stack pointer based
>stores in the const sibcall case
>> and they can be optimized.
>>
>> For me, keeping the sibcall handling separate from normal calls is
>easier to understand and
>> potentially provides a means to optimize stack pointer based stores. 
>Are you sure that the prior
>> behaviour was always correct on x86 (e.g., more than 6 arguments)?
>>
>
>I'd like to do it in 2 steps:
>
>1. Bring x86 back to the behavior prior to revision 21903 since it
>won't
>cause any regressions.
>2. Investigate if sibcall is handled correctly on x86.

But either your new hook or the original fix makes no sense.

Richard.

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

[H.J. Lu]

On Sun, Jan 4, 2015 at 3:37 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On January 3, 2015 10:48:47 PM CET, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>>On Sat, Jan 3, 2015 at 12:58 PM, John David Anglin
>><dave.anglin@bell.net> wrote:
>>> On 2015-01-03, at 3:18 PM, H.J. Lu wrote:
>>>
>>>> On Sat, Jan 3, 2015 at 12:10 PM, John David Anglin
>><dave.anglin@bell.net> wrote:
>>>>> On 2015-01-03, at 2:48 PM, H.J. Lu wrote:
>>>>>
>>>>>> On Sat, Jan 3, 2015 at 9:35 AM, John David Anglin
>>>>>> <dave@hiauly1.hia.nrc.ca> wrote:
>>>>>>> On Wed, 31 Dec 2014, H.J. Lu wrote:
>>>>>>>
>>>>>>>> -      /* Arguments for a sibling call that are pushed to memory
>>are passed
>>>>>>>> -      using the incoming argument pointer of the current
>>function.  These
>>>>>>>> -      may or may not be frame related depending on the target.
>>Since
>>>>>>>> -      argument pointer related stores are not currently
>>tracked, we treat
>>>>>>>> -      a sibling call as though it does a wild read.  */
>>>>>>>> -      if (SIBLING_CALL_P (insn))
>>>>>>>> +      if (targetm.sibcall_wild_read_p (insn))
>>>>>>>>     {
>>>>>>>>       add_wild_read (bb_info);
>>>>>>>>       return;
>>>>>>>
>>>>>>> Instead of falling through to code designed to handle normal
>>calls, it
>>>>>>> would be better to treat them separately.  Potentially, there are
>>other
>>>>>>> optimizations that may be applicable.  If a sibcall doesn't read
>>from
>>>>>>> the frame, add_non_frame_wild_read() can be called.  This would
>>restore
>>>>>>> the x86 optimization.
>>>>>>>
>>>>>>
>>>>>> That will a new optimization.  I am trying to restore the old
>>behavior on
>>>>>> x86 with minimum impact in stage 3.
>>>>>
>>>>>
>>>>> Not really.  In gcc.dg/pr44194-1.c, the sibcall was not a const
>>function and this case
>>>>> was covered by this hunk of code:
>>>>>
>>>>>      else
>>>>>        /* Every other call, including pure functions, may read any
>>memory
>>>>>           that is not relative to the frame.  */
>>>>>        add_non_frame_wild_read (bb_info);
>>>>>
>>>>
>>>> Revision 219037 has
>>>>
>>>> diff --git a/gcc/dse.c b/gcc/dse.c
>>>> index 2555bd1..3a7f31c 100644
>>>> --- a/gcc/dse.c
>>>> +++ b/gcc/dse.c
>>>> @@ -2483,6 +2483,17 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn)
>>>>
>>>>       insn_info->cannot_delete = true;
>>>>
>>>> +      /* Arguments for a sibling call that are pushed to memory are
>>passed
>>>> +   using the incoming argument pointer of the current function.
>>These
>>>> +   may or may not be frame related depending on the target.  Since
>>>> +   argument pointer related stores are not currently tracked, we
>>treat
>>>> +   a sibling call as though it does a wild read.  */
>>>> +      if (SIBLING_CALL_P (insn))
>>>> +  {
>>>> +    add_wild_read (bb_info);
>>>> +    return;
>>>> +  }
>>>> +
>>>>       /* Const functions cannot do anything bad i.e. read memory,
>>>>    however, they can read their parameters which may have
>>>>    been pushed onto the stack.
>>>>
>>>> My patch changes it to
>>>>
>>>> diff --git a/gcc/dse.c b/gcc/dse.c
>>>> index 2555bd1..c0e1a0c 100644
>>>> --- a/gcc/dse.c
>>>> +++ b/gcc/dse.c
>>>> @@ -2483,6 +2483,12 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn)
>>>>
>>>>       insn_info->cannot_delete = true;
>>>>
>>>> +      if (targetm.sibcall_wild_read_p (insn))
>>>> +  {
>>>> +    add_wild_read (bb_info);
>>>> +    return;
>>>> +  }
>>>> +
>>>>       /* Const functions cannot do anything bad i.e. read memory,
>>>>    however, they can read their parameters which may have
>>>>    been pushed onto the stack.
>>>>
>>>> On x86, it is the same as before revision 219037 since
>>>> targetm.sibcall_wild_read_p always returns false on x86.
>>>
>>>
>>> Understood.  The point is the subsequent code for const functions is
>>based on assumptions that
>>> are not generally true for sibcalls:
>>>
>>>   /* This field is only used for the processing of const functions.
>>>      These functions cannot read memory, but they can read the stack
>>>      because that is where they may get their parms.  We need to be
>>>      this conservative because, like the store motion pass, we don't
>>>      consider CALL_INSN_FUNCTION_USAGE when processing call insns.
>>>      Moreover, we need to distinguish two cases:
>>>      1. Before reload (register elimination), the stores related to
>>>         outgoing arguments are stack pointer based and thus deemed
>>>         of non-constant base in this pass.  This requires special
>>>         handling but also means that the frame pointer based stores
>>>         need not be killed upon encountering a const function call.
>>>      2. After reload, the stores related to outgoing arguments can be
>>>         either stack pointer or hard frame pointer based.  This means
>>>         that we have no other choice than also killing all the frame
>>>         pointer based stores upon encountering a const function call.
>>>      This field is set after reload for const function calls.  Having
>>>      this set is less severe than a wild read, it just means that all
>>>      the frame related stores are killed rather than all the stores.
>>*/
>>>   bool frame_read;
>>>
>>> For example, the stores related to sibcall arguments are not in
>>general stack pointer based.  This
>>> suggests to me that we don't have to always kill stack pointer based
>>stores in the const sibcall case
>>> and they can be optimized.
>>>
>>> For me, keeping the sibcall handling separate from normal calls is
>>easier to understand and
>>> potentially provides a means to optimize stack pointer based stores.
>>Are you sure that the prior
>>> behaviour was always correct on x86 (e.g., more than 6 arguments)?
>>>
>>
>>I'd like to do it in 2 steps:
>>
>>1. Bring x86 back to the behavior prior to revision 21903 since it
>>won't
>>cause any regressions.
>>2. Investigate if sibcall is handled correctly on x86.
>
> But either your new hook or the original fix makes no sense.

All I want is to restore the old behavior on x86.  If the original fix
makes no sense, should it be reverted?


-- 
H.J.

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

[Richard Biener]

On January 4, 2015 3:57:07 PM CET, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>On Sun, Jan 4, 2015 at 3:37 AM, Richard Biener
><richard.guenther@gmail.com> wrote:
>> On January 3, 2015 10:48:47 PM CET, "H.J. Lu" <hjl.tools@gmail.com>
>wrote:
>>>On Sat, Jan 3, 2015 at 12:58 PM, John David Anglin
>>><dave.anglin@bell.net> wrote:
>>>> On 2015-01-03, at 3:18 PM, H.J. Lu wrote:
>>>>
>>>>> On Sat, Jan 3, 2015 at 12:10 PM, John David Anglin
>>><dave.anglin@bell.net> wrote:
>>>>>> On 2015-01-03, at 2:48 PM, H.J. Lu wrote:
>>>>>>
>>>>>>> On Sat, Jan 3, 2015 at 9:35 AM, John David Anglin
>>>>>>> <dave@hiauly1.hia.nrc.ca> wrote:
>>>>>>>> On Wed, 31 Dec 2014, H.J. Lu wrote:
>>>>>>>>
>>>>>>>>> -      /* Arguments for a sibling call that are pushed to
>memory
>>>are passed
>>>>>>>>> -      using the incoming argument pointer of the current
>>>function.  These
>>>>>>>>> -      may or may not be frame related depending on the
>target.
>>>Since
>>>>>>>>> -      argument pointer related stores are not currently
>>>tracked, we treat
>>>>>>>>> -      a sibling call as though it does a wild read.  */
>>>>>>>>> -      if (SIBLING_CALL_P (insn))
>>>>>>>>> +      if (targetm.sibcall_wild_read_p (insn))
>>>>>>>>>     {
>>>>>>>>>       add_wild_read (bb_info);
>>>>>>>>>       return;
>>>>>>>>
>>>>>>>> Instead of falling through to code designed to handle normal
>>>calls, it
>>>>>>>> would be better to treat them separately.  Potentially, there
>are
>>>other
>>>>>>>> optimizations that may be applicable.  If a sibcall doesn't
>read
>>>from
>>>>>>>> the frame, add_non_frame_wild_read() can be called.  This would
>>>restore
>>>>>>>> the x86 optimization.
>>>>>>>>
>>>>>>>
>>>>>>> That will a new optimization.  I am trying to restore the old
>>>behavior on
>>>>>>> x86 with minimum impact in stage 3.
>>>>>>
>>>>>>
>>>>>> Not really.  In gcc.dg/pr44194-1.c, the sibcall was not a const
>>>function and this case
>>>>>> was covered by this hunk of code:
>>>>>>
>>>>>>      else
>>>>>>        /* Every other call, including pure functions, may read
>any
>>>memory
>>>>>>           that is not relative to the frame.  */
>>>>>>        add_non_frame_wild_read (bb_info);
>>>>>>
>>>>>
>>>>> Revision 219037 has
>>>>>
>>>>> diff --git a/gcc/dse.c b/gcc/dse.c
>>>>> index 2555bd1..3a7f31c 100644
>>>>> --- a/gcc/dse.c
>>>>> +++ b/gcc/dse.c
>>>>> @@ -2483,6 +2483,17 @@ scan_insn (bb_info_t bb_info, rtx_insn
>*insn)
>>>>>
>>>>>       insn_info->cannot_delete = true;
>>>>>
>>>>> +      /* Arguments for a sibling call that are pushed to memory
>are
>>>passed
>>>>> +   using the incoming argument pointer of the current function.
>>>These
>>>>> +   may or may not be frame related depending on the target. 
>Since
>>>>> +   argument pointer related stores are not currently tracked, we
>>>treat
>>>>> +   a sibling call as though it does a wild read.  */
>>>>> +      if (SIBLING_CALL_P (insn))
>>>>> +  {
>>>>> +    add_wild_read (bb_info);
>>>>> +    return;
>>>>> +  }
>>>>> +
>>>>>       /* Const functions cannot do anything bad i.e. read memory,
>>>>>    however, they can read their parameters which may have
>>>>>    been pushed onto the stack.
>>>>>
>>>>> My patch changes it to
>>>>>
>>>>> diff --git a/gcc/dse.c b/gcc/dse.c
>>>>> index 2555bd1..c0e1a0c 100644
>>>>> --- a/gcc/dse.c
>>>>> +++ b/gcc/dse.c
>>>>> @@ -2483,6 +2483,12 @@ scan_insn (bb_info_t bb_info, rtx_insn
>*insn)
>>>>>
>>>>>       insn_info->cannot_delete = true;
>>>>>
>>>>> +      if (targetm.sibcall_wild_read_p (insn))
>>>>> +  {
>>>>> +    add_wild_read (bb_info);
>>>>> +    return;
>>>>> +  }
>>>>> +
>>>>>       /* Const functions cannot do anything bad i.e. read memory,
>>>>>    however, they can read their parameters which may have
>>>>>    been pushed onto the stack.
>>>>>
>>>>> On x86, it is the same as before revision 219037 since
>>>>> targetm.sibcall_wild_read_p always returns false on x86.
>>>>
>>>>
>>>> Understood.  The point is the subsequent code for const functions
>is
>>>based on assumptions that
>>>> are not generally true for sibcalls:
>>>>
>>>>   /* This field is only used for the processing of const functions.
>>>>      These functions cannot read memory, but they can read the
>stack
>>>>      because that is where they may get their parms.  We need to be
>>>>      this conservative because, like the store motion pass, we
>don't
>>>>      consider CALL_INSN_FUNCTION_USAGE when processing call insns.
>>>>      Moreover, we need to distinguish two cases:
>>>>      1. Before reload (register elimination), the stores related to
>>>>         outgoing arguments are stack pointer based and thus deemed
>>>>         of non-constant base in this pass.  This requires special
>>>>         handling but also means that the frame pointer based stores
>>>>         need not be killed upon encountering a const function call.
>>>>      2. After reload, the stores related to outgoing arguments can
>be
>>>>         either stack pointer or hard frame pointer based.  This
>means
>>>>         that we have no other choice than also killing all the
>frame
>>>>         pointer based stores upon encountering a const function
>call.
>>>>      This field is set after reload for const function calls. 
>Having
>>>>      this set is less severe than a wild read, it just means that
>all
>>>>      the frame related stores are killed rather than all the
>stores.
>>>*/
>>>>   bool frame_read;
>>>>
>>>> For example, the stores related to sibcall arguments are not in
>>>general stack pointer based.  This
>>>> suggests to me that we don't have to always kill stack pointer
>based
>>>stores in the const sibcall case
>>>> and they can be optimized.
>>>>
>>>> For me, keeping the sibcall handling separate from normal calls is
>>>easier to understand and
>>>> potentially provides a means to optimize stack pointer based
>stores.
>>>Are you sure that the prior
>>>> behaviour was always correct on x86 (e.g., more than 6 arguments)?
>>>>
>>>
>>>I'd like to do it in 2 steps:
>>>
>>>1. Bring x86 back to the behavior prior to revision 21903 since it
>>>won't
>>>cause any regressions.
>>>2. Investigate if sibcall is handled correctly on x86.
>>
>> But either your new hook or the original fix makes no sense.
>
>All I want is to restore the old behavior on x86.  If the original fix
>makes no sense, should it be reverted?

I think the original fix is too conservative
But I also think there is a target independent bug, thus a target hook to "fix" things for x86 makes no sense.

Richard.

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

[Jeff Law]

On 01/04/15 10:16, Richard Biener wrote:
>>>
>>> But either your new hook or the original fix makes no sense.
>>
>> All I want is to restore the old behavior on x86.  If the original fix
>> makes no sense, should it be reverted?
>
> I think the original fix is too conservative
Perhaps.  Neither John nor I felt it was too conservative.  But 
disagreeing slightly on this isn't a big deal.

> But I also think there is a target independent bug, thus a target hook to "fix" things for x86 makes no sense.
Agreed 100%.

Jeff

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

[Jakub Jelinek]

On Mon, Jan 05, 2015 at 07:19:33AM -0700, Jeff Law wrote:
> On 01/04/15 10:16, Richard Biener wrote:
> >>>
> >>>But either your new hook or the original fix makes no sense.
> >>
> >>All I want is to restore the old behavior on x86.  If the original fix
> >>makes no sense, should it be reverted?
> >
> >I think the original fix is too conservative
> Perhaps.  Neither John nor I felt it was too conservative.  But disagreeing
> slightly on this isn't a big deal.
> 
> >But I also think there is a target independent bug, thus a target hook to "fix" things for x86 makes no sense.
> Agreed 100%.

So isn't the right replacement for the target hook H.J. wanted to add
the HARD_FRAME_POINTER_IS_ARG_POINTER macro?
If argp is not hfp, then the stores through argp are not considered
to be based on the frame pointer, if it is the same thing, such as on PA,
then indeed sibcall stack argument stores are frame related.
You could also add a !reload_completed to that, after RA frame_read is
how this is handled.

      /* Arguments for a sibling call that are pushed to memory are passed
         using the incoming argument pointer of the current function.  If
	 argument pointer is the hard frame pointer, then treat sibling
	 calls as wild reads before reload; after reload frame_read flag
	 should take care of this.  */
      if (HARD_FRAME_POINTER_IS_ARG_POINTER
	  && SIBLING_CALL_P (insn)
	  && !reload_completed)
        {
          add_wild_read (bb_info);
          return;
        }

?

	Jakub

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

[John David Anglin]

On 1/5/2015 1:51 PM, Jakub Jelinek wrote:
> So isn't the right replacement for the target hook H.J. wanted to add
> the HARD_FRAME_POINTER_IS_ARG_POINTER macro?
> If argp is not hfp, then the stores through argp are not considered
> to be based on the frame pointer, if it is the same thing, such as on PA,
> then indeed sibcall stack argument stores are frame related.
> You could also add a !reload_completed to that, after RA frame_read is
> how this is handled.
>
>        /* Arguments for a sibling call that are pushed to memory are passed
>           using the incoming argument pointer of the current function.  If
> 	 argument pointer is the hard frame pointer, then treat sibling
> 	 calls as wild reads before reload; after reload frame_read flag
> 	 should take care of this.  */
>        if (HARD_FRAME_POINTER_IS_ARG_POINTER
> 	  && SIBLING_CALL_P (insn)
> 	  && !reload_completed)
>          {
>            add_wild_read (bb_info);
>            return;
>          }
>
> ?
I think there may be one situation after reload that's not handled
by the above.  frame_read is only used for const calls.  What about
the situation where we have a non const sibcall and the frame pointer
isn't eliminated?

Dave

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

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

[Jakub Jelinek]

On Mon, Jan 05, 2015 at 03:16:46PM -0500, John David Anglin wrote:
> I think there may be one situation after reload that's not handled
> by the above.  frame_read is only used for const calls.  What about
> the situation where we have a non const sibcall and the frame pointer
> isn't eliminated?

After reload DSE is run after threading prologues/epilogues, end
the prologue/epilogue sequences usually contain wild reads, e.g.
(mem:BLK (scratch)) in some insn etc.
Do you have some particular testcase in mind?

That said, DSE after reload is much more limited than the DSE before reload,
so is less important, so perhaps even
  if ((HARD_FRAME_POINTER_IS_ARG_POINTER || reload_completed)
      && SIBLING_CALL_P (insn))
    {
      add_wild_read (bb_info);                                                                                                                 
      return;                                                                                                                                  
    }
might be good enough.

	Jakub

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

[Jakub Jelinek]

On Mon, Jan 05, 2015 at 10:23:57PM +0100, Jakub Jelinek wrote:
> On Mon, Jan 05, 2015 at 03:16:46PM -0500, John David Anglin wrote:
> > I think there may be one situation after reload that's not handled
> > by the above.  frame_read is only used for const calls.  What about
> > the situation where we have a non const sibcall and the frame pointer
> > isn't eliminated?
> 
> After reload DSE is run after threading prologues/epilogues, end
> the prologue/epilogue sequences usually contain wild reads, e.g.
> (mem:BLK (scratch)) in some insn etc.
> Do you have some particular testcase in mind?
> 
> That said, DSE after reload is much more limited than the DSE before reload,
> so is less important, so perhaps even
>   if ((HARD_FRAME_POINTER_IS_ARG_POINTER || reload_completed)
>       && SIBLING_CALL_P (insn))
>     {
>       add_wild_read (bb_info);                                                                                                                 
>       return;                                                                                                                                  
>     }
> might be good enough.

Or you could e.g. do the
  if (HARD_FRAME_POINTER_IS_ARG_POINTER
      && !reload_completed
      && SIBLING_CALL_P (insn))
    { add_wild_read (bb_info); return; }
case first, then compute const_call and memset_call,
  if (const_call || memset_call)
    { current_big_block }
  else if (reload_completed && SIBLING_CALL_P (insn))
    add_wild_read (bb_info);
  else
    add_non_frame_wild_read (bb_info);

That way, you would not punish const/memset calls unnecessarily after reload
when they are sibling calls.

	Jakub

[PATCH] Fix up DSE - PR middle-end/64388, target/55023

[Jakub Jelinek]

Hi!

On Mon, Jan 05, 2015 at 10:31:17PM +0100, Jakub Jelinek wrote:
> Or you could e.g. do the
>   if (HARD_FRAME_POINTER_IS_ARG_POINTER
>       && !reload_completed
>       && SIBLING_CALL_P (insn))
>     { add_wild_read (bb_info); return; }
> case first, then compute const_call and memset_call,
>   if (const_call || memset_call)
>     { current_big_block }
>   else if (reload_completed && SIBLING_CALL_P (insn))
>     add_wild_read (bb_info);
>   else
>     add_non_frame_wild_read (bb_info);
> 
> That way, you would not punish const/memset calls unnecessarily after reload
> when they are sibling calls.

So what about this way?  Even for the HARD_FRAME_POINTER_IS_ARG_POINTER
before reload case, there is no reason for wild read IMHO, just setting
frame_read should do all that is necessary.  Bootstrapped/regtested on
x86_64-linux and i686-linux, tested on the pr55023 testcase with cross
to hppa-linux.  Ok for trunk?

2015-01-06  Jakub Jelinek  <jakub@redhat.com>

	PR target/55023
	PR middle-end/64388
	* dse.c (struct insn_info): Mention frame_read set also
	before reload for tail calls on some targets.
	(scan_insn): Revert 2014-12-22 change.  Set frame_read
	also before reload for tail calls if
	HARD_FRAME_POINTER_IS_ARG_POINTER.  Call add_wild_read
	instead of add_non_frame_wild_read for non-const/memset
	tail calls after reload.

--- gcc/dse.c.jj	2015-01-05 22:41:19.000000000 +0100
+++ gcc/dse.c	2015-01-06 11:39:39.450832915 +0100
@@ -371,9 +371,11 @@ struct insn_info
 	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.  */
+     This field is set after reload for const function calls and before
+     reload for const tail function calls on targets where arg pointer
+     is the frame pointer.  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;
 
   /* This field is only used for the processing of const functions.
@@ -2483,17 +2485,6 @@ scan_insn (bb_info_t bb_info, rtx_insn *
 
       insn_info->cannot_delete = true;
 
-      /* Arguments for a sibling call that are pushed to memory are passed
-	 using the incoming argument pointer of the current function.  These
-	 may or may not be frame related depending on the target.  Since
-	 argument pointer related stores are not currently tracked, we treat
-	 a sibling call as though it does a wild read.  */
-      if (SIBLING_CALL_P (insn))
-	{
-	  add_wild_read (bb_info);
-	  return;
-	}
-
       /* Const functions cannot do anything bad i.e. read memory,
 	 however, they can read their parameters which may have
 	 been pushed onto the stack.
@@ -2527,7 +2518,13 @@ scan_insn (bb_info_t bb_info, rtx_insn *
 		     const_call ? "const" : "memset", INSN_UID (insn));
 
 	  /* See the head comment of the frame_read field.  */
-	  if (reload_completed)
+	  if (reload_completed
+	      /* Tail calls are storing their arguments using
+		 arg poinnter.  If it is a frame pointer on the target,
+		 even before reload we need to kill frame pointer based
+		 stores.  */
+	      || (SIBLING_CALL_P (insn)
+		  && HARD_FRAME_POINTER_IS_ARG_POINTER))
 	    insn_info->frame_read = true;
 
 	  /* Loop over the active stores and remove those which are
@@ -2601,7 +2598,11 @@ scan_insn (bb_info_t bb_info, rtx_insn *
 		}
 	    }
 	}
-
+      else if (SIBLING_CALL_P (insn) && reload_completed)
+	/* Arguments for a sibling call that are pushed to memory are passed
+	   using the incoming argument pointer of the current function.  After
+	   reload that might be (and likely is) frame pointer based.  */
+	add_wild_read (bb_info);
       else
 	/* Every other call, including pure functions, may read any memory
            that is not relative to the frame.  */


	Jakub

Re: [PATCH] Fix up DSE - PR middle-end/64388, target/55023

[John David Anglin]

On 1/6/2015 9:08 AM, Jakub Jelinek wrote:
> @@ -2527,7 +2518,13 @@ scan_insn (bb_info_t bb_info, rtx_insn *
>   		     const_call ? "const" : "memset", INSN_UID (insn));
>   
>   	  /* See the head comment of the frame_read field.  */
> -	  if (reload_completed)
> +	  if (reload_completed
> +	      /* Tail calls are storing their arguments using
> +		 arg poinnter.  If it is a frame pointer on the target,
Typo.
> +		 even before reload we need to kill frame pointer based
> +		 stores.  */
> +	      || (SIBLING_CALL_P (insn)
> +		  && HARD_FRAME_POINTER_IS_ARG_POINTER))
>   	    insn_info->frame_read = true;
>   

I had tested this hunk before, without the 
"HARD_FRAME_POINTER_IS_ARG_POINTER"
addition, on 32-bit hppa and it resolved the original test case.

Thanks,
Dave

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

Re: [PATCH] Fix up DSE - PR middle-end/64388, target/55023

[Jakub Jelinek]

On Tue, Jan 06, 2015 at 12:07:17PM -0500, John David Anglin wrote:
> On 1/6/2015 9:08 AM, Jakub Jelinek wrote:
> >@@ -2527,7 +2518,13 @@ scan_insn (bb_info_t bb_info, rtx_insn *
> >  		     const_call ? "const" : "memset", INSN_UID (insn));
> >  	  /* See the head comment of the frame_read field.  */
> >-	  if (reload_completed)
> >+	  if (reload_completed
> >+	      /* Tail calls are storing their arguments using
> >+		 arg poinnter.  If it is a frame pointer on the target,
> Typo.

Consider it fixed in my copy.

> >+		 even before reload we need to kill frame pointer based
> >+		 stores.  */
> >+	      || (SIBLING_CALL_P (insn)
> >+		  && HARD_FRAME_POINTER_IS_ARG_POINTER))
> >  	    insn_info->frame_read = true;
> 
> I had tested this hunk before, without the
> "HARD_FRAME_POINTER_IS_ARG_POINTER"
> addition, on 32-bit hppa and it resolved the original test case.

	Jakub

Re: [PATCH] Fix up DSE - PR middle-end/64388, target/55023

[Jeff Law]

On 01/06/15 07:08, Jakub Jelinek wrote:
> Hi!
>
> On Mon, Jan 05, 2015 at 10:31:17PM +0100, Jakub Jelinek wrote:
>> Or you could e.g. do the
>>    if (HARD_FRAME_POINTER_IS_ARG_POINTER
>>        && !reload_completed
>>        && SIBLING_CALL_P (insn))
>>      { add_wild_read (bb_info); return; }
>> case first, then compute const_call and memset_call,
>>    if (const_call || memset_call)
>>      { current_big_block }
>>    else if (reload_completed && SIBLING_CALL_P (insn))
>>      add_wild_read (bb_info);
>>    else
>>      add_non_frame_wild_read (bb_info);
>>
>> That way, you would not punish const/memset calls unnecessarily after reload
>> when they are sibling calls.
>
> So what about this way?  Even for the HARD_FRAME_POINTER_IS_ARG_POINTER
> before reload case, there is no reason for wild read IMHO, just setting
> frame_read should do all that is necessary.  Bootstrapped/regtested on
> x86_64-linux and i686-linux, tested on the pr55023 testcase with cross
> to hppa-linux.  Ok for trunk?
>
> 2015-01-06  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR target/55023
> 	PR middle-end/64388
> 	* dse.c (struct insn_info): Mention frame_read set also
> 	before reload for tail calls on some targets.
> 	(scan_insn): Revert 2014-12-22 change.  Set frame_read
> 	also before reload for tail calls if
> 	HARD_FRAME_POINTER_IS_ARG_POINTER.  Call add_wild_read
> 	instead of add_non_frame_wild_read for non-const/memset
> 	tail calls after reload.
OK with the typo John pointed out fixed.

jeff