Hi,
Thanks again for your kind review! Jeff Law <jeffreya...@gmail.com> writes: > 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. "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
