Re: [PATCH v4 1/3] [RFC] ifcvt: handle sequences that clobber flags in noce_convert_multiple_sets

[Manolis Tsamis <manolis.tsamis@vrull.eu>]

On Wed, Jun 5, 2024 at 2:00 PM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Sorry for the slow review.
>
> Manolis Tsamis <manolis.tsamis@vrull.eu> writes:
> > This is an extension of what was done in PR106590.
> >
> > Currently if a sequence generated in noce_convert_multiple_sets clobbers the
> > condition rtx (cc_cmp or rev_cc_cmp) then only seq1 is used afterwards
> > (sequences that emit the comparison itself). Since this applies only from the
> > next iteration it assumes that the sequences generated (in particular seq2)
> > doesn't clobber the condition rtx itself before using it in the if_then_else,
> > which is only true in specific cases (currently only register/subregister moves
> > are allowed).
> >
> > This patch changes this so it also tests if seq2 clobbers cc_cmp/rev_cc_cmp in
> > the current iteration. This makes it possible to include arithmetic operations
> > in noce_convert_multiple_sets.
> >
> > It also makes the code that checks whether the condition is used outside of the
> > if_then_else emitted more robust.
> >
> > gcc/ChangeLog:
> >
> >       * ifcvt.cc (check_for_cc_cmp_clobbers): Use modified_in_p instead.
> >       (noce_convert_multiple_sets_1): Don't use seq2 if it clobbers cc_cmp.
> >       Refactor the code that sets read_comparison.
> >
> > Signed-off-by: Manolis Tsamis <manolis.tsamis@vrull.eu>
> > ---
> >
> > (no changes since v1)
> >
> >  gcc/ifcvt.cc | 106 ++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 59 insertions(+), 47 deletions(-)
> >
> > diff --git a/gcc/ifcvt.cc b/gcc/ifcvt.cc
> > index 58ed42673e5..763a25f816e 100644
> > --- a/gcc/ifcvt.cc
> > +++ b/gcc/ifcvt.cc
> > @@ -3592,20 +3592,6 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)
> >    return true;
> >  }
> >
> > -/* Helper function for noce_convert_multiple_sets_1.  If store to
> > -   DEST can affect P[0] or P[1], clear P[0].  Called via note_stores.  */
> > -
> > -static void
> > -check_for_cc_cmp_clobbers (rtx dest, const_rtx, void *p0)
> > -{
> > -  rtx *p = (rtx *) p0;
> > -  if (p[0] == NULL_RTX)
> > -    return;
> > -  if (reg_overlap_mentioned_p (dest, p[0])
> > -      || (p[1] && reg_overlap_mentioned_p (dest, p[1])))
> > -    p[0] = NULL_RTX;
> > -}
> > -
> >  /* This goes through all relevant insns of IF_INFO->then_bb and tries to
> >     create conditional moves.  In case a simple move sufficis the insn
> >     should be listed in NEED_NO_CMOV.  The rewired-src cases should be
> > @@ -3731,36 +3717,67 @@ noce_convert_multiple_sets_1 (struct noce_if_info *if_info,
> >        creating an additional compare for each.  If successful, costing
> >        is easier and this sequence is usually preferred.  */
> >        if (cc_cmp)
> > -     seq2 = try_emit_cmove_seq (if_info, temp, cond,
> > -                                new_val, old_val, need_cmov,
> > -                                &cost2, &temp_dest2, cc_cmp, rev_cc_cmp);
> > +     {
> > +       seq2 = try_emit_cmove_seq (if_info, temp, cond,
> > +                                  new_val, old_val, need_cmov,
> > +                                  &cost2, &temp_dest2, cc_cmp, rev_cc_cmp);
> > +
> > +       /* The if_then_else in SEQ2 may be affected when cc_cmp/rev_cc_cmp is
> > +          clobbered.  We can't safely use the sequence in this case.  */
> > +       if (seq2 && (modified_in_p (cc_cmp, seq2)
> > +           || (rev_cc_cmp && modified_in_p (rev_cc_cmp, seq2))))
> > +         seq2 = NULL;
>
> It looks like this still has the problem that I mentioned in the
> previous round: that modified_in_p only checks the first instruction
> in seq2, not the whole sequence.  Or is that the intention?
>
Fixed in v5.

Thanks,
Manolis

> Thanks,
> Richard
>
> > +     }
> >
> >        /* The backend might have created a sequence that uses the
> > -      condition.  Check this.  */
> > +      condition as a value.  Check this.  */
> > +
> > +      /* We cannot handle anything more complex than a reg or constant.  */
> > +      if (!REG_P (XEXP (cond, 0)) && !CONSTANT_P (XEXP (cond, 0)))
> > +     read_comparison = true;
> > +
> > +      if (!REG_P (XEXP (cond, 1)) && !CONSTANT_P (XEXP (cond, 1)))
> > +     read_comparison = true;
> > +
> >        rtx_insn *walk = seq2;
> > -      while (walk)
> > +      int if_then_else_count = 0;
> > +      while (walk && !read_comparison)
> >       {
> > -       rtx set = single_set (walk);
> > +       rtx exprs_to_check[2];
> > +       unsigned int exprs_count = 0;
> >
> > -       if (!set || !SET_SRC (set))
> > +       rtx set = single_set (walk);
> > +       if (set && XEXP (set, 1)
> > +           && GET_CODE (XEXP (set, 1)) == IF_THEN_ELSE)
> >           {
> > -           walk = NEXT_INSN (walk);
> > -           continue;
> > +           /* We assume that this is the cmove created by the backend that
> > +              naturally uses the condition.  */
> > +           exprs_to_check[exprs_count++] = XEXP (XEXP (set, 1), 1);
> > +           exprs_to_check[exprs_count++] = XEXP (XEXP (set, 1), 2);
> > +           if_then_else_count++;
> >           }
> > +       else if (NONDEBUG_INSN_P (walk))
> > +         exprs_to_check[exprs_count++] = PATTERN (walk);
> >
> > -       rtx src = SET_SRC (set);
> > +       /* Bail if we get more than one if_then_else because the assumption
> > +          above may be incorrect.  */
> > +       if (if_then_else_count > 1)
> > +         {
> > +           read_comparison = true;
> > +           break;
> > +         }
> >
> > -       if (XEXP (set, 1) && GET_CODE (XEXP (set, 1)) == IF_THEN_ELSE)
> > -         ; /* We assume that this is the cmove created by the backend that
> > -              naturally uses the condition.  Therefore we ignore it.  */
> > -       else
> > +       for (unsigned int i = 0; i < exprs_count; i++)
> >           {
> > -           if (reg_mentioned_p (XEXP (cond, 0), src)
> > -               || reg_mentioned_p (XEXP (cond, 1), src))
> > -             {
> > -               read_comparison = true;
> > -               break;
> > -             }
> > +           subrtx_iterator::array_type array;
> > +           FOR_EACH_SUBRTX (iter, array, exprs_to_check[i], NONCONST)
> > +             if (*iter != NULL_RTX
> > +                 && (reg_overlap_mentioned_p (XEXP (cond, 0), *iter)
> > +                 || reg_overlap_mentioned_p (XEXP (cond, 1), *iter)))
> > +               {
> > +                 read_comparison = true;
> > +                 break;
> > +               }
> >           }
> >
> >         walk = NEXT_INSN (walk);
> > @@ -3788,21 +3805,16 @@ noce_convert_multiple_sets_1 (struct noce_if_info *if_info,
> >         return false;
> >       }
> >
> > -      if (cc_cmp)
> > +      if (cc_cmp && seq == seq1)
> >       {
> > -       /* Check if SEQ can clobber registers mentioned in
> > -          cc_cmp and/or rev_cc_cmp.  If yes, we need to use
> > -          only seq1 from that point on.  */
> > -       rtx cc_cmp_pair[2] = { cc_cmp, rev_cc_cmp };
> > -       for (walk = seq; walk; walk = NEXT_INSN (walk))
> > +       /* Check if SEQ can clobber registers mentioned in cc_cmp/rev_cc_cmp.
> > +          If yes, we need to use only seq1 from that point on.
> > +          Only check when we use seq1 since we have already tested seq2.  */
> > +       if (modified_in_p (cc_cmp, seq)
> > +           || (rev_cc_cmp && modified_in_p (rev_cc_cmp, seq)))
> >           {
> > -           note_stores (walk, check_for_cc_cmp_clobbers, cc_cmp_pair);
> > -           if (cc_cmp_pair[0] == NULL_RTX)
> > -             {
> > -               cc_cmp = NULL_RTX;
> > -               rev_cc_cmp = NULL_RTX;
> > -               break;
> > -             }
> > +           cc_cmp = NULL_RTX;
> > +           rev_cc_cmp = NULL_RTX;
> >           }
> >       }