On Wed, Jun 5, 2024 at 2:00 PM Richard Sandiford
<richard.sandif...@arm.com> wrote:
>
> Sorry for the slow review.
>
> Manolis Tsamis <manolis.tsa...@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.tsa...@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?
>
Sorry for missing that. I was busy refactoring the read_comparison
related code and then forgot about modified_in_p...
>From what I checked just changing this to modified_between_p instead
should be enough. I will make the change now so I don't miss it when
submitting the next version.
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;
> > }
> > }
