| Message ID | 20240726104035.4100769-2-manolis.tsamis@vrull.eu |
|---|---|
| State | New |
| Headers | show |
| Series | ifcvt: Allow if conversion of arithmetic in basic blocks with multiple sets | expand |
Manolis Tsamis <manolis.tsamis@vrull.eu> writes: > This is an extension of what was done in PR106590. FWIW, I think that if a bug is worth mentioning in the commit message, it's worth tagging so the hooks pick it up (as you get a nice reverse-mapping then if anyone is looking at it and wondering if a follow-up occurred). CC'd Jakub too given he wrote that commit and maybe he wants to review. Fixed Robin's email in CC list too. > [...] thanks, sam
Sam, Jakub & Robin, We had an "OK for trunk" from Jeff for v4 (see https://gcc.gnu.org/pipermail/gcc-patches/2024-July/656907.html) and it has been two more weeks for this RESEND. I'll push this by end of this week unless I hear otherwise. Thanks, Philipp. On Fri, 26 Jul 2024 at 12:50, Sam James <sam@gentoo.org> wrote: > > Manolis Tsamis <manolis.tsamis@vrull.eu> writes: > > > This is an extension of what was done in PR106590. > > FWIW, I think that if a bug is worth mentioning in the commit message, > it's worth tagging so the hooks pick it up (as you get a nice > reverse-mapping then if anyone is looking at it and wondering if a > follow-up occurred). > > CC'd Jakub too given he wrote that commit and maybe he wants to review. > > Fixed Robin's email in CC list too. > > > [...] > > thanks, > sam
Philipp Tomsich <philipp.tomsich@vrull.eu> writes: Hi, > Sam, Jakub & Robin, > > We had an "OK for trunk" from Jeff for v4 (see > https://gcc.gnu.org/pipermail/gcc-patches/2024-July/656907.html) and > it has been two more weeks for this RESEND. > I'll push this by end of this week unless I hear otherwise. I'd ping Jeff for a quick re-review of v5 to make sure he's happy with the changes to address https://gcc.gnu.org/pipermail/gcc-patches/2024-July/656908.html. I can't comment on ifcvt changes to say if they're OK or not otherwise, sorry. > > Thanks, > Philipp. > > > On Fri, 26 Jul 2024 at 12:50, Sam James <sam@gentoo.org> wrote: >> >> Manolis Tsamis <manolis.tsamis@vrull.eu> writes: >> >> > This is an extension of what was done in PR106590. >> >> FWIW, I think that if a bug is worth mentioning in the commit message, >> it's worth tagging so the hooks pick it up (as you get a nice >> reverse-mapping then if anyone is looking at it and wondering if a >> follow-up occurred). >> >> CC'd Jakub too given he wrote that commit and maybe he wants to review. >> >> Fixed Robin's email in CC list too. >> >> > [...] >> >> thanks, >> sam
On Wed, Aug 7, 2024 at 8:57 AM Sam James <sam@gentoo.org> wrote: > > Philipp Tomsich <philipp.tomsich@vrull.eu> writes: > > Hi, > > > Sam, Jakub & Robin, > > > > We had an "OK for trunk" from Jeff for v4 (see > > https://gcc.gnu.org/pipermail/gcc-patches/2024-July/656907.html) and > > it has been two more weeks for this RESEND. > > I'll push this by end of this week unless I hear otherwise. > > I'd ping Jeff for a quick re-review of v5 to make sure he's happy with > the changes to address > https://gcc.gnu.org/pipermail/gcc-patches/2024-July/656908.html. > To answer Jeff's question, although that patch removes the checks for particular SRC codes, it adds noce_operand_ok (src) which checks side_effects_p (src) and may_trap_p (src). This handles the asm related things and is in line with what the rest of ifcvt does, so there was no change needed for that comment. > I can't comment on ifcvt changes to say if they're OK or not otherwise, > sorry. > > > > > Thanks, > > Philipp. > > > > > > On Fri, 26 Jul 2024 at 12:50, Sam James <sam@gentoo.org> wrote: > >> > >> Manolis Tsamis <manolis.tsamis@vrull.eu> writes: > >> > >> > This is an extension of what was done in PR106590. > >> > >> FWIW, I think that if a bug is worth mentioning in the commit message, > >> it's worth tagging so the hooks pick it up (as you get a nice > >> reverse-mapping then if anyone is looking at it and wondering if a > >> follow-up occurred). > >> > >> CC'd Jakub too given he wrote that commit and maybe he wants to review. > >> > >> Fixed Robin's email in CC list too. > >> > >> > [...] > >> > >> thanks, > >> sam
On 8/7/24 1:25 AM, Manolis Tsamis wrote: > On Wed, Aug 7, 2024 at 8:57 AM Sam James <sam@gentoo.org> wrote: >> >> Philipp Tomsich <philipp.tomsich@vrull.eu> writes: >> >> Hi, >> >>> Sam, Jakub & Robin, >>> >>> We had an "OK for trunk" from Jeff for v4 (see >>> https://gcc.gnu.org/pipermail/gcc-patches/2024-July/656907.html) and >>> it has been two more weeks for this RESEND. >>> I'll push this by end of this week unless I hear otherwise. >> >> I'd ping Jeff for a quick re-review of v5 to make sure he's happy with >> the changes to address >> https://gcc.gnu.org/pipermail/gcc-patches/2024-July/656908.html. >> > To answer Jeff's question, although that patch removes the checks for > particular SRC codes, it adds noce_operand_ok (src) which checks > side_effects_p (src) and may_trap_p (src). This handles the asm > related things and is in line with what the rest of ifcvt does, so > there was no change needed for that comment. Excellent. Thanks. As noted elsewhere, I see some evidence that the conditions we're using for "noce_can_force_operand" aren't really sufficient to tell us if we can force an operand into a register. Case in point is the v850 port where we can't force certain rotates into a register using force_operand. I've got a workaround as the port can be slightly improved and that improvement will make the problem go away. But I do suspect we're going to see additional issues in this space. jeff
diff --git a/gcc/ifcvt.cc b/gcc/ifcvt.cc index 58ed42673e5..58c34aaf1ee 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,71 @@ 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. */ + for (rtx_insn *iter = seq2; iter; iter = NEXT_INSN (iter)) + if (modified_in_p (cc_cmp, iter) + || (rev_cc_cmp && modified_in_p (rev_cc_cmp, iter))) + { + seq2 = NULL; + break; + } + } /* 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,22 +3809,32 @@ noce_convert_multiple_sets_1 (struct noce_if_info *if_info, return false; } - if (cc_cmp) - { - /* 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)) + /* Although we use temporaries if there is register overlap of COND and + TARGET, it is possible that SEQ modifies COND anyway. For example, + COND may use the flags register and if INSN clobbers flags then + we may be unable to emit a valid sequence (e.g. in x86 that would + require saving and restoring the flags register). */ + if (!second_try) + for (rtx_insn *iter = seq; iter; iter = NEXT_INSN (iter)) + if (modified_in_p (cond, iter)) { - 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; - } + end_sequence (); + return false; } + + if (cc_cmp && seq == seq1) + { + /* 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. */ + for (rtx_insn *iter = seq; iter; iter = NEXT_INSN (iter)) + if (modified_in_p (cc_cmp, iter) + || (rev_cc_cmp && modified_in_p (rev_cc_cmp, iter))) + { + cc_cmp = NULL_RTX; + rev_cc_cmp = NULL_RTX; + break; + } } /* End the sub sequence and emit to the main sequence. */
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. It also checks whether the resulting sequence clobbers the condition attached to the jump. 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. Punt if seq clobbers cond. Refactor the code that sets read_comparison. Signed-off-by: Manolis Tsamis <manolis.tsamis@vrull.eu> --- (no changes since v1) gcc/ifcvt.cc | 127 ++++++++++++++++++++++++++++++++------------------- 1 file changed, 79 insertions(+), 48 deletions(-)