On 12/8/23 00:18, Jiufu Guo wrote:
Hi,
Jeff Law <jeffreya...@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
