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.
BR,
Jeff (Jiufu Guo)
>
> jeff
