treat argp-based mem as frame related in dse
Checks
Commit Message
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 ®test 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
Comments
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 ®test 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
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 ®test 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
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 ®test 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
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 ®test 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
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
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
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
> 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 ®test 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
@@ -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;
}
new file mode 100644
@@ -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} } } */