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