[PATCH] treat argp-based mem as frame related in dse

[PATCH] treat argp-based mem as frame related in dse

[Jiufu Guo]

Hi,

The issue mentioned in PR112525 would be able to be handled by                             
updating dse.cc to treat arg_pointer_rtx similarly with frame_pointer_rtx.                             
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=30271#c10 also mentioned                              
this idea.                                                           
                                                                
One thing, arpg area may be used to pass argument to callee. So, it would                            
be needed to check if call insns are using that mem.

Bootstrap &regtest pass on ppc64{,le} and x86_64.
Is this ok for trunk?

BR,
Jeff (Jiufu Guo)


	PR rtl-optimization/112525

gcc/ChangeLog:

	* dse.cc (get_group_info): Add arg_pointer_rtx as frame_related.
	(check_mem_read_rtx): Add parameter to indicate if it is checking mem
	for call insn.
	(scan_insn): Add mem checking on call usage.

gcc/testsuite/ChangeLog:

	* gcc.target/powerpc/pr112525.c: New test.

---
 gcc/dse.cc                                  | 17 +++++++++++++----
 gcc/testsuite/gcc.target/powerpc/pr112525.c | 15 +++++++++++++++
 2 files changed, 28 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr112525.c

diff --git a/gcc/dse.cc b/gcc/dse.cc
index 1a85dae1f8c..f43b7bf8ba6 100644
--- a/gcc/dse.cc
+++ b/gcc/dse.cc
@@ -682,7 +682,8 @@ get_group_info (rtx base)
       gi->group_kill = BITMAP_ALLOC (&dse_bitmap_obstack);
       gi->process_globally = false;
       gi->frame_related =
-	(base == frame_pointer_rtx) || (base == hard_frame_pointer_rtx);
+	(base == frame_pointer_rtx) || (base == hard_frame_pointer_rtx)
+	|| (base == arg_pointer_rtx && fixed_regs[ARG_POINTER_REGNUM]);
       gi->offset_map_size_n = 0;
       gi->offset_map_size_p = 0;
       gi->offset_map_n = NULL;
