Re: [PATCH v4 1/3] [RFC] ifcvt: handle sequences that clobber flags in noce_convert_multiple_sets
On Wed, Jun 5, 2024 at 2:00 PM Richard Sandiford
wrote:
>
> Sorry for the slow review.
>
> Manolis Tsamis 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
> > ---
> >
> > (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,
> > -, _dest2, cc_cmp, rev_cc_cmp);
> > + {
> > + seq2 = try_emit_cmove_seq (if_info, temp, cond,
> > + new_val, old_val, need_cmov,
> > + , _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
Re: [PATCH v4 1/3] [RFC] ifcvt: handle sequences that clobber flags in noce_convert_multiple_sets
Sorry for the slow review.
Manolis Tsamis 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
> ---
>
> (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,
> -, _dest2, cc_cmp, rev_cc_cmp);
> + {
> + seq2 = try_emit_cmove_seq (if_info, temp, cond,
> + new_val, old_val, need_cmov,
> + , _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)
> + {
>
[PING] [PATCH v4 1/3] [RFC] ifcvt: handle sequences that clobber flags in noce_convert_multiple_sets
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
---
(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,
- , _dest2, cc_cmp, rev_cc_cmp);
+ {
+ seq2 = try_emit_cmove_seq (if_info, temp, cond,
+new_val, old_val, need_cmov,
+, _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;
+ }
/* 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
[PATCH v4 1/3] [RFC] ifcvt: handle sequences that clobber flags in noce_convert_multiple_sets
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
---
(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,
- , _dest2, cc_cmp, rev_cc_cmp);
+ {
+ seq2 = try_emit_cmove_seq (if_info, temp, cond,
+new_val, old_val, need_cmov,
+, _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;
+ }
/* 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
