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?
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;
> }
> }