@@ -2157,7 +2158,7 @@ replace_read (store_info *store_info, insn_info_t store_insn,
    be active.  */
 
 static void
-check_mem_read_rtx (rtx *loc, bb_info_t bb_info)
+check_mem_read_rtx (rtx *loc, bb_info_t bb_info, bool used_in_call = false)
 {
   rtx mem = *loc, mem_addr;
   insn_info_t insn_info;
@@ -2302,7 +2303,8 @@ check_mem_read_rtx (rtx *loc, bb_info_t bb_info)
 		 stored, rewrite the read.  */
 	      else
 		{
-		  if (store_info->rhs
+		  if (!used_in_call
+		      && store_info->rhs
 		      && known_subrange_p (offset, width, store_info->offset,
 					   store_info->width)
 		      && all_positions_needed_p (store_info,
@@ -2368,7 +2370,8 @@ check_mem_read_rtx (rtx *loc, bb_info_t bb_info)
 
 	  /* If this read is just reading back something that we just
 	     stored, rewrite the read.  */
-	  if (store_info->rhs
+	  if (!used_in_call
+	      && store_info->rhs
 	      && store_info->group_id == -1
 	      && store_info->cse_base == base
 	      && known_subrange_p (offset, width, store_info->offset,
@@ -2650,6 +2653,12 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn, int max_active_local_stores)
            that is not relative to the frame.  */
         add_non_frame_wild_read (bb_info);
 
+      for (rtx link = CALL_INSN_FUNCTION_USAGE (insn);
+	   link != NULL_RTX;
+	   link = XEXP (link, 1))
+	if (GET_CODE (XEXP (link, 0)) == USE && MEM_P (XEXP (XEXP (link, 0),0)))
+	  check_mem_read_rtx (&XEXP (XEXP (link, 0),0), bb_info, true);
+
       return;
     }
 
diff --git a/gcc/testsuite/gcc.target/powerpc/pr112525.c b/gcc/testsuite/gcc.target/powerpc/pr112525.c
new file mode 100644
index 00000000000..428598188e7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr112525.c
@@ -0,0 +1,15 @@
+/* { dg-do compile { target lp64 } } */
+/* { dg-options "-O2" } */
+
+typedef struct teststruct
+{
+  double d;
+  int arr[15]; 
+} teststruct;
+
+void
+foo (teststruct p)
+{
+}
+
+/* { dg-final { scan-assembler-not {\mstd\M} } } */
-- 
2.25.1

Re: [PATCH] treat argp-based mem as frame related in dse

[Jeff Law]

On 12/6/23 02:27, Jiufu Guo wrote:
> Hi,
> 
> The issue mentioned in PR112525 would be able to be handled by
> updating dse.cc to treat arg_pointer_rtx similarly with frame_pointer_rtx.
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=30271#c10 also mentioned
> this idea.
>                                                                  
> One thing, arpg area may be used to pass argument to callee. So, it would
> be needed to check if call insns are using that mem.
> 
> Bootstrap &regtest pass on ppc64{,le} and x86_64.
> Is this ok for trunk?
> 
> BR,
> Jeff (Jiufu Guo)
> 
> 
> 	PR rtl-optimization/112525
> 
> gcc/ChangeLog:
> 
> 	* dse.cc (get_group_info): Add arg_pointer_rtx as frame_related.
> 	(check_mem_read_rtx): Add parameter to indicate if it is checking mem
> 	for call insn.
> 	(scan_insn): Add mem checking on call usage.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/powerpc/pr112525.c: New test.
So conceptually the first chunk makes sense.  Though I do worry about 
Andrew's comment about it causing a bootstrap failure.  Even thought it 
was 15 years ago, it remains worrisome.


> @@ -2368,7 +2370,8 @@ check_mem_read_rtx (rtx *loc, bb_info_t bb_info)
>   
>   	  /* If this read is just reading back something that we just
>   	     stored, rewrite the read.  */
> -	  if (store_info->rhs
> +	  if (!used_in_call
> +	      && store_info->rhs
>   	      && store_info->group_id == -1
>   	      && store_info->cse_base == base
>   	      && known_subrange_p (offset, width, store_info->offset,
> @@ -2650,6 +2653,12 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn, int max_active_local_stores)
>              that is not relative to the frame.  */
>           add_non_frame_wild_read (bb_info);
>   
> +      for (rtx link = CALL_INSN_FUNCTION_USAGE (insn);
> +	   link != NULL_RTX;
> +	   link = XEXP (link, 1))
> +	if (GET_CODE (XEXP (link, 0)) == USE && MEM_P (XEXP (XEXP (link, 0),0)))
> +	  check_mem_read_rtx (&XEXP (XEXP (link, 0),0), bb_info, true);
I'm having a bit of a hard time convincing myself this is correct 
though.  I can't see how rewriting the load to read the source of the 
prior store is unsafe.  If that fixes a problem, then it would seem like 
we've gone wrong before here -- perhaps failing to use the fusage loads 
to "kill" any available stores to the same or aliased memory locations.

Assuming we get to a point where we think this or something similar to 
it is safe, then we should retest pr30271 and if it's fixed reference it 
in the ChangeLog.

Jeff

Re: [PATCH] treat argp-based mem as frame related in dse

[Jiufu Guo]

Hi,

Jeff Law <jeffreyalaw@gmail.com> writes:

> On 12/6/23 02:27, Jiufu Guo wrote:
>> Hi,
>>
>> The issue mentioned in PR112525 would be able to be handled by
>> updating dse.cc to treat arg_pointer_rtx similarly with frame_pointer_rtx.
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=30271#c10 also mentioned
>> this idea.
>>                                                                  One thing, arpg area may be used to pass argument to callee. So, it would
>> be needed to check if call insns are using that mem.
>>
>> Bootstrap &regtest pass on ppc64{,le} and x86_64.
>> Is this ok for trunk?
>>
>> BR,
>> Jeff (Jiufu Guo)
>>
>>
>> 	PR rtl-optimization/112525
>>
>> gcc/ChangeLog:
>>
>> 	* dse.cc (get_group_info): Add arg_pointer_rtx as frame_related.
>> 	(check_mem_read_rtx): Add parameter to indicate if it is checking mem
>> 	for call insn.
>> 	(scan_insn): Add mem checking on call usage.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	* gcc.target/powerpc/pr112525.c: New test.
> So conceptually the first chunk makes sense.  Though I do worry about
> Andrew's comment about it causing a bootstrap failure.  Even thought
> it was 15 years ago, it remains worrisome.
>
Yes, I understand your point.
At that time, it is a comparesion failure. It may be related to debug
info.  But I did not figure out possible failures.

>
>> @@ -2368,7 +2370,8 @@ check_mem_read_rtx (rtx *loc, bb_info_t bb_info)
>>     	  /* If this read is just reading back something that we just
>>   	     stored, rewrite the read.  */
>> -	  if (store_info->rhs
>> +	  if (!used_in_call
>> +	      && store_info->rhs
>>   	      && store_info->group_id == -1
>>   	      && store_info->cse_base == base
>>   	      && known_subrange_p (offset, width, store_info->offset,
>> @@ -2650,6 +2653,12 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn, int max_active_local_stores)
>>              that is not relative to the frame.  */
>>           add_non_frame_wild_read (bb_info);
>>   +      for (rtx link = CALL_INSN_FUNCTION_USAGE (insn);
>> +	   link != NULL_RTX;
>> +	   link = XEXP (link, 1))
>> +	if (GET_CODE (XEXP (link, 0)) == USE && MEM_P (XEXP (XEXP (link, 0),0)))
>> +	  check_mem_read_rtx (&XEXP (XEXP (link, 0),0), bb_info, true);
> I'm having a bit of a hard time convincing myself this is correct
> though.  I can't see how rewriting the load to read the source of the
> prior store is unsafe.  If that fixes a problem, then it would seem
> like we've gone wrong before here -- perhaps failing to use the fusage
> loads to "kill" any available stores to the same or aliased memory
> locations.
As you said the later one, call's fusage would killing the previous
store. It is a kind of case like:

  134: [argp:SI+0x8]=r134:SI
  135: [argp:SI+0x4]=0x1
  136: [argp:SI]=r132:SI
  137: ax:SI=call [`memset'] argc:0xc
      REG_CALL_DECL `memset'
      REG_EH_REGION 0

This call insn is:
(call_insn/j 137 136 147 27 (set (reg:SI 0 ax)
        (call (mem:QI (symbol_ref:SI ("memset") [flags 0x41]  <function_decl __builtin_memset>) [0 __builtin_memset S1 A8])
            (const_int 12 [0xc]))) "pr102798.c":23:22 1086 {*sibcall_value}
     (expr_list:REG_UNUSED (reg:SI 0 ax)
        (expr_list:REG_CALL_DECL (symbol_ref:SI ("memset") [flags 0x41]  <function_decl __builtin_memset>)
            (expr_list:REG_EH_REGION (const_int 0 [0])
                (nil))))
    (expr_list:SI (use (mem/f:SI (reg/f:SI 16 argp) [0  S4 A32]))
        (expr_list:SI (use (mem:SI (plus:SI (reg/f:SI 16 argp) (const_int 4 [0x4])) [0  S4 A32]))
            (expr_list:SI (use (mem:SI (plus:SI (reg/f:SI 16 argp) (const_int 8 [0x8])) [0  S4 A32]))
                (nil)))))

The stores in "insns 134-136" are used by the call. "check_mem_read_rtx"
would prevent them to eliminated.

>
> Assuming we get to a point where we think this or something similar to
> it is safe, then we should retest pr30271 and if it's fixed reference
> it in the ChangeLog.
Yes, thanks for pointing out this.  The ChangeLog should also reference
30271.

BR,
Jeff (Jiufu Guo)

>
> Jeff

Re: [PATCH] treat argp-based mem as frame related in dse

[Jeff Law]

On 12/8/23 00:18, Jiufu Guo wrote:
> 
> Hi,
> 
> Jeff Law <jeffreyalaw@gmail.com> writes:
> 
>> On 12/6/23 02:27, Jiufu Guo wrote:
>>> Hi,
>>>
>>> The issue mentioned in PR112525 would be able to be handled by
>>> updating dse.cc to treat arg_pointer_rtx similarly with frame_pointer_rtx.
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=30271#c10 also mentioned
>>> this idea.
>>>                                                                   One thing, arpg area may be used to pass argument to callee. So, it would
>>> be needed to check if call insns are using that mem.
>>>
>>> Bootstrap &regtest pass on ppc64{,le} and x86_64.
>>> Is this ok for trunk?
>>>
>>> BR,
>>> Jeff (Jiufu Guo)
>>>
>>>
>>> 	PR rtl-optimization/112525
>>>
>>> gcc/ChangeLog:
>>>
>>> 	* dse.cc (get_group_info): Add arg_pointer_rtx as frame_related.
>>> 	(check_mem_read_rtx): Add parameter to indicate if it is checking mem
>>> 	for call insn.
>>> 	(scan_insn): Add mem checking on call usage.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	* gcc.target/powerpc/pr112525.c: New test.
>> So conceptually the first chunk makes sense.  Though I do worry about
>> Andrew's comment about it causing a bootstrap failure.  Even thought
>> it was 15 years ago, it remains worrisome.
>>
> Yes, I understand your point.
> At that time, it is a comparesion failure. It may be related to debug
> info.  But I did not figure out possible failures.
> 
>>
>>> @@ -2368,7 +2370,8 @@ check_mem_read_rtx (rtx *loc, bb_info_t bb_info)
>>>      	  /* If this read is just reading back something that we just
>>>    	     stored, rewrite the read.  */
>>> -	  if (store_info->rhs
>>> +	  if (!used_in_call
>>> +	      && store_info->rhs
>>>    	      && store_info->group_id == -1
>>>    	      && store_info->cse_base == base
>>>    	      && known_subrange_p (offset, width, store_info->offset,
>>> @@ -2650,6 +2653,12 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn, int max_active_local_stores)
>>>               that is not relative to the frame.  */
>>>            add_non_frame_wild_read (bb_info);
>>>    +      for (rtx link = CALL_INSN_FUNCTION_USAGE (insn);
>>> +	   link != NULL_RTX;
>>> +	   link = XEXP (link, 1))
>>> +	if (GET_CODE (XEXP (link, 0)) == USE && MEM_P (XEXP (XEXP (link, 0),0)))
>>> +	  check_mem_read_rtx (&XEXP (XEXP (link, 0),0), bb_info, true);
>> I'm having a bit of a hard time convincing myself this is correct
>> though.  I can't see how rewriting the load to read the source of the
>> prior store is unsafe.  If that fixes a problem, then it would seem
>> like we've gone wrong before here -- perhaps failing to use the fusage
>> loads to "kill" any available stores to the same or aliased memory
>> locations.
> As you said the later one, call's fusage would killing the previous
> store. It is a kind of case like:
> 
>    134: [argp:SI+0x8]=r134:SI
>    135: [argp:SI+0x4]=0x1
>    136: [argp:SI]=r132:SI
>    137: ax:SI=call [`memset'] argc:0xc
>        REG_CALL_DECL `memset'
>        REG_EH_REGION 0
> 
> This call insn is:
> (call_insn/j 137 136 147 27 (set (reg:SI 0 ax)
>          (call (mem:QI (symbol_ref:SI ("memset") [flags 0x41]  <function_decl __builtin_memset>) [0 __builtin_memset S1 A8])
>              (const_int 12 [0xc]))) "pr102798.c":23:22 1086 {*sibcall_value}
>       (expr_list:REG_UNUSED (reg:SI 0 ax)
>          (expr_list:REG_CALL_DECL (symbol_ref:SI ("memset") [flags 0x41]  <function_decl __builtin_memset>)
>              (expr_list:REG_EH_REGION (const_int 0 [0])
>                  (nil))))
>      (expr_list:SI (use (mem/f:SI (reg/f:SI 16 argp) [0  S4 A32]))
>          (expr_list:SI (use (mem:SI (plus:SI (reg/f:SI 16 argp) (const_int 4 [0x4])) [0  S4 A32]))
>              (expr_list:SI (use (mem:SI (plus:SI (reg/f:SI 16 argp) (const_int 8 [0x8])) [0  S4 A32]))
>                  (nil)))))
> 
> The stores in "insns 134-136" are used by the call. "check_mem_read_rtx"
> would prevent them to eliminated.
Right.  But unless I read something wrong, the patch wasn't changing 
store removal, it was changing whether or not we forwarded the source of 
the store into the destination of a subsequent load from the same address.

So I'm still a bit confused how this patch impacted store removal.

jeff

Re: [PATCH] treat argp-based mem as frame related in dse

[Jiufu Guo]

Hi,

Thanks again for your kind review!

Jeff Law <jeffreyalaw@gmail.com> writes:

> On 12/8/23 00:18, Jiufu Guo wrote:
>>
>> Hi,
>>
>> Jeff Law <jeffreyalaw@gmail.com> writes:
>>
>>> On 12/6/23 02:27, Jiufu Guo wrote:
>>>> Hi,
>>>>
>>>> The issue mentioned in PR112525 would be able to be handled by
>>>> updating dse.cc to treat arg_pointer_rtx similarly with frame_pointer_rtx.
>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=30271#c10 also mentioned
>>>> this idea.
>>>>                                                                   One thing, arpg area may be used to pass argument to callee. So, it would
>>>> be needed to check if call insns are using that mem.
>>>>
>>>> Bootstrap &regtest pass on ppc64{,le} and x86_64.
>>>> Is this ok for trunk?
>>>>
>>>> BR,
>>>> Jeff (Jiufu Guo)
>>>>
>>>>
>>>> 	PR rtl-optimization/112525
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>> 	* dse.cc (get_group_info): Add arg_pointer_rtx as frame_related.
>>>> 	(check_mem_read_rtx): Add parameter to indicate if it is checking mem
>>>> 	for call insn.
>>>> 	(scan_insn): Add mem checking on call usage.
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>>
>>>> 	* gcc.target/powerpc/pr112525.c: New test.
>>> So conceptually the first chunk makes sense.  Though I do worry about
>>> Andrew's comment about it causing a bootstrap failure.  Even thought
>>> it was 15 years ago, it remains worrisome.
>>>
>> Yes, I understand your point.
>> At that time, it is a comparesion failure. It may be related to debug
>> info.  But I did not figure out possible failures.
>>
>>>
>>>> @@ -2368,7 +2370,8 @@ check_mem_read_rtx (rtx *loc, bb_info_t bb_info)
>>>>      	  /* If this read is just reading back something that we just
>>>>    	     stored, rewrite the read.  */
>>>> -	  if (store_info->rhs
>>>> +	  if (!used_in_call
>>>> +	      && store_info->rhs
>>>>    	      && store_info->group_id == -1
>>>>    	      && store_info->cse_base == base
>>>>    	      && known_subrange_p (offset, width, store_info->offset,
>>>> @@ -2650,6 +2653,12 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn, int max_active_local_stores)
>>>>               that is not relative to the frame.  */
>>>>            add_non_frame_wild_read (bb_info);
>>>>    +      for (rtx link = CALL_INSN_FUNCTION_USAGE (insn);
>>>> +	   link != NULL_RTX;
>>>> +	   link = XEXP (link, 1))
>>>> +	if (GET_CODE (XEXP (link, 0)) == USE && MEM_P (XEXP (XEXP (link, 0),0)))
>>>> +	  check_mem_read_rtx (&XEXP (XEXP (link, 0),0), bb_info, true);
>>> I'm having a bit of a hard time convincing myself this is correct
>>> though.  I can't see how rewriting the load to read the source of the
>>> prior store is unsafe.  If that fixes a problem, then it would seem
>>> like we've gone wrong before here -- perhaps failing to use the fusage
>>> loads to "kill" any available stores to the same or aliased memory
>>> locations.
>> As you said the later one, call's fusage would killing the previous
>> store. It is a kind of case like:
>>
>>    134: [argp:SI+0x8]=r134:SI
>>    135: [argp:SI+0x4]=0x1
>>    136: [argp:SI]=r132:SI
>>    137: ax:SI=call [`memset'] argc:0xc
>>        REG_CALL_DECL `memset'
>>        REG_EH_REGION 0
>>
>> This call insn is:
>> (call_insn/j 137 136 147 27 (set (reg:SI 0 ax)
>>          (call (mem:QI (symbol_ref:SI ("memset") [flags 0x41]  <function_decl __builtin_memset>) [0 __builtin_memset S1 A8])
>>              (const_int 12 [0xc]))) "pr102798.c":23:22 1086 {*sibcall_value}
>>       (expr_list:REG_UNUSED (reg:SI 0 ax)
>>          (expr_list:REG_CALL_DECL (symbol_ref:SI ("memset") [flags 0x41]  <function_decl __builtin_memset>)
>>              (expr_list:REG_EH_REGION (const_int 0 [0])
>>                  (nil))))
>>      (expr_list:SI (use (mem/f:SI (reg/f:SI 16 argp) [0  S4 A32]))
>>          (expr_list:SI (use (mem:SI (plus:SI (reg/f:SI 16 argp) (const_int 4 [0x4])) [0  S4 A32]))
>>              (expr_list:SI (use (mem:SI (plus:SI (reg/f:SI 16 argp) (const_int 8 [0x8])) [0  S4 A32]))
>>                  (nil)))))
>>
>> The stores in "insns 134-136" are used by the call. "check_mem_read_rtx"
>> would prevent them to eliminated.
> Right.  But unless I read something wrong, the patch wasn't changing
> store removal, it was changing whether or not we forwarded the source
> of the store into the destination of a subsequent load from the same
> address.
"check_mem_read_rtx" has another behavior which checks the mem
and adds read_info to insn_info->read_rec. "read_rec" could prevent
the "store" from being eliminated during the dse's global alg. This
patch leverages this behavior.
And to avoid the "mem on fusage" to be replaced by leading store's rhs
"replace_read" was disabled if the mem is on the call's fusage.


BR,
Jeff (Jiufu Guo)

>
> So I'm still a bit confused how this patch impacted store removal.
>
> jeff

Re: [PATCH] treat argp-based mem as frame related in dse

[Jeff Law]

On 12/10/23 20:07, Jiufu Guo wrote:

>>>> I'm having a bit of a hard time convincing myself this is correct
>>>> though.  I can't see how rewriting the load to read the source of the
>>>> prior store is unsafe.  If that fixes a problem, then it would seem
>>>> like we've gone wrong before here -- perhaps failing to use the fusage
>>>> loads to "kill" any available stores to the same or aliased memory
>>>> locations.
>>> As you said the later one, call's fusage would killing the previous
>>> store. It is a kind of case like:
>>>
>>>     134: [argp:SI+0x8]=r134:SI
>>>     135: [argp:SI+0x4]=0x1
>>>     136: [argp:SI]=r132:SI
>>>     137: ax:SI=call [`memset'] argc:0xc
>>>         REG_CALL_DECL `memset'
>>>         REG_EH_REGION 0
>>>
>>> This call insn is:
>>> (call_insn/j 137 136 147 27 (set (reg:SI 0 ax)
>>>           (call (mem:QI (symbol_ref:SI ("memset") [flags 0x41]  <function_decl __builtin_memset>) [0 __builtin_memset S1 A8])
>>>               (const_int 12 [0xc]))) "pr102798.c":23:22 1086 {*sibcall_value}
>>>        (expr_list:REG_UNUSED (reg:SI 0 ax)
>>>           (expr_list:REG_CALL_DECL (symbol_ref:SI ("memset") [flags 0x41]  <function_decl __builtin_memset>)
>>>               (expr_list:REG_EH_REGION (const_int 0 [0])
>>>                   (nil))))
>>>       (expr_list:SI (use (mem/f:SI (reg/f:SI 16 argp) [0  S4 A32]))
>>>           (expr_list:SI (use (mem:SI (plus:SI (reg/f:SI 16 argp) (const_int 4 [0x4])) [0  S4 A32]))
>>>               (expr_list:SI (use (mem:SI (plus:SI (reg/f:SI 16 argp) (const_int 8 [0x8])) [0  S4 A32]))
>>>                   (nil)))))
>>>
>>> The stores in "insns 134-136" are used by the call. "check_mem_read_rtx"
>>> would prevent them to eliminated.
>> Right.  But unless I read something wrong, the patch wasn't changing
>> store removal, it was changing whether or not we forwarded the source
>> of the store into the destination of a subsequent load from the same
>> address.
> "check_mem_read_rtx" has another behavior which checks the mem
> and adds read_info to insn_info->read_rec. "read_rec" could prevent
> the "store" from being eliminated during the dse's global alg. This
> patch leverages this behavior.
> And to avoid the "mem on fusage" to be replaced by leading store's rhs
> "replace_read" was disabled if the mem is on the call's fusage.
Ah, so not only do we want to avoid the call to replace_read, but also 
avoid the early return.

By avoiding the early return, we proceed into later code which "kills" 
the tracked store, thus avoiding the problem.  Right?

jeff

Re: [PATCH] treat argp-based mem as frame related in dse

[Jiufu Guo]

Hi,

Thanks for your quick reply!

Jeff Law <jeffreyalaw@gmail.com> writes:

> On 12/10/23 20:07, Jiufu Guo wrote:
>
>>>>> I'm having a bit of a hard time convincing myself this is correct
>>>>> though.  I can't see how rewriting the load to read the source of the
>>>>> prior store is unsafe.  If that fixes a problem, then it would seem
>>>>> like we've gone wrong before here -- perhaps failing to use the fusage
>>>>> loads to "kill" any available stores to the same or aliased memory
>>>>> locations.
>>>> As you said the later one, call's fusage would killing the previous
>>>> store. It is a kind of case like:
>>>>
>>>>     134: [argp:SI+0x8]=r134:SI
>>>>     135: [argp:SI+0x4]=0x1
>>>>     136: [argp:SI]=r132:SI
>>>>     137: ax:SI=call [`memset'] argc:0xc
>>>>         REG_CALL_DECL `memset'
>>>>         REG_EH_REGION 0
>>>>
>>>> This call insn is:
>>>> (call_insn/j 137 136 147 27 (set (reg:SI 0 ax)
>>>>           (call (mem:QI (symbol_ref:SI ("memset") [flags 0x41]  <function_decl __builtin_memset>) [0 __builtin_memset S1 A8])
>>>>               (const_int 12 [0xc]))) "pr102798.c":23:22 1086 {*sibcall_value}
>>>>        (expr_list:REG_UNUSED (reg:SI 0 ax)
>>>>           (expr_list:REG_CALL_DECL (symbol_ref:SI ("memset") [flags 0x41]  <function_decl __builtin_memset>)
>>>>               (expr_list:REG_EH_REGION (const_int 0 [0])
>>>>                   (nil))))
>>>>       (expr_list:SI (use (mem/f:SI (reg/f:SI 16 argp) [0  S4 A32]))
>>>>           (expr_list:SI (use (mem:SI (plus:SI (reg/f:SI 16 argp) (const_int 4 [0x4])) [0  S4 A32]))
>>>>               (expr_list:SI (use (mem:SI (plus:SI (reg/f:SI 16 argp) (const_int 8 [0x8])) [0  S4 A32]))
>>>>                   (nil)))))
>>>>
>>>> The stores in "insns 134-136" are used by the call. "check_mem_read_rtx"
>>>> would prevent them to eliminated.
>>> Right.  But unless I read something wrong, the patch wasn't changing
>>> store removal, it was changing whether or not we forwarded the source
>>> of the store into the destination of a subsequent load from the same
>>> address.
>> "check_mem_read_rtx" has another behavior which checks the mem
>> and adds read_info to insn_info->read_rec. "read_rec" could prevent
>> the "store" from being eliminated during the dse's global alg. This
>> patch leverages this behavior.
>> And to avoid the "mem on fusage" to be replaced by leading store's rhs
>> "replace_read" was disabled if the mem is on the call's fusage.
> Ah, so not only do we want to avoid the call to replace_read, but also avoid the early return.
>
> By avoiding the early return, we proceed into later code which "kills"
> the tracked store, thus avoiding the problem.  Right?
It is similar, I would say.  There is "leading code" as below:
  /* Look at all of the uses in the insn.  */
  note_uses (&PATTERN (insn), check_mem_read_use, bb_info);

This checks possible loads in the "insn" and "kills" the tracked
stores if needed.
But "note_uses" does not check the fusage of the call insn.
So, this patch proceed the code "check_mem_read" for the "use mem"
on fusage.

BR,
Jeff (Jiufu Guo)

>
> jeff

Re: [PATCH] treat argp-based mem as frame related in dse

[Jeff Law]

On 12/11/23 02:26, Jiufu Guo wrote:
> 
> Hi,
> 
> Thanks for your quick reply!
> 
> Jeff Law <jeffreyalaw@gmail.com> writes:
> 
>> On 12/10/23 20:07, Jiufu Guo wrote:
>>
>>>>>> I'm having a bit of a hard time convincing myself this is correct
>>>>>> though.  I can't see how rewriting the load to read the source of the
>>>>>> prior store is unsafe.  If that fixes a problem, then it would seem
>>>>>> like we've gone wrong before here -- perhaps failing to use the fusage
>>>>>> loads to "kill" any available stores to the same or aliased memory
>>>>>> locations.
>>>>> As you said the later one, call's fusage would killing the previous
>>>>> store. It is a kind of case like:
>>>>>
>>>>>      134: [argp:SI+0x8]=r134:SI
>>>>>      135: [argp:SI+0x4]=0x1
>>>>>      136: [argp:SI]=r132:SI
>>>>>      137: ax:SI=call [`memset'] argc:0xc
>>>>>          REG_CALL_DECL `memset'
>>>>>          REG_EH_REGION 0
>>>>>
>>>>> This call insn is:
>>>>> (call_insn/j 137 136 147 27 (set (reg:SI 0 ax)
>>>>>            (call (mem:QI (symbol_ref:SI ("memset") [flags 0x41]  <function_decl __builtin_memset>) [0 __builtin_memset S1 A8])
>>>>>                (const_int 12 [0xc]))) "pr102798.c":23:22 1086 {*sibcall_value}
>>>>>         (expr_list:REG_UNUSED (reg:SI 0 ax)
>>>>>            (expr_list:REG_CALL_DECL (symbol_ref:SI ("memset") [flags 0x41]  <function_decl __builtin_memset>)
>>>>>                (expr_list:REG_EH_REGION (const_int 0 [0])
>>>>>                    (nil))))
>>>>>        (expr_list:SI (use (mem/f:SI (reg/f:SI 16 argp) [0  S4 A32]))
>>>>>            (expr_list:SI (use (mem:SI (plus:SI (reg/f:SI 16 argp) (const_int 4 [0x4])) [0  S4 A32]))
>>>>>                (expr_list:SI (use (mem:SI (plus:SI (reg/f:SI 16 argp) (const_int 8 [0x8])) [0  S4 A32]))
>>>>>                    (nil)))))
>>>>>
>>>>> The stores in "insns 134-136" are used by the call. "check_mem_read_rtx"
>>>>> would prevent them to eliminated.
>>>> Right.  But unless I read something wrong, the patch wasn't changing
>>>> store removal, it was changing whether or not we forwarded the source
>>>> of the store into the destination of a subsequent load from the same
>>>> address.
>>> "check_mem_read_rtx" has another behavior which checks the mem
>>> and adds read_info to insn_info->read_rec. "read_rec" could prevent
>>> the "store" from being eliminated during the dse's global alg. This
>>> patch leverages this behavior.
>>> And to avoid the "mem on fusage" to be replaced by leading store's rhs
>>> "replace_read" was disabled if the mem is on the call's fusage.
>> Ah, so not only do we want to avoid the call to replace_read, but also avoid the early return.
>>
>> By avoiding the early return, we proceed into later code which "kills"
>> the tracked store, thus avoiding the problem.  Right?
> It is similar, I would say.  There is "leading code" as below:
>    /* Look at all of the uses in the insn.  */
>    note_uses (&PATTERN (insn), check_mem_read_use, bb_info);
> 
> This checks possible loads in the "insn" and "kills" the tracked
> stores if needed.
> But "note_uses" does not check the fusage of the call insn.
> So, this patch proceed the code "check_mem_read" for the "use mem"
> on fusage.
OK for the trunk.  Please double check that older BZ and if that issue 
is fixed as well, add it to the commit log.  Thanks for walking me 
through the details.

jeff

Re: [PATCH] treat argp-based mem as frame related in dse

[Hans-Peter Nilsson]

> From: Jiufu Guo <guojiufu@linux.ibm.com>
> Date: Wed,  6 Dec 2023 17:27:58 +0800

> Hi,
> 
> The issue mentioned in PR112525 would be able to be handled by                             
> updating dse.cc to treat arg_pointer_rtx similarly with frame_pointer_rtx.                             
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=30271#c10 also mentioned                              
> this idea.                                                           
>                                                                 
> One thing, arpg area may be used to pass argument to callee. So, it would                            
> be needed to check if call insns are using that mem.
> 
> Bootstrap &regtest pass on ppc64{,le} and x86_64.
> Is this ok for trunk?
> 
> BR,
> Jeff (Jiufu Guo)
> 
> 
> 	PR rtl-optimization/112525
> 
> gcc/ChangeLog:
> 
> 	* dse.cc (get_group_info): Add arg_pointer_rtx as frame_related.
> 	(check_mem_read_rtx): Add parameter to indicate if it is checking mem
> 	for call insn.
> 	(scan_insn): Add mem checking on call usage.

This, when committed as r14-6674-g4759383245ac97, caused all
or most test that "throw" to fail for cris-elf at execution
time, but I don't see other targets failing (cf. gcc-testresults
archives).  I opened PR113109 and will dig a little deeper.

brgds, H-P