RE: [PATCH] tcg: `reachable_code_pass()` remove empty else-branch

[Taylor Simpson]

> -----Original Message-----
> From: Anton Johansson <anjo@rev.ng>
> Sent: Friday, March 3, 2023 7:32 AM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Cc: ale@rev.ng; richard.henderson@linaro.org
> Subject: Re: [PATCH] tcg: `reachable_code_pass()` remove empty else-
> branch
> 
> On 3/1/23 23:37, Taylor Simpson wrote:
> >> diff --git a/tcg/tcg.c b/tcg/tcg.c
> >> index a4a3da6804..531bc74231 100644
> >> --- a/tcg/tcg.c
> >> +++ b/tcg/tcg.c
> >> @@ -2664,21 +2664,40 @@ static void reachable_code_pass(TCGContext
> *s)
> >>                   dead = false;
> >>                   remove = false;
> >>
> >> -                /*
> >> -                 * Optimization can fold conditional branches to 
> >> unconditional.
> >> -                 * If we find a label with one reference which is 
> >> preceded by
> >> -                 * an unconditional branch to it, remove both.  This 
> >> needed to
> >> -                 * wait until the dead code in between them was removed.
> >> -                 */
> >> -                if (label->refs == 1) {
> >> -                    TCGOp *op_prev = QTAILQ_PREV(op, link);
> > Can't we just insert a while loop here to move op_prev back across labels?
> >
> >      while (op_next->opc == INDEX_op_set_label) {
> >          op_prev = QTAILQ_PREV(op, op_prev);
> >      }
> >
> >> -                    if (op_prev->opc == INDEX_op_br &&
> >> -                        label == arg_label(op_prev->args[0])) {
> Ah I see, you're thinking something like
> 
>      dead = false;
>      remove = false;
> 
>      if (label->refs == 1) {
>          TCGOp *op_prev = NULL;
>          do {
>              op_prev = QTAILQ_PREV(op_prev, link);

You can't use op_prev as the argument here.  It is NULL the first time through 
the loop ☹

>          } while (op_prev->opc == INDEX_op_set_label);
> 
>          if (op_prev->opc == INDEX_op_br &&
>              label == arg_label(op_prev->args[0])) {
>              tcg_op_remove(s, op_prev);
>              remove = true;
>          }
>      }
> 
> to handle
> 
>      br
>      set_label
>      ...
>      set_label
> 
> I do like being able to combine both branches, and we're no longer relying on
> the next iteration of the loop to clean up the final set_label. Since we won't
> ever encounter more than one intermediate set_label this might be overkill,
> but either way I think it's an improvement.

It's overkill for the code that idef-parser generates, but my suggestion is to 
make able to skip back over as many labels as possible.

> 
> Thanks for the feedback, I'll resubmit with this change! :)
> 
> --
> Anton Johansson,
> rev.ng Labs Srl.