On 12/11/23 02:26, Jiufu Guo wrote:
Hi,
Thanks for your quick reply!
Jeff Law <jeffreya...@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
