Re: Re: [PATCH 0/2] fix RISC-V zcmp popretz [PR113715]

["Fei Gao" <gaofei@eswincomputing.com>]

On 2024-06-05 21:58  Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
>On 6/5/24 1:47 AM, Fei Gao wrote:
>>
>> On 2024-06-05 14:36  Kito Cheng <kito.cheng@gmail.com> wrote:
>>>
>>> Thanks for fixing this issue, and I am wondering doest it possible to
>>> fix that without introduce target hook? I ask that because...GCC 14
>>> also has this bug, but I am not sure it's OK to introduce new target
>>> hook for release branch? or would you suggest we just revert patch to
>>> fix that on GCC 14?
>>
>> If hook is unacceptable in GCC14, I suggest to revert on GCC 14 the following commit.
>> https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=b27d323a368033f0b37e93c57a57a35fd9997864
>>
>> I started fixing this issue by adding changes in mach pass but abandoned it
>> due to the following reasons:
>> 1. more codes to detect location of epilogue in the whole insn list.
>> 2. due to impact by scheduling pass, clear a0 and use a0 insns get reordered, resulting in more
>>      codes.
>> 3. data flow analysis is needed, but insn does't have bb info any more, so rescan actually does
>>      nothing, which I guess there's some hidden issue in riscv_remove_unneeded_save_restore_calls
>>      using dfa.
>>
>> So I came up this hook based patch in prologue and epilogue pass to make the optimization
>> happen as earlier as possible. It ends up with simplicity and clear logic.
>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.
	
	issue of the commit above:
	Insertion of the generated epilogue sequence
	into the insn chain doesn't happen at this moment.
	If later shrink-wrap decides NOT to insert the epilogue sequence at the end
	of insn chain, then the li	a0,0 insn has already been mistakeny removed.  */
  rtx_insn *epilogue_seq = make_epilogue_seq ();

  /* Try to perform a kind of shrink-wrapping, making sure the
     prologue/epilogue is emitted only around those parts of the
     function that require it.  */
  /* feigao:
     Make a simple_return for those exits that run without prologue and
     force nonfallthru by e->flags &= ~EDGE_FALLTHRU	 */
  try_shrink_wrapping (&entry_edge, prologue_seq);

...

  edge exit_fallthru_edge = find_fallthru_edge (EXIT_BLOCK_PTR_FOR_FN (cfun)->preds);

  /* feigao: fasle here in simple_return case, so no insertion of epilogue,
     but the li	a0,0 insn has already been mistakeny removed.  */
  if (exit_fallthru_edge) 
    {
      if (epilogue_seq)
	{
	  insert_insn_on_edge (epilogue_seq, exit_fallthru_edge);
	  commit_edge_insertions ();

	  /* The epilogue insns we inserted may cause the exit edge to no longer
	     be fallthru.  */
	  FOR_EACH_EDGE (e, ei, EXIT_BLOCK_PTR_FOR_FN (cfun)->preds)
	    {
	      if (((e->flags & EDGE_FALLTHRU) != 0)
		  && returnjump_p (BB_END (e->src)))
		e->flags &= ~EDGE_FALLTHRU;
	    }

	  find_sub_basic_blocks (BLOCK_FOR_INSN (epilogue_seq));
	}
...

  if (epilogue_seq)
    {
      rtx_insn *insn, *next;

      /* Similarly, move any line notes that appear after the epilogue.
         There is no need, however, to be quite so anal about the existence
	 of such a note.  Also possibly move
	 NOTE_INSN_FUNCTION_BEG notes, as those can be relevant for debug
	 info generation.  */
      for (insn = epilogue_seq; insn; insn = next)
	{
	  next = NEXT_INSN (insn);
	  if (NOTE_P (insn)
	      && (NOTE_KIND (insn) == NOTE_INSN_FUNCTION_BEG))
	    reorder_insns (insn, insn, PREV_INSN (epilogue_seq));
	}
    }

  /* feigao: new hook */
  targetm.post_epilogue_proc (epilogue_seq); 
...

BR, 
Fei 

>
>So let's get a good explanation of the problem and perhaps we'll find a
>better way to solve it.
>
>jeff
>