[RISCV] redundant return-only BB not eliminated

[Robert Guthrie]

>Sometimes its better to ask for help as you work through these 
>things.  Ideally with before/after dumps and the patch you're working 
>with.  GCC is fairly complex and often even very experienced 
>developers need to throw things under the debugger to really 
>understand what's happening.
>

Thanks -- I didn't want to ask anyone to debug my stuff, and
anyways it took a bit of time but was straightforward. Most of the debugging
has just been me resolving my misunderstandings about what certain functions
do or variables are for, which is helpful for me.

>>
>>diff --git a/gcc/cfgcleanup.cc b/gcc/cfgcleanup.cc
>>index 1d9ec908dab..d74bfcf10e2 100644
>>--- a/gcc/cfgcleanup.cc
>>+++ b/gcc/cfgcleanup.cc
>>@@ -2831,14 +2831,39 @@ try_optimize_cfg (int mode)
>>               redirect_edge_succ (single_succ_edge (b),
>>                       EXIT_BLOCK_PTR_FOR_FN (cfun));
>>               single_succ_edge (b)->flags &= ~EDGE_CROSSING;
>>               changed_here = true;
>>             }
>>         }
>>
>>+          /* If we have two consecutive return-only basic blocks,
>>+         delete the second one, redirecting all of its predecessors
>>+         to the first one.  */
>>+          if (!(mode & CLEANUP_NO_INSN_DEL)
>>+          && bb_is_just_return (b, &ret, &use)
>>+          && bb_is_just_return (b->prev_bb, &ret, &use))
>So in general you don't want to be looking at prev_bb/next_bb.  Yes, 
>blocks are kept in a chain, but there's really no meaning to the order 
>of the chain until after block layout.  What really matters are the 
>pred/succ lists.
>
>So roughly
>
>/* Find predecessor of exit block that is just a return and nothing 
>else.  */
>FOR_EACH_EDGE (e, ei, EXIT_BLOCK_PTR_FOR_FN (cfun)->preds)
>  {
>    if (bb_is_just_return (e->pred))
>      {
>        target_bb = e->pred;
>        break;
>  }
>
>if (target_bb)
>  {
>    FOR_EACH_EDGE (e, ei, EXIT_BLOCK_PTR_FOR_FN (cfun)->preds)
>      {
>        if (e->pred == target_bb)
>          continue;
>
>        /* If the block unconditionally reaches EXIT_BLOCK, then 
>redirect it to TARGET_BB.  */
>        if (single_succ_p (e->pred))
>          {
>             /* CFG Manipulations */
>          }
>      }
>  }
>
>As I'm scribbling this code out, I do recall that we had this kind of 
>transformation.   I see the remnants of one in reorg.cc, but that's 
>not something I'd really suggest copying.  That doesn't really work 
>the CFG.  I seem to recall it was in jump.cc, but that's been largely 
>gutted and subsumed by cfgcleanup, but I don't see anything useful in 
>cfgcleanup.
>
>Jeff

I am a bit confused about this, I am probably missing something.
Let me try and clarify my understanding:
cfgcleanup.cc has effectively the inverse of this (in try_optimize_cfg):

         /* Try to change a branch to a return to just that return.  */
         rtx_insn *ret, *use;
         if (single_succ_p (b)
         && onlyjump_p (BB_END (b))
         && bb_is_just_return (single_succ (b), &ret, &use))
     {
         if (redirect_jump (as_a <rtx_jump_insn *> (BB_END (b)),
                     PATTERN (ret), 0))
         {
             if (use)
         emit_insn_before (copy_insn (PATTERN (use)),
                     BB_END (b));
             if (dump_file)
         fprintf (dump_file, "Changed jump %d->%d to return.\n",
                 b->index, single_succ (b)->index);
             redirect_edge_succ (single_succ_edge (b),
                     EXIT_BLOCK_PTR_FOR_FN (cfun));
             single_succ_edge (b)->flags &= ~EDGE_CROSSING;
             changed_here = true;
         }
     }

So I think your suggested transformation would be undone by this one,
if you were intending to keep the CFG in this form throughout
compilation like you suggested earlier.

Another thing is: even if we put the graph in this form, what mechanism would
ultimately delete the extraneous ret insns?  It would have to be "undone" at some point
(RET instructions inserted back into the linear sequence, rather than having every BB jump
to a single RET) at which point we might still end up with two RETs in a row in the linear stream.

Perhaps the second part of your code above could be like (I did not
compile this, just writing inline for ideas):

FOR_EACH_EDGE (e, ei, EXIT_BLOCK_PTR_FOR_FN (cfun)->preds)
   {
     basic_block b = e->src;
     if (b == target_bb || !bb_is_just_return (b)) continue;
     /* b is a ret-only block that is not `target_bb`, delete it and redirect
        its predecessors to `target_bb` */
     FOR_EACH_EDGE (e2, ei2, b->preds)
     {
       /* redirect e2 dst to target_bb */
     }
     delete_basic_block (b);
   }

I think something along these lines would maintain that we have at most one ret-only BB
(perhaps we also need to consider partitions here?).  Does this make sense?
(In this case you still might end up with a single extra RET insn, which I feel like you
can only deal with once near the end of compilation after bb-reorder when the linear sequence
is set, rather than trying to do it in the CFG -- I can elaborate on this but I've probably
said enough in this mail!).

Best
Robert