On 2024-07-07 22:53 Jeff Law <jeffreya...@gmail.com> wrote:
>
>
>
>On 6/8/24 2:36 PM, Jeff Law wrote:
>>
>>
>> On 6/5/24 8:42 PM, Fei Gao wrote:
>>
>>>> But let's back up and get a good explanation of what the problem is.
>>>> Based on patch 2/2 it looks like we have lost an assignment to the
>>>> return register.
>>>>
>>>> To someone not familiar with this code, it sounds to me like we've made
>>>> a mistake earlier and we're now defining a hook that lets us go back and
>>>> fix that earlier mistake. I'm probably wrong, but so far that's what
>>>> it sounds like.
>>> Hi Jeff
>>>
>>> You're right. Let me rephrase patch 2/2 with more details. Search /*
>>> feigao to location the point I'm
>>> tring to explain.
>>>
>>> code snippets from gcc/function.cc
>>> void
>>> thread_prologue_and_epilogue_insns (void)
>>> {
>>> ...
>>> /*feigao:
>>> targetm.gen_epilogue () is called here to generate epilogue
>>> sequence.
>>> https://gcc.gnu.org/git/?
>>> p=gcc.git;a=commit;h=b27d323a368033f0b37e93c57a57a35fd9997864
>>> Commit above tries in targetm.gen_epilogue () to detect if
>>> there's li a0,0 insn at the end of insn chain, if so, cm.popret
>>> is replaced by cm.popretz and li a0,0 insn is deleted.
>> So that seems like the critical issue. Generation of the prologue/
>> epilogue really shouldn't be changing other instructions in the
>> instruction stream. I'm not immediately aware of another target that
>> does that, an it seems like a rather risky thing to do.
>>
>>
>> It looks like the cm.popretz's RTL exposes the assignment to a0 and
>> there's a DCE pass that runs after insertion of the prologue/epilogue.
>> So I would suggest leaving the assignment to a0 in the RTL chain and see
>> if the later DCE pass after prologue generation eliminates the redundant
>> assignment. That seems a lot cleaner.
>So I looked at this in a bit more detail. I'm going to explicitly
>reject this patch now.
>
>The removal of the set to a0 in riscv_gen_multi_pop_insn looks wrong on
>multiple levels. I don't think you have enough context in that routine
>or its callers to know if it's safe ie given this fragment of RTL:
>
>> (call_insn 12 11 13 3 (parallel [
>> (call (mem:SI (symbol_ref:SI ("test_1") [flags 0x41]
>><function_decl 0x7ffff7796d00 test_1>) [0 test_1 S4 A32])
>> (const_int 0 [0]))
>> (use (unspec:SI [
>> (const_int 0 [0])
>> ] UNSPEC_CALLEE_CC))
>> (clobber (reg:SI 1 ra))
>> ]) "j.c":14:9 441 {call_internal}
>> (expr_list:REG_CALL_DECL (symbol_ref:SI ("test_1") [flags 0x41]
>><function_decl 0x7ffff7796d00 test_1>)
>> (nil))
>> (expr_list:SI (use (reg:SI 10 a0))
>> (nil)))
>>
>> (code_label 13 12 14 4 2 (nil) [1 uses])
>>
>> (note 14 13 19 4 [bb 4] NOTE_INSN_BASIC_BLOCK)
>>
>> (insn 19 14 20 4 (set (reg/i:SI 10 a0)
>> (const_int 0 [0])) "j.c":18:1 276 {*movsi_internal}
>> (nil))
>>
>> (insn 20 19 24 4 (use (reg/i:SI 10 a0)) "j.c":18:1 -1
>> (nil))
>
>
>You delete insns 19 and 20 resulting in this:
>
>> (call_insn 12 11 13 3 (parallel [
>> (call (mem:SI (symbol_ref:SI ("test_1") [flags 0x41]
>><function_decl 0x7ffff7796d00 test_1>) [0 test_1 S4 A32])
>> (const_int 0 [0]))
>> (use (unspec:SI [
>> (const_int 0 [0])
>> ] UNSPEC_CALLEE_CC))
>> (clobber (reg:SI 1 ra))
>> ]) "j.c":14:9 441 {call_internal}
>> (expr_list:REG_CALL_DECL (symbol_ref:SI ("test_1") [flags 0x41]
>><function_decl 0x7ffff7796d00 test_1>)
>> (nil))
>> (expr_list:SI (use (reg:SI 10 a0))
>> (nil)))
>>
>> (code_label 13 12 14 4 2 (nil) [1 uses])
>>
>> (note 14 13 24 4 [bb 4] NOTE_INSN_BASIC_BLOCK)
>>
>> (note 24 14 0 NOTE_INSN_DELETED)
>
>Which is incorrect/inconsistent RTL. And as I've noted before, it's
>conceptually wrong for the backend code to be removing insns from the
>insn chain during prologue/epilogue generation.
>
>I realize you're trying to use a hook to limit how this impacts other
>targets, but if you're making a bad decision in the RISC-V backend code,
>working around it with a target hook rather than fixing the core problem
>in the RISC-V backend just makes the whole situation worse.
>
>My suggest is this. Leave the assignment to a0 and use alone. That's
>likely going to result in some kind of code size regression, but not a
>correctness regression. Then address the code size regressions with the
>invariant that prologue/epilogue generation must not change existing
>insns on the insn chain.
Hi Jeff
Thanks for your patience.
Got your point never remove insns from the
insn chain during prologue/epilogue generation.
As you suggested, I will fix this issue by leaving the assignment to a0 and use
insn
with cm.popret. Then optimize to cm.popretz in correct pass in future if
possilbe.
>jeff
>
>
>
>
>
>
>
>
>
>
